Check cedar-policy SemVer compatibility on pull requests

[spinda]

Description of changes

This adds a GItHub Actions workflow to check changes to cedar-policy's public API against the base branch for SemVer compatibility, using cargo semver-checks.

Currently this depends on my fork of cargo-semver-checks-action, with changes I'll send upstream if this is deemed worth merging.

Issue #, if available

N/A

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

• A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

I confirm that this PR (choose one, and delete the other options):

• Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

• Does not require updates because my change does not impact the Cedar Dafny model or DRT infrastructure.

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[john-h-kastner-aws]

Is this blocking for a merge into main? I don't think that's what we want, at least with our current development practices.

[spinda]

I think you can change which status checks are required to pass for merging in the repository settings?

I figure it's better to surface this information at the individual PR level rather than in bulk on preparing to cut a release. At code review time, the results are more actionable: unintended API breakage can be immediately corrected by the PR author in the course of their normal workflow; intended breakage can be allowed through. Then each merge ends up annotated with whether it breaks SemVer, which could be used for deciding what's safe to cherry-pick into a release. If the checks are put off until it's time to actually cut a release, then fixing detected breakage would involve tracking down the origin of each incompatibility, looping in the responsible parties, and pulling them away from whatever they were doing to make corrections to work they already 'completed'. Scheduling and other pressures at release-time could make it tempting to take shortcuts or allow things through that we otherwise wouldn't. This was my takeaway from the Google paper on successfully implementing static analysis tools in developer workflows.

[john-h-kastner-aws]

Checking semver for each PR is good, but I would not want to be a position where we are regularly overriding an otherwise blocking CI failure due to an expected semver break.

If there's an option to treat this one check as a warning, then that's good by me.

[spinda]

Looks like we could set continue-on-error on the cargo_semver_checks job to accomplish that.

[spinda]

I set continue-on-error as above and swapped out the custom action for a few run: lines.

[spinda]

I tacked on sample commit that breaks SemVer compatibility. How does that look?

[andrewmwells-amazon]

I tacked on sample commit that breaks SemVer compatibility. How does that look?

LGTM

[andrewmwells-amazon]

Checking semver for each PR is good, but I would not want to be a position where we are regularly overriding an otherwise blocking CI failure due to an expected semver break.

If there's an option to treat this one check as a warning, then that's good by me.

As I understand it, this will check for symver breaks just for this PR's change. So I don't think we'd have to override the merge continually (as we would if it checked for symver against e.g. the latest tagged commit starting with "v").

Personally I'm fine with this. It's similar to our process for overriding changes that will break cedar-spec. I view this as a sanity check where the failing action reminds you to check that the changelog is modified in this PR before overriding.

[aaronjeline]

I think it's at least worth trying. It's not hard to undo if we end up not liking the process.

[spinda]

I've made the check optional for normal PRs and mandatory for PRs into release/* branches. For normal PRs, the SemVer check is relative to the code in the target branch, so existing SemVer breakage is ignored and only new breakage introduced by the PR should be surfaced. For release PRs, the SemVer check is relative to the latest version published to crates.io <= the version in the PR's cedar-policy/Cargo.toml.

This PR should currently be showing up with a failed SemVer check that doesn't block merging. I submitted #172 against release/2.3.x, which should be showing up with a failed SemVer check that does block merging.

@andrewmwells-amazon @john-h-kastner-aws If this looks good to y'all (I can't confirm from my end whether the checks are properly requiring/not-requiring overrides), then I'll remove the breakage-inducing commit and this'll be ready to merge.

[john-h-kastner-aws]

Seems alright to me.

Two comments the CI script.

[john-h-kastner-aws]

After approving I see

which looks good to me. It clearly signals that something is failing, but the merge is possible without override.

[andrewmwells-amazon]

If this looks good to y'all (I can't confirm from my end whether the checks are properly requiring/not-requiring overrides), then I'll remove the breakage-inducing commit and this'll be ready to merge.

Seems good to me (modulo comments)

[spinda]

This should be good to go!

[spinda]

Resolved a merge conflict.