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

detect if API breaks on PR #727

Merged
merged 19 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from 17 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
45 changes: 45 additions & 0 deletions .github/workflows/change.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
name: Check for API breaks

on:
push:
branches:
- main
pull_request:
branches:
- main

jobs:
check:
runs-on: ubuntu-latest
permissions:
pull-requests: write
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0

- uses: actions/setup-python@v5
with:
python-version: "3.12"

- name: Check for API breaks
continue-on-error: true
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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?

id: check
run: |
pip install griffe
pip install -e .
griffe check "openfe" --verbose

- name: Post Comment on Failure
if: steps.check.outcome == 'failure'
uses: actions/github-script@v7
with:
script: |
const prNumber = context.payload.pull_request.number;
github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body: '🚨 API breaking changes detected! 🚨'
});
2 changes: 1 addition & 1 deletion openfe/analysis/plotting.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def plot_lambda_transition_matrix(matrix: npt.NDArray) -> Axes:
Notes
-----
Borrowed from `alchemlyb <https://github.com/alchemistry/alchemlyb/blob/master/src/alchemlyb/visualisation/mbar_matrix.py>`_
which itself borrows from `alchemical-analysis <https://github.com/MobleyLab/alchemical-analysis>`_.
which itself borrows from `alchemical-analysis <https://github.com/MobleyLab/alchemical-analysis>`_.
"""
num_states = len(matrix)

Expand Down
Loading