detect if API breaks on PR

[mikemhenry]

Developers certificate of origin

• I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (6b3d1a7) to head (f24a252).
Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #727      +/-   ##
==========================================
- Coverage   93.42%   92.84%   -0.58%     
==========================================
  Files         134      134              
  Lines        9940     9940              
==========================================
- Hits         9286     9229      -57     
- Misses        654      711      +57     

Flag	Coverage Δ
fast-tests	92.84% <ø> (?)
slow-tests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[mikemhenry]

See https://mkdocstrings.github.io/griffe/checking/#detected-breakages

We get errors, but that is because we are breaking things, after the 1.0 tag, we can use this check to make sure we update the change log with breakage.

For example:
https://github.com/OpenFreeEnergy/openfe/actions/runs/7925490023/job/21638754579?pr=727#step:4:65

[IAlibay]

This is really cool @mikemhenry - I personally like it.

[IAlibay]

I can't remember what side effect this has anymore - does it mean that we get a green tick even if it fails? If so, we should make it loud and not a merge requirement imho. That way we can have the red tick and know that it was API breaking?

[mikemhenry]

Okay counteroffer, how about if the step fails (which indicates API breakage) a comment is added to the PR saying "This PR breaks API, be sure to add a note in the change log" I was thinking it would be annoying to have a red check if API breaks and you knew it was going to. That way the check will always be green, but it tells us in a more loud way then having to read a CI log file every time.

[IAlibay]

To do that you'll need to trigger a separate workflow iirc. That can be a bit messy / time consuming. Could this be a follow-up PR?

[mikemhenry]

I don't think it needs to trigger a separate workflow. I just added a bit to do that, it worked in 192b5e9 now I just need to add an API break to see if it does what we expect

[IAlibay]

@mikemhenry is it not that anything that needs secrets (like comment posting) will fail for PRs coming from non-org members unless it's in a pull_request_target workflow type?

[atravitz]

I like this! @mikemhenry could you just address this comment before merge?

[github-actions]

🚨 API breaking changes detected!. 🚨

[mikemhenry]

cool, bot posted the comment and the check stayed green, and this is what the output looks like on the action

We can refine the bot to update the comment/or do more, but it works!

[IAlibay]

Just as an FYI - last I tried this, it didn't work when the PR comes from a fork.

[mikemhenry]

You are correct, which is why we need to use pull_request_target

[mikemhenry]

From https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/

Having said that, mixing pull_request_target with an explicit PR checkout is not always vulnerable. The workflow may, for example:

• Reformat and commit the code
• Checkout both base and head repositories and generate a diff
• Run grep on the checked out source.

Generally speaking, when the PR contents are treated as passive data, i.e. not in a position of influence over the build/testing process, it is safe. But the repository owners must be extra careful not to trigger any script that may operate on PR controlled contents like in the case of npm install.

I believe now that we don't actually build the code in the PR, we should be okay.