Consolidate backend checks

[ryan-mcneil]

Summary

• Labels are not properly being applied. Sometimes labels are seen being removed then immediately re-applied in the PR history, and sometimes the PR will show both test-failure AND test-passing.
  • Reason: test-failure and test-passing are both appearing because failure() returns IF a failure happened, and success() triggers upon a successful finish, so if a spec fails, retries, and succeeds BOTH of those methods return true. There are some issues with race conditions as well. When a label is added in the CodeChecks workflow, the other checks that are triggered by the labeled or unlabeled event run immediately, so those may add back a label unintentionally (several workflows add and remove the same labels`
  • Solution: Have a single job to add labels, that gets triggered by the CodeChecks workflow finishing (with a single success/failure output) or by other events: Backend PR Labeler
• Require backend-review-group approval / check-approval-requirements requiring a manual re-run and sometimes showing up twice: (pull_request) vs (pull_request_review).
  • Reason: The issue is that the different event types (pull_request and pull_request_review) run as two separate checks. If a commit is pushed (pull_request event) and doesn't have an approval (naturally), it'll fail. Upon receiving a review, the check will run (pull_request_review event), and potentially pass, but that doesn't trigger the existing, failed pull_request event to re-run. This is why we'll see one failed and one passing, and the failed one needs to be re-run
  • Solution: Separate responsibilities - Have a single check for labeling that gets run on various events, but would never fail/block the PR AND a secondary, separate check that follows the other workflows, and would make a determination purely off of labels. It would always get re-run, and wouldn't require a manual retry
• Exempt teams (Identity, Mobile, and Lighthouse) are still required to get backend approval (they shouldn't need it)
  • Reason: Two possible things - race conditions around the require-backend-review label (like above), potential boolean syntax (should be "true" not true),
  • Solution: More explicit comparisons, and the secondary check would immediately pass if PR didn't require backend

NOTE: All additions to backend-pr-labeler and backend-pr-approver were part of this in iteration. I had to merge small PRs to master to test the workflow_run trigger.

Related issue(s)

• [VETS-API] Finish Backend CI Work va.gov-team#96587

[ryan-mcneil]

notice a quiet failure caused by the workflow attempting to remove a label that wasn't there.

[rmtolmach]

Is this true? In CODEOWNERS, files either have backend-review-group listed, OR they have a lighthouse team, octo-identity, or mobile? 🤔 no... I just found this line: spec/lib/token_validation @department-of-veterans-affairs/lighthouse-dash @department-of-veterans-affairs/lighthouse-banana-peels @department-of-veterans-affairs/va-api-engineers @department-of-veterans-affairs/backend-review-group
With the logic here, a PR that changes files in this dir would hit the else block.

[rmtolmach]

Maybe the solution is to remove backend-review-group from those lines in codeowners 🤔

[ryan-mcneil]

I think this would be an example of a CODEOWNERS issue. If they own it, our team doesn't need to be on it. If we should be involved in the review, then it'll work as expected. I think these cases will iron themselves out. I can't think of a good reason for why both would be needed yet we shouldn't be a required approver.