Semver checks #909

Lorak-mmk · 2024-01-10T22:45:02Z

This PR introduces cargo-semver-checks to our CI.
The behavior is very similar to what I described in the issue: #906

On a PR, it sets semver-checks-breaking label if the PR breaks API (compared to PR base). Report from the tool, containing info about incompatibilities, is posted as a comment.
When a tag is pushed, the job will fail if update type (major / minor / patch) was incorrect. This requires new version to NOT be published to crates.io. So we should wait for the workflow to finish before publishing.

Apart from CI integration this PR also modifies Makefile and CONTRIBUTING.MD to make it easier for contributors to run checks locally.

cargo-semver-checks has some false-negatives and false-positives, so it is NOT a replacement for a reviewer.
Reviewer still needs to check the code to see if it's API-breaking. semver-checks should still be a big help with that, catching many cases that
are very hard to identify manually or that people forget about.

Security:

PR CI uses pull_request_target trigger which can receive read-write GITHUB_TOKEN (while pull_request can't if a PR is from a fork - which is a vast majority of PRs).
This is required in order to be able to add/remove label to a PR. It makes such jobs a valuable target for attackers.

Workflows using pull_request_target are run in the context of base of a PR instead of head of a PR (like pull_request workflows do). This prevents the attacker from modifying the workflow file.

PR CI is split into two parts. First, unprivileged, job (called semver-pull-request-check) performs actual check, which is an insecure process (for example: attacker could add malicious build.rs file).
For this reason, this first job has no permissions (permissions: {} attribute), so taking over it should not give anything interesting to the attacker.

The second, privileged, job (called semver-pull-request-label) has write permissions for pull requests and its only task is adding or removing a label to/from a PR.
It checks the output from the first job and labels a PR based on that. The only thing done with outputs from unprivileged job is a simple comparison with "0",
so modifying those outputs will not result in takeover of privileged job.

As far as I can tell, from GitHub docs each job is run on a clean VM, so taking over unprivileged job should not allow attacker to take over privileged job.

Fixes: #906

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
~~I added relevant tests for new features and bug fixes.~~
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
~~I have provided docstrings for the public items that I want to introduce.~~
~~I have adjusted the documentation in ./docs/source/.~~
I added appropriate Fixes: annotations to PR description.

Lorak-mmk · 2024-01-12T16:38:14Z

v2:
Moved commands to a separate script, called by CI and newly introduced Makefile targets.
This is because invocation of semver-checks may become more complicated, e.g. when #911 gets merged, as it introduces mutually-exclusive features.
It is also just easier for developer to use Makefile targets than longer semver-checks invocation.

Documented semver-checks in CONTRIBUTING.md.

Lorak-mmk · 2024-01-12T16:40:46Z

v3: Noticed that I didn't install semver-checks in push action... Well, I did say I have no idea how to test it.

Lorak-mmk · 2024-01-12T16:49:30Z

TODO / Considerations:

We are before 1.0, so we don't have patch versions. After we release 1.0, it may be beneficial to replace semver-checks-breaking label with semver-major-required / semver-minor-required / semver-patch-required trio, so we know exactly which type of release to perform.
I tested that PR job works, both for adding and removing label. I didn't test the push job - I don't see any way to do it. I'll push a testing tag after PR is merged, and if it turns out to not work I'll fix it.
Official semver-checks action (which is not suitable for our usage) installs semver-checks from a github release binary instead of using cargo install. I think we can skip this for now, as we have pipelines that take more time anyway. Added a comment in workflow file about this.

Lorak-mmk · 2024-01-28T20:22:25Z

v5:

Addresed review comments (responded to some, changed the PR accordingly for the rest)
Added a report mechanism. You can see example comment here: Semver checks example PR #920 (comment) . Message in this can of course be changed. I'm also not sure if I should add posting a comment that there were no incompatibilities? I think it's reduntant and would increase spam/

.github/workflows/semver_checks.yml

CONTRIBUTING.md

piodul · 2024-01-29T16:41:51Z

.github/workflows/semver_checks.yml

+    - uses: actions/checkout@v3
+    - name: Install semver-checks
+      run: cargo install cargo-semver-checks --no-default-features
+    - name: Verify that's it's safe to publish the version.


I think this label implies too much, that the tool does really exhaustive checks and its assessment about semver compatibility can be trusted.

Changed. Is the current version better?

piodul · 2024-01-29T16:52:02Z

I'm also not sure if I should add posting a comment that there were no incompatibilities? I think it's reduntant and would increase spam

If the workflow removes the semver-checks-breaking label after declaring that there are no incompatibilities, then I think it is sufficient and there is no need for the additional comment.

This allows to run the tool manually. Right now the script doesn't do much, but when we want to run it with various feature sets or other flags it will be more useful.

This workflow looks for breaking API changes using popular cargo-semver-checks tool. This comit introduces it only for PRs. If PR breaks API, appropriate label is added, otherwise it is removed.

Add a job that checks compatibility when a tag is pushed. It gets current version from Cargo.toml, calculates previous version (unless current version is on crates.io - which should not be the case) and checks that the release type (major / minor / patch) is correct.

This is to avoid a race condition where a job scheduled for a first push finished after the job scheduled for a second push, resulting in incorrectly assigning / removing a label to PR.

Lorak-mmk · 2024-02-01T23:52:16Z

v6: Addressed review comments, added concurrency control

piodul

Looks good. Please drop the "TMP" commit and I'll merge.

Lorak-mmk · 2024-02-02T11:17:38Z

Looks good. Please drop the "TMP" commit and I'll merge.

Done

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 10, 2024

Lorak-mmk force-pushed the semver-checks branch from d00b919 to d3ede9f Compare January 10, 2024 22:53

github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 10, 2024

Lorak-mmk force-pushed the semver-checks branch from d3ede9f to f86bb91 Compare January 10, 2024 23:30

Lorak-mmk marked this pull request as ready for review January 10, 2024 23:40

Lorak-mmk requested a review from piodul January 10, 2024 23:40

Lorak-mmk force-pushed the semver-checks branch from f86bb91 to 63647aa Compare January 12, 2024 16:36

Lorak-mmk force-pushed the semver-checks branch from 63647aa to 70fc2d5 Compare January 12, 2024 16:40

Lorak-mmk force-pushed the semver-checks branch from 70fc2d5 to fa1ad25 Compare January 12, 2024 16:51

avelanarius assigned Lorak-mmk Jan 15, 2024

Lorak-mmk force-pushed the semver-checks branch from fa1ad25 to 7e69c96 Compare January 15, 2024 09:49

Lorak-mmk force-pushed the semver-checks branch 16 times, most recently from b560b84 to ee643d8 Compare January 24, 2024 23:13

Lorak-mmk force-pushed the semver-checks branch 7 times, most recently from 9eefa4e to 0502fa1 Compare January 28, 2024 20:18

Lorak-mmk requested a review from piodul January 28, 2024 20:23

piodul reviewed Jan 29, 2024

View reviewed changes

.github/workflows/semver_checks.yml Show resolved Hide resolved

.github/workflows/semver_checks.yml Show resolved Hide resolved

piodul reviewed Jan 29, 2024

View reviewed changes

Lorak-mmk added 5 commits February 2, 2024 00:28

Add script and makefile enteries for cargo-semver-checks

17302ec

This allows to run the tool manually. Right now the script doesn't do much, but when we want to run it with various feature sets or other flags it will be more useful.

CI: Introduce cargo-semver-checks

509941a

This workflow looks for breaking API changes using popular cargo-semver-checks tool. This comit introduces it only for PRs. If PR breaks API, appropriate label is added, otherwise it is removed.

Contributing.md: add info about semver checks

1e4c551

CI: Post output from semver-checks as a comment

4297204

Lorak-mmk force-pushed the semver-checks branch 2 times, most recently from b180328 to d190123 Compare February 1, 2024 23:44

CI: Add concurrency control to semver-checks PR job

f6d68d3

This is to avoid a race condition where a job scheduled for a first push finished after the job scheduled for a second push, resulting in incorrectly assigning / removing a label to PR.

Lorak-mmk force-pushed the semver-checks branch from d190123 to 8fb2355 Compare February 1, 2024 23:48

Lorak-mmk requested a review from piodul February 1, 2024 23:54

piodul approved these changes Feb 2, 2024

View reviewed changes

Lorak-mmk force-pushed the semver-checks branch from 8fb2355 to f6d68d3 Compare February 2, 2024 11:17

piodul approved these changes Feb 2, 2024

View reviewed changes

piodul merged commit f45887e into main Feb 2, 2024
8 checks passed

Lorak-mmk mentioned this pull request Feb 2, 2024

Release 0.12 #924

Closed

Lorak-mmk mentioned this pull request Sep 16, 2024

Add advanced/extended CI for gocql scylladb/gocql#204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semver checks #909

Semver checks #909

Lorak-mmk commented Jan 10, 2024 •

edited

Loading

Lorak-mmk commented Jan 12, 2024 •

edited

Loading

Lorak-mmk commented Jan 12, 2024

Lorak-mmk commented Jan 12, 2024 •

edited

Loading

Lorak-mmk commented Jan 28, 2024

piodul Jan 29, 2024

Lorak-mmk Feb 1, 2024

piodul commented Jan 29, 2024

Lorak-mmk commented Feb 1, 2024

piodul left a comment

Lorak-mmk commented Feb 2, 2024

Semver checks #909

Semver checks #909

Conversation

Lorak-mmk commented Jan 10, 2024 • edited Loading

Pre-review checklist

Lorak-mmk commented Jan 12, 2024 • edited Loading

Lorak-mmk commented Jan 12, 2024

Lorak-mmk commented Jan 12, 2024 • edited Loading

Lorak-mmk commented Jan 28, 2024

piodul Jan 29, 2024

Choose a reason for hiding this comment

Lorak-mmk Feb 1, 2024

Choose a reason for hiding this comment

piodul commented Jan 29, 2024

Lorak-mmk commented Feb 1, 2024

piodul left a comment

Choose a reason for hiding this comment

Lorak-mmk commented Feb 2, 2024

Lorak-mmk commented Jan 10, 2024 •

edited

Loading

Lorak-mmk commented Jan 12, 2024 •

edited

Loading

Lorak-mmk commented Jan 12, 2024 •

edited

Loading