Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding cargo semver checks #369

Merged
merged 21 commits into from
Oct 9, 2024
Merged

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented Oct 3, 2024

Adding a check that will run cargo semver-checks against each PR.

This implements a check for PRs that don't bump the crate version. The check will be run between the HEAD of the PR and main. So any changes introduced in the PR vs main that break semver are picked up, and the PR will be labeled with breaking-change.

Currently can't test the actual adding of a label because (probably) the check needs to be in the main repo, rather than in a PR branch, to be allowed to get write access to the repo.

So sadly to actually test this we'll need to merge and see if it works. (hooray github actions dev workflow)

Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.86%. Comparing base (86ffe02) to head (ed47177).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #369   +/-   ##
=======================================
  Coverage   76.86%   76.86%           
=======================================
  Files          47       47           
  Lines        9436     9436           
  Branches     9436     9436           
=======================================
  Hits         7253     7253           
  Misses       1789     1789           
  Partials      394      394           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the last step of what we discussed in slack? namely:

  • if bump minor version = fail this CI when semver checks fail
  • if bump major version = job is just expected to fail? or not run?
  • if no version change, job adds tag if fails (will it report success in UI in this case?)

.github/workflows/semver-checks.yml Show resolved Hide resolved
@nicklan nicklan force-pushed the add-cargo-semver-checks branch from e6c0818 to b715a03 Compare October 7, 2024 23:04
@nicklan nicklan force-pushed the add-cargo-semver-checks branch from b715a03 to ed47177 Compare October 7, 2024 23:37
@nicklan nicklan marked this pull request as ready for review October 7, 2024 23:42
Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but curious if this job is red will it fail and add the label but stay red? can we get it to 'succeed' (when it fails semver) just so it doesn't look like CI is breaking for any breaking change?

@nicklan
Copy link
Collaborator Author

nicklan commented Oct 8, 2024

LGTM but curious if this job is red will it fail and add the label but stay red? can we get it to 'succeed' (when it fails semver) just so it doesn't look like CI is breaking for any breaking change?

The if: succeed() and if: failure(0 cases mean the job overall will succeed even if the check step fails

@zachschuermann
Copy link
Collaborator

LGTM but curious if this job is red will it fail and add the label but stay red? can we get it to 'succeed' (when it fails semver) just so it doesn't look like CI is breaking for any breaking change?

The if: succeed() and if: failure(0 cases mean the job overall will succeed even if the check step fails

Ah nice okay thanks!!

Copy link
Collaborator

@OussamaSaoudi-db OussamaSaoudi-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Sanity checked this using act. It failed on a far away commit due to a pub api change, but succeeded when treating "pr.base.sha" as the previous commit.

@nicklan
Copy link
Collaborator Author

nicklan commented Oct 9, 2024

LGTM. Sanity checked this using act. It failed on a far away commit due to a pub api change, but succeeded when treating "pr.base.sha" as the previous commit.

Thanks for the link to act. I will use that in the future so I don't go quite so insane :)

@nicklan nicklan merged commit 340c5e4 into delta-io:main Oct 9, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants