chore(dev): static api analysis #6159
Conversation
|
|
…g api change reports and requiring additional approval if needed
ekjotmultani
force-pushed
the
chore/api-analysis
branch
from
93ffb4a to
f5f5157
Compare
|
|
|
|
… assuming we are at the root of the repo
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
…and look specifically for the text that tells us if anything changed
|
|
…mplify-flutter into chore/api-analysis
…nsistency from local to gh actions
…flow instead of comparing diffs
…into chore/api-analysis
packages/aft/lib/src/commands/generate/generate_api_report_command.dart
Outdated
Show resolved
Hide resolved
packages/aft/lib/src/commands/generate/generate_api_report_command.dart
Outdated
Show resolved
Hide resolved
packages/aft/lib/src/commands/generate/generate_api_report_command.dart
Outdated
Show resolved
Hide resolved
…e for parsing pubspecs
…have consistent ordering when viewing change reports
tyllark
left a comment
tyllark
left a comment
There was a problem hiding this comment.
Can you update the AFT ReadMe to reference this new command.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: Label API Changes | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Check for API changes | ||
| id: check-api-changes | ||
| run: | | ||
| # Check for changes in API JSON files | ||
| API_JSON_CHANGES=$(git diff --name-only origin/main...HEAD | grep -E 'packages/aft/api_reports/.*\.api\.json' || true) | ||
|
|
||
| if [[ -n "$API_JSON_CHANGES" ]]; then | ||
| echo "API_CHANGES_DETECTED=true" >> $GITHUB_ENV | ||
| echo "API changes detected in the following files:" | ||
| echo "$API_JSON_CHANGES" | ||
| else | ||
| echo "API_CHANGES_DETECTED=false" >> $GITHUB_ENV | ||
| echo "No API changes detected." | ||
| fi | ||
|
|
||
| - name: Add API changes label | ||
| if: env.API_CHANGES_DETECTED == 'true' | ||
| uses: actions/github-script@v6 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| github.rest.issues.addLabels({ | ||
| issue_number: context.issue.number, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| labels: ['api-changes'] | ||
| }) | ||
|
|
||
| - name: Comment on PR | ||
| if: env.API_CHANGES_DETECTED == 'true' | ||
| uses: actions/github-script@v6 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| // Check if we already commented about API changes | ||
| const commentMarker = '⚠️ This PR contains API changes'; | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| issue_number: context.issue.number, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo | ||
| }); | ||
|
|
||
| // Look for an existing comment with our marker | ||
| const apiChangeComment = comments.find(comment => | ||
| comment.body.includes(commentMarker) && | ||
| comment.user.login === 'github-actions[bot]' | ||
| ); | ||
|
|
||
| // Only create a comment if we haven't already | ||
| if (!apiChangeComment) { | ||
| await github.rest.issues.createComment({ | ||
| issue_number: context.issue.number, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| body: '⚠️ This PR contains API changes and requires careful review. Please ensure these changes are intentional and backward compatible where possible.' | ||
| }); | ||
| } else { | ||
| console.log('API changes comment already exists, skipping comment creation'); | ||
| } No newline at end of file |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, this problem is fixed by explicitly adding a permissions block to the workflow (at the root or per job) so that the GITHUB_TOKEN has only the minimal scopes needed. This prevents the token from inheriting potentially broad repository or organization defaults.
For this specific workflow, the job needs:
- To check out code and run
git diff→contents: read - To add a label to the pull request → this uses
issues.addLabelson a PR, which is allowed viapull-requests: write(orissues: writeas labels live on the issue/PR). Usingpull-requests: writeis a clear fit for PR-centric operations. - To create a comment on the pull request → also covered by
pull-requests: write.
No other scopes (like contents: write, actions, checks, etc.) are required. The safest minimal fix is to add a permissions block under the label-api-changes job (or at the root; job-level is enough here) with:
permissions:
contents: read
pull-requests: writeThis change should be inserted directly under the job definition (label-api-changes: / name: Label API Changes region) in .github/workflows/api-report-approval.yaml. No imports or additional methods are required, as this is purely YAML configuration for GitHub Actions.
| @@ -11,6 +11,9 @@ | ||
| label-api-changes: | ||
| name: Label API Changes | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
| name: Check API Reports | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Git Checkout | ||
| uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Setup Flutter | ||
| uses: subosito/flutter-action@v2 | ||
| with: | ||
| cache: true | ||
| channel: 'stable' | ||
| flutter-version: '3.32.4' | ||
|
|
||
| - name: Setup aft | ||
| run: flutter pub global activate -spath packages/aft | ||
|
|
||
| - name: Bootstrap | ||
| id: bootstrap | ||
| timeout-minutes: 20 | ||
| run: aft bootstrap --fail-fast --include=aft --verbose | ||
|
|
||
| - name: Install dart_apitool | ||
| run: flutter pub global activate dart_apitool | ||
|
|
||
| - name: Generate API reports | ||
| run: aft generate api-report | ||
|
|
||
| - name: Check for changes in API reports | ||
| id: check-changes | ||
| run: | | ||
| # Debug: Print git status | ||
| echo "=== DEBUG: Git status ===" | ||
| git status --porcelain | ||
|
|
||
| # Check API JSON files for changes | ||
| API_JSON_CHANGES=$(git status --porcelain | grep 'packages/aft/api_reports/.*\.api\.json' || true) | ||
|
|
||
| if [[ -n "$API_JSON_CHANGES" ]]; then | ||
| echo "API report files have changes. Checking if they are actual API changes..." | ||
| git diff --name-only | grep -E 'packages/aft/api_reports/.*\.api\.json' | ||
|
|
||
| ACTUAL_API_CHANGES=false | ||
|
|
||
| # Compare API interfaces using sets instead of text diff | ||
| # We're comparing the PR's committed API reports vs freshly generated ones | ||
| for file in $(git diff --name-only | grep -E 'packages/aft/api_reports/.*\.api\.json'); do | ||
| echo "=== Checking API changes in: $file ===" | ||
|
|
||
| # Get the version from the PR (what developer committed) | ||
| git show HEAD:"$file" | jq -r '.interfaceDeclarations[] | tostring' > /tmp/pr_interfaces.txt 2>/dev/null || touch /tmp/pr_interfaces.txt | ||
|
|
||
| # Get the freshly generated version (what CI just created) | ||
| jq -r '.interfaceDeclarations[] | tostring' "$file" > /tmp/generated_interfaces.txt | ||
|
|
||
| # Compare the interface sets | ||
| if ! cmp -s /tmp/pr_interfaces.txt /tmp/generated_interfaces.txt; then | ||
| echo "=== ACTUAL API CHANGES DETECTED in: $file ===" | ||
| echo "The API report in your PR doesn't match the current code. Please run 'aft generate api-report' and commit the changes." | ||
| echo "Full diff for debugging:" | ||
| git diff "$file" | ||
| ACTUAL_API_CHANGES=true | ||
| else | ||
| echo "API report is up-to-date for: $file" | ||
| fi | ||
|
|
||
| rm -f /tmp/pr_interfaces.txt /tmp/generated_interfaces.txt | ||
| done | ||
|
|
||
| if [[ "$ACTUAL_API_CHANGES" == "true" ]]; then | ||
| echo "ERROR: API report files are not up-to-date. Please run 'aft generate api-report' and commit the changes." | ||
| exit 1 | ||
| else | ||
| echo "All API reports are up-to-date (detected only ordering differences)." | ||
| fi | ||
| else | ||
| echo "API report files are up-to-date." | ||
| fi | ||
|
|
||
| - name: Check if API reports are included in PR | ||
| run: | | ||
| # Get the list of changed files in the PR | ||
| CHANGED_FILES=$(git diff --name-only origin/main...HEAD) | ||
|
|
||
| # Check if any Dart files were modified | ||
| DART_FILES=$(echo "$CHANGED_FILES" | grep -E '\.dart$' || true) | ||
| if [[ -z "$DART_FILES" ]]; then | ||
| echo "No Dart files were modified, skipping API report check." | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Check if API report files are included | ||
| API_REPORTS=$(echo "$CHANGED_FILES" | grep -E 'packages/aft/api_reports/.*\.api\.json' || true) | ||
| if [[ -z "$API_REPORTS" ]]; then | ||
| echo "Error: API report files are not included in this PR." | ||
| echo "Please run 'aft generate api-report' and commit the changes." | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "API report files are included in the PR." |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, fix this by explicitly defining a permissions: block to scope down the GITHUB_TOKEN to only what this workflow needs. Since this job only checks out code and runs local tools and git commands, it needs only read access to repository contents (and no write permissions, and no access to other scopes like issues, pull-requests, etc.).
The best minimal change without altering functionality is to add a permissions: block at the workflow root (top-level, alongside name: and on:) so it applies to all jobs. Given current usage, contents: read is sufficient. No steps in the job push commits, create releases, comment on PRs, or otherwise call GitHub APIs in a way that needs broader scopes.
Concretely:
- Edit
.github/workflows/api-report-check.yaml. - Insert a
permissions:block after thename: API Report Checkline (line 1 in the snippet) and before theon:block. - Set it to:
permissions: contents: read
No additional imports, methods, or definitions are needed since this is purely YAML configuration.
| @@ -1,5 +1,8 @@ | ||
| name: API Report Check | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: |
2 participants
Issue #, if available:
Description of changes:
Introduced the
aft generate api-reportcommand to AFT. This command uses dart_apitool from @bmw-tech to create snapshots of our package's dart level API surfaces, and a change report for these. Developers must now always use this AFT command to generate these files for each package (just running the command as is once here works) on every Pull Request that is expected to merge into main.This PR also includes a workflow for checking that these change reports are actually updated according to the changes on the PR branch, and a workflow for requiring additional approvals when an API change actually occurs (as shown by the lovely bot in this thread).
Overall this change to our development flow will enforce that developers are always aware of API changes to help us get the right eyes on it and to help with versioning.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.