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

[vets-api] Fix backend CI labeling bugs #19659

Merged
merged 14 commits into from
Dec 2, 2024
Merged

Conversation

ryan-mcneil
Copy link
Contributor

@ryan-mcneil ryan-mcneil commented Nov 29, 2024

Summary

  • General fixes:
    • Rename a file and some jobs/steps to more accurately represent what they're doing.
    • fix test-passing/test-failure conflict by evaluating overall result, instead of "existence" of a failure (while the job could still pass on retry
    • Remove unnecessary features (decided upon in CoP channel) such as omit-backend-approval label and approval comment
    • The majority of the workflow triggered by the workflow_run (after Code Checks, etc) was not running, because the workflow didn't have access to the PR (no github.event.pull_request). So I added a step (get_pr_data) to pull the PR number from the workflow_run and retrieve the PR (and set the required vars as outputs)
    • Once unnecessary features were removed, the require_backend_label workflow could be easily added to ready_for_review to remove chance of race condition
    • removed unnecessary checkout actions

Related issue(s)

Testing

  • Failed test adds test-failure label
  • Passing test adds test-passing label
  • Test failure/passing labels not unnecessarily removed then immediately re-added
  • Test failure/passing labels don't appear at same time
  • lint-failure added/removed properly
  • Code owners labels added/removed properly
  • [ ]

Copy link

github-actions bot commented Nov 29, 2024

1 Warning
⚠️ This PR changes 388 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding
200. Expect some delays getting reviews.

File Summary

Files

  • .github/workflows/audit_service_tags.yml (+0/-7)

  • .github/workflows/backend-pr-labeler.yml (+1/-0)

  • .github/workflows/check_codeowners.yml (+0/-14)

  • .github/workflows/code_checks.yml (+33/-72)

  • .github/workflows/ready_for_review.yml (+130/-46)

  • .github/workflows/require_backend_label.yml (+0/-47)

  • .github/workflows/{be_review_prs.yml => require_be_approval.yml} (+8/-30)

    Note: We exclude files matching the following when considering PR size:

    *.csv, *.json, *.tsv, *.txt, *.md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru, *.pdf
    

Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

@va-vfs-bot va-vfs-bot temporarily deployed to rm-fix-be-labeling/main/main November 29, 2024 19:10 Inactive
@@ -21,14 +21,6 @@ jobs:
steps:
- uses: actions/checkout@v4

- name: Remove Review label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled in ready-for-review

@va-vfs-bot va-vfs-bot temporarily deployed to rm-fix-be-labeling/main/main November 29, 2024 19:31 Inactive
@@ -20,14 +20,6 @@ jobs:
with:
fetch-depth: 2

- name: Remove Review label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled in ready-for-review

Comment on lines +118 to +126
- name: Set Test Status
id: test-status
if: always()
run: |
if [ "${{ job.status }}" = "success" ]; then
echo "status=success" >> $GITHUB_OUTPUT
else
echo "status=failure" >> $GITHUB_OUTPUT
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By capturing the result, we can make sure only one of the two labels gets set at a time

types: [completed]
jobs:
ready_for_review:
get-pr-data:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

workflow_run didn't have access to the PR, so this job retrieves it properly

exit 1
fi

handle-draft-state:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to simplify other jobs, I handle the removal of the "ready" tag on drafts here

Comment on lines +77 to +80
env:
pr_number: ${{ needs.get-pr-data.outputs.pr_number }}
pr_labels: ${{ needs.get-pr-data.outputs.pr_labels }}
pr_requested_teams: ${{ needs.get-pr-data.outputs.pr_requested_teams }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set the outputs of get-pr-data job as env vars to clean up logic

@va-vfs-bot va-vfs-bot temporarily deployed to rm-fix-be-labeling/main/main November 29, 2024 19:40 Inactive
@rmtolmach
Copy link
Contributor

rmtolmach commented Nov 29, 2024

Your tests were failing initially (they hit the timeout bug), so I re-ran them and now they're passing. But I just realized 😕 I don't think the test-failure label got added when they failed initially. Makes me think: what are all the ways we can test this? Some ideas:

  • Introduce a test failure - add label
  • Introduce a linting failure - add label
  • Introduce a codeowners problem - add label
  • PR in draft - remove ready-for-backend-review label
  • get a non backend-review-group review
  • Approval by be-review-group makes Check Approval Requirements pass

rmtolmach
rmtolmach previously approved these changes Nov 29, 2024
Copy link
Contributor

@rmtolmach rmtolmach left a comment

Choose a reason for hiding this comment

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

approving so we can see if this makes the automation happy. I think some more testing needs to be done, though. See comment above ☝️

@ryan-mcneil ryan-mcneil requested a review from a team as a code owner November 29, 2024 22:22
@va-vfs-bot va-vfs-bot temporarily deployed to rm-fix-be-labeling/main/main November 29, 2024 22:24 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to rm-fix-be-labeling/main/main November 29, 2024 22:56 Inactive
Comment on lines 8 to 9
check-approval-requirements:
name: Check Approval Requirements
Copy link
Contributor

@rmtolmach rmtolmach Dec 2, 2024

Choose a reason for hiding this comment

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

small thing, but what do you think of changing the name of one of these to "require backend review" or something. I've seen people get confused about why this check hangs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

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'll just use require-backend-approval to match the label/job name

@va-vfs-bot va-vfs-bot temporarily deployed to rm-fix-be-labeling/main/main December 2, 2024 17:34 Inactive
@ryan-mcneil ryan-mcneil merged commit af54f53 into master Dec 2, 2024
25 checks passed
@ryan-mcneil ryan-mcneil deleted the rm-fix-be-labeling branch December 2, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants