chore(ci): integrate cargo-semver-checks

[orhun]

cargo-semver-checks: Lint your crate API changes for semver violations.

It is intended to be run before publishing the crate but I don't feel like that is aligned with our workflow since we are already tracking the breaking changes in a separate document. That is why I integrated it into check-pr.yml where it is being run for PRs.

I had some experiments with cargo-semver-checks before and I think it is pretty useful. I have an open PR where I also improved the workflow a bit: obi1kenobi/cargo-semver-checks-action#65 - in the future we will be able to add comments to the PRs about semver violations.

Let me know what you think!

Inviting @obi1kenobi to the discussion 🐻

[EdJoPaTo]

I like to see cargo-semver-check being integrated!

[joshka]

I wonder if it might also be possible to make this ensure that a breaking PR has a !, check that there are changes in the breaking changes doc, and add a breaking change label.

+1 to remove the unnecessary checkout config

[obi1kenobi]

👋 Hi all! cargo-semver-checks maintainer here, thanks for checking it out.

@orhun is exactly right that the current action is best when used immediately before release, and that a better workflow we want to support in the future is running on each PR. We ran into some security concerns around being able to post comments and set labels safely after scanning an arbitrary PR, which e.g. might contain a malicious build.rs script. We obviously don't want a malicious PR to be able to steal a project's GitHub API key and take over.

This is a problem that I'm confident has a solution. I'm currently in the middle of a fundraising push aimed at securing funding to solve this problem and add several other missing pieces of functionality. Wish me luck! 🤞 If I'm successful, then PRs with breaking changes will be able to be labeled e.g. semver:major, they'll have inline comments describing the breakage on the lines where they happened, and will be able to optionally enforce the conventional commits ! as well as @joshka suggested.

Please ping me if you run into any issues and I'd be happy to help! If you find the tool helpful, I also love hearing about that too 😁 And if you are able to support its development, support on GitHub Sponsors goes a long way 🙏

[orhun]

I wonder if it might also be possible to make this ensure that a breaking PR has a !, check that there are changes in the breaking changes doc, and add a breaking change label.

Good idea! I think we can do that in a follow-up PR.

Hi all!

Hey @obi1kenobi! 👋🏼

I'm currently in the middle of a fundraising push

Best of luck! Very excited to see the future of cargo-semver-checks.

[EdJoPaTo]

We ran into some security concerns around being able to post comments and set labels safely after scanning an arbitrary PR, which e.g. might contain a malicious build.rs script. We obviously don't want a malicious PR to be able to steal a project's GitHub API key and take over.

Personal opinion: GitHub Actions should only run after approving it as every PR can add things to the .github/workflows/… or whatever. (sadly I can not set that automated for all my repos: https://github.com/orgs/community/discussions/35808)

[obi1kenobi]

I agree with the sentiment! I use two things in my own repos for this:

• A CODEOWNERS rule that ensures I personally must review any changes to .github/: https://github.com/obi1kenobi/cargo-semver-checks/blob/main/CODEOWNERS#L4-L9
• All PRs run only on the pull_request trigger and not on pull_request_target, which means PR changes to anything inside .github/ cannot see my repo's secrets etc. — only ones in their own fork instead.
  I've found this works well enough for me that I only require approvals for GitHub Actions runs for first-time contributors.

Unfortunately for our desired cargo-semver-checks workflow, GitHub currently doesn't allow leaving PR comments or setting labels from a pull_request trigger, only from pull_request_target. The easiest way I know of working around this is having a piece of infra that I maintain that runs a GitHub App which you authorize to comment on and add labels to your PRs. But obviously, I can't afford to pay out of pocket for thousands of people and companies' use of cargo-semver-checks. Hence the funding drive! 🤞

[EdJoPaTo]

Can it read the PR title and check whether a !: exists or not? Or read the labels?

When it exists, and it seems breaking, then the run can be fine. When it does not exist, and it isn't breaking, then it can be fine too. Only when this misaligns the run could fail in order to correct that title.

[obi1kenobi]

The cargo-semver-checks action by itself cannot do that at the moment. But I'd be happy to work with you and @orhun to put something like that together, and once it works well, I'd love to highlight it as a recommended workflow for all users of cargo-semver-checks!

[joshka]

Generally approving, with some docs needed.

[joshka]

Personal opinion: GitHub Actions should only run after approving it as every PR can add things to the .github/workflows/… or whatever. (sadly I can not set that automated for all my repos: https://github.com/orgs/community/discussions/35808)

I was under the impression that Github runs the workflows from the main branch, and not the PR itself. You can see that in action in this PR as the check-semver check isn't actually run https://github.com/ratatui-org/ratatui/actions/runs/10186995364. It's possibly I'm misunderstanding this however.

[orhun]

Ready to go.

You can see that in action in this PR as the check-semver check isn't actually run

I thought that's because the PR hasn't landed in main yet 🤔

[EdJoPaTo]

Currently, this will be annoyed at every PR after a single breaking change PR is merged into main. So we need to be extra careful not to merge breaking changes until directly before a breaking release, so this can actually be useful in its current state.

[obi1kenobi]

I recommended to @orhun that the cargo-semver-checks run be moved out of the workflow with the pull_request_target trigger and into a separate pull_request-triggered workflow.

The concern is this:

• cargo-semver-checks needs to compile your crates, which means that build.rs scripts in your crates and their dependencies execute.
• With the pull_request_target trigger, the GITHUB_TOKEN passed to workflows is your repo's (https://github.com/ratatui-org/ratatui) and not the PR's repo (https://github.com/someones-fork/ratatui).
• An attacker that adds a malicious dependency could use a build.rs script to steal the ratatui-org/ratatui repo's GITHUB_TOKEN and abuse it at will.

[obi1kenobi]

This looks good to me. The only other thing I might recommend is allowing this job to fail without blocking CI for now, to allow for breaking changes to be merged without immediately releasing a new major version.

Like @orhun pointed out, creating a variant of the cargo-semver-checks GitHub Action that is more PR-friendly instead of the current release-optimized one is a near-term TODO item for me.

[orhun]

The only other thing I might recommend is allowing this job to fail without blocking CI for now,

The last time I checked there wasn't an option for it: actions/runner#2347 (well, there is but AFAIK it is not shown as "failed" in the UI)

Do you know any workarounds for it?

[obi1kenobi]

Oh I just meant not marking it as a required check in a branch protection rule.

I think you'd still want it to look "failed" in the UI so that you know to inspect it when it first breaks.

[orhun]

Got it, we all good then!

[EdJoPaTo]

For a PR --baseline-rev=upstream/main should be useful. The action should be able to detect that from the context what the PR target branch is and use that as the argument here?

When the PR has a label or ! in the title it could use --release-type=major but that's something that should not be part of the action itself, only some input of release-type. But as major basically means don't care (does it?), the action could just be skipped in that case as it's a no-op then?

[orhun]

Maybe we can look for a way to ignore certain lints until the next release because we now made some breaking changes which means that our CI will be failing all the time...