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!

[EdJoPaTo]

Isn't the default of checkout to use the head? Could this be removed with the same outcome → simpler config?

[joshka]

As an aside - if there's something obvious like this that can be removed which would make you give an approval, feel free to slap the approval on and trust that the maintainers will do the right thing with the code. With all of us in various time zones, this helps move things along rather than PRs getting stagnated waiting for the next round of approval.

[orhun]

Isn't the default of checkout to use the head?

Nope, it checks out the merge commit as default: https://github.com/actions/checkout?tab=readme-ov-file#checkout-pull-request-head-commit-instead-of-merge-commit

I also used repository here since we want to check out from the source (forked repository) instead of destination.

Apparently I tried to think ahead of time here, it might work with/without these settings. I'd say let's give it a shot and see what happens :)

[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! 🤞