-
-
Notifications
You must be signed in to change notification settings - Fork 36
chore(ci): run cargo semver checks on PRs #366
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
Conversation
seqre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @kumarUjjawal! Instead of it being a separate workflow, please add it to our Rust CI workflow as another job!
Ok Sure! |
|
@seqre Should I add if after the |
|
@kumarUjjawal, just add it at the end, but make it require our |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Flags with carried forward coverage won't be shown. Click here to find out more. see 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two cents from me:
- I don't think we should compare with the latest tag. We should compare with PR's destination branch (usually
master, but could be something else), so that we only get warnings/errors about current PR, rather than everything since the last release (which isn't very informative, especially when there's a lot of changes since the last release). - I think it would be ideal for this to create a GitHub comment with a summary of the checks (similarly to what codecov creates). However, if this would require a non-trivial amount of work, I'm fine with keeping this as is now—as a check in our workflow (which fails if there's a non-zero number of errors). (any thoughts on this @seqre?)
Does this aligns with what you intended?
I will wait for @seqre for the comments about this one. |
seqre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the above question, I'd skip the automatic comment creation for now. In most cases, there should not be violations, so that comment would only clutter the conversation. In the few instances where there is an unexpected violation, we can simply check the job logs.
seqre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We'll just have to test it now and see if it works
m4tx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kumarUjjawal the workflow does not run:
The workflow is not valid. .github/workflows/rust.yml (Line: 384, Col: 9): Unexpected value '' .github/workflows/rust.yml (Line: 385, Col: 5): Unexpected value 'RUSTC_WRAPPER'
https://github.com/cot-rs/cot/actions/runs/16079459652
Please fix the issue and then we should be able to merge this.
1153277 to
d4a0ef6
Compare
There was an identation issue, should work now. |
m4tx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks a lot for the contribution!
Description
Added a new Github workflow to run
cargo semvar checkson every PR.Closes #357