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

Semver checks #909

Merged
merged 6 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions .github/workflows/semver_checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
# This workflow tests semver compatibilty.
# For PRs it checks if PR makes any API breaking changes, and assings appropriate label if so.
name: Semver checks

on:
pull_request_target:
branches:
- main
- 'branch-*'
push:
tags:
- v*.*.*

env:
CARGO_TERM_COLOR: always
RUST_BACKTRACE: full
PR_BASE: ${{ github.event.pull_request.base.sha }}
PR_HEAD: ${{ github.event.pull_request.head.sha }}
PR_ID: ${{ github.event.number }}

jobs:
semver-pull-request-check:
runs-on: ubuntu-latest
if: github.event_name == 'pull_request_target'
# Disable all permissions
# This is important, because this job runs on untrusted input from
# the user and it's possible for the user to take over the job,
# for example by adding malicious build.rs file. If the job had,
# for example, `pull_requests: write` permission, malicous user
# could do us a lot of harm. This is also the reason that there are
# 2 jobs - it's so that it's not possible to take over a job that
# has permissions.
permissions: {}
timeout-minutes: 30
# This is to prevent a situation, when job A triggered by push 1 finishes
# after job B triggered by push 2. That could result in incorrectly assigning
# or removing a PR label.
concurrency:
# Can't use `env.PR_ID` because concurrency doesn't have access to env context.
group: semver-pull-request-check-${{ github.event.number }}
cancel-in-progress: true
outputs:
exitcode: ${{ steps.semver-pr-check.outputs.exitcode }}
output: ${{ steps.semver-pr-check.outputs.output }}
steps:
- name: Checkout
uses: actions/checkout@v3
with:
fetch-depth: "2"
ref: "refs/pull/${{ env.PR_ID }}/merge"
Lorak-mmk marked this conversation as resolved.
Show resolved Hide resolved
# Check if there was another push before this job started.
# If there was, wrong commit would be checked out.
- name: Sanity check
run: |
[[ "$(git rev-parse 'HEAD^2')" == "$PR_HEAD" ]]
# I don't know any way to do this using checkout action
- name: Fetch PR base
run: git fetch origin "$PR_BASE"
piodul marked this conversation as resolved.
Show resolved Hide resolved
- name: Install semver-checks
# Official action uses binary releases fetched from GitHub
# If this pipeline becomes too slow, we should do this too
run: cargo install cargo-semver-checks --no-default-features
Comment on lines +60 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should, building it from scratch on every push sounds like a waste of resources.

If building it currently doesn't take too long, then I suppose we can leave it for a follow-up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Other jobs are definitely longer, so this is not a problem now, but I agree we should definitely do this at some point.

- name: Verify the API compatibilty with PR base
id: semver-pr-check
run: |
set +e
echo "output<<SEMVER_STDOUT_EOF" >> $GITHUB_OUTPUT
# Weird sed strip ANSI colors from output
make semver-rev rev="$PR_BASE" |& tee /dev/tty | sed -r "s/\x1B\[([0-9]{1,2}(;[0-9]{1,2})?)?[mGK]//g" >> $GITHUB_OUTPUT
piodul marked this conversation as resolved.
Show resolved Hide resolved
exitcode=${PIPESTATUS[0]}
echo "SEMVER_STDOUT_EOF" >> $GITHUB_OUTPUT
echo "exitcode=$exitcode" >> $GITHUB_OUTPUT
exit "$exitcode"
continue-on-error: true

semver-pull-request-label:
runs-on: ubuntu-latest
if: github.event_name == 'pull_request_target'
permissions:
pull-requests: write
needs: semver-pull-request-check
timeout-minutes: 3
steps:
- name: Remove breaking label on success
run: gh pr edit "$PR_ID" --remove-label semver-checks-breaking
if: needs.semver-pull-request-check.outputs.exitcode == '0'
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GH_REPO: ${{ github.repository }}
- name: Add breaking label on failure
run: gh pr edit "$PR_ID" --add-label semver-checks-breaking
if: needs.semver-pull-request-check.outputs.exitcode != '0'
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GH_REPO: ${{ github.repository }}
Lorak-mmk marked this conversation as resolved.
Show resolved Hide resolved
- name: Post report on semver break
run: |
gh pr comment "$PR_ID" --body "\
\`cargo semver-checks\` detected some API incompatibilities in this PR.
See the following report for details:
<details>
<summary>cargo semver-checks output</summary>

\`\`\`
$SEMVER_OUTPUT
\`\`\`

</details>
"
if: needs.semver-pull-request-check.outputs.exitcode != '0'
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
GH_REPO: ${{ github.repository }}
SEMVER_OUTPUT: ${{ needs.semver-pull-request-check.outputs.output }}


semver-push-tag:
runs-on: ubuntu-latest
if: github.event_name == 'push'
timeout-minutes: 30
steps:
- uses: actions/checkout@v3
- name: Install semver-checks
run: cargo install cargo-semver-checks --no-default-features
- name: Run semver-checks to see if it agrees with version updates
run: make semver-version
16 changes: 16 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,22 @@ There are a few scenarios:
using command like `cargo update -p toml_datetime --precise 0.6.3` and go back to step 3.
5. Rename `Cargo.lock` to `Cargo.lock.msrv`.

### Semver checking

Our CI runs cargo semver-checks and labels PRs that introduce breaking changes.
If you don't intend to change public API, you can perform the checks locally,
using command `make semver-rev`. Make sure you have semver-checks installed first,
you can install it using `cargo install cargo-semver-checks`.

`make semver-rev` will check for API breaking changes using `main` branch as baseline.
To use different branch / commit call `make semver-rev rev=BRANCH`.
Lorak-mmk marked this conversation as resolved.
Show resolved Hide resolved

The tool is NOT perfect and only checks some aspect of semver-compatibility.
It is NOT a replacement for a human reviewer, it is only supposed to help them
and catch some erros that they might have missed.

Tool that we curently use: https://github.com/obi1kenobi/cargo-semver-checks

## Contributing to the book

The documentation book is written using [mdbook](https://github.com/rust-lang/mdBook)\
Expand Down
8 changes: 8 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ build:
docs:
mdbook build docs

.PHONY: semver-rev
semver-rev:
./scripts/semver-checks.sh $(if $(rev),--baseline-rev $(rev),--baseline-rev main)

.PHONY: semver-version
semver-version:
./scripts/semver-checks.sh $(if $(version),--baseline-version $(version),)

.PHONY: up
up:
$(COMPOSE) up -d --wait
Expand Down
5 changes: 5 additions & 0 deletions scripts/semver-checks.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/bin/sh

set -x

cargo semver-checks -p scylla -p scylla-cql $@
Lorak-mmk marked this conversation as resolved.
Show resolved Hide resolved
Loading