Skip to content

Add CI for api surface area review verification #6099

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 15, 2025
Merged

Add CI for api surface area review verification #6099

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d824400
Add CI for api surface area review verification
zoewangg May 13, 2025
56e1bc9
Merge branch 'master' into zoewang/api-surface-area-review
zoewangg May 15, 2025
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
36 changes: 36 additions & 0 deletions .github/workflows/api-surface-area-review-verification.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: API Surface Area Review Verification

on:
pull_request:
types: [ opened, synchronize, reopened, labeled, unlabeled ]
branches:
- master

jobs:
api-surface-area-review-verification:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Verifies updates to protected/public APIs have been reviewed and approved by the team, if any
id: api-surface-area-review-verification
if: ${{ !contains(github.event.pull_request.labels.*.name, 'no-api-surface-area-change') }}
run: |
git fetch origin ${{ github.base_ref }} --depth 1
FILES=$( git diff remotes/origin/${{ github.base_ref }} --name-only | grep "\.java$" | grep -v -E "(^|/)(internal|test|codegen|v2-migration)/" || true)
Copy link
Contributor

@joviegas joviegas May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nice CI action +1
I had below suggestions

  1. Would it be possible to narrow down the checks to only files containing SdkPublicInterface and SdkProtectedInterface annotations that way wee need not worry of skipping/adding the directories?
  2. Could we enhance the output to include the specific file names that failed the check?
  3. Would it be possible to add a tag to the PR using github.rest.issues.addLabels ? This way we can mark it as 'api-surface-area-review-required'

Copy link
Contributor Author

@zoewangg zoewangg May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. It's just more complexity and maintenance to inspect the exact diff for each file and may increase the check time. IMO we really shouldn't expand on internal|test|codegen|v2-migration since this repo is already huge, so I'm not sure if the effort it worth it.
  2. Yes, it is in the output.https://github.com/aws/aws-sdk-java-v2/pull/6099/files#diff-03e23d2b8990386383cd6d9efa82ad2dd53af6d72b4f33a6be1d0932442b71c1R21
  3. I actually thought about it and tried with the GH labeler action, I didn't go with that because it's a bit trickier to flag false positives since we want it to always automatically add api-surface-area-review-required (it's not straightforward to tell it to not re-add the label for new revision if the label was removed previously) and having both api-surface-area-review-required and no-api-surface-area-change is a bit confusing

if [ -n "$FILES" ]; then
echo "::error::Changes around protected/public APIs found:"
echo "$FILES" | while read file; do
echo "::error::$file"
done
echo "has_matches=true" >> $GITHUB_OUTPUT
else
echo "No changes around protected/public APIs found."
echo "has_matches=false" >> $GITHUB_OUTPUT
fi
- name: Fail if there are changes around protected/public APIs and there's no label
if: ${{ steps.api-surface-area-review-verification.outputs.has_matches == 'true' && !contains(github.event.pull_request.labels.*.name, 'api-surface-area-approved-by-team') }}
run: |
echo "::error ::Change around public/protected APIs has been detected. Please either:"
echo "::error ::* Review it with the team and add the 'api-surface-area-reviewed' label to this PR after approval –or–"
echo "::error ::* Add the 'no-api-surface-area-change' label to this PR in case this is a false positive"
exit 1
Loading