test(e2e): improving the E2E flow to make E2E references not get removed on approval

[charliedmcb]

Fixes #

Description
This is a larger update with the intended goal of a quality of life improvement on triggering E2E tests for reviews.

Currently:
Each review will trigger the E2EMatrixTrigger pipeline, but only run the E2E test suites if the approval comment is prefixed with /test.

Note: the issue with this is that if the PR has already had a set of e2e tests triggered via a review comment, and another review is added the e2e test suites will be unlinked.

Updating the flow as follows:
Adopting aws' pattern of having an ApprovalComment workflow that gets triggered on the PR reviews. This does the filtering on the prefix to see if we are supposed to kick off an e2e test suite and skips if not.

From there the E2EMatrixTrigger pipeline gets a cascading trigger when the ApprovalComment workflow finishes, but only continues itself if the ApprovalComment workflow was successful.

Note: within the E2EMatrixTrigger workflow it don't have access to the initial github variables from ApprovalComment, so we stash the git_ref in an artifact we upload and download which we need for linking the status of the e2e tests back to the PR.

Additionally, within the e2e workflow we now send back a status ourselves to the PR at the start signaling that we are In progress, and then a completed result of success/failure.

The fact that we've unlinked this so the status of the e2e tests is sent by us in this indirect way means that the only e2e workflow that is automatically updated/reset on PR reviews is the ApprovalComment. The e2e runs are only updated if the review is requesting another e2e run.

How was this change tested?
Tested on my forked branch here:
charliedmcb#2

Note: that this was testing with the trigger being /e2e, where for checking it back into the main Azure/karpenter repo I'm using /test as the trigger again as that is what folks are currently familiar with.
*

Does this change impact docs?

• Yes, PR includes docs updates
• Yes, issue opened: #
• No

Release Note

NONE

[rakechill]

Note: the issue with this is that if the PR has already had a set of e2e tests triggered via a review comment, and another review is added the e2e test suites will be unlinked.

What does it mean for the e2e test suites to be "unlinked"?

[rakechill]

Overall lgtm. Have two questions:

• How will this look on a PR? Is it like a separate comment is added in the PR with the commit hash, status, and target_url. Or does it like add the status right next the greyed out commit. Or maybe does it just show up in the Commits tab?
• ApprovalComment doesn't make sense to me. Maybe I'm misunderstanding, but I would expect it to run before an approval is received rather than after, because I would want that information to help me to decide whether or not to approve. Maybe ReviewComment instead iiuc.

[charliedmcb]

Note: the issue with this is that if the PR has already had a set of e2e tests triggered via a review comment, and another review is added the e2e test suites will be unlinked.

What does it mean for the e2e test suites to be "unlinked"?

The run would get removed from the checks section of the PR:

[charliedmcb]

• How will this look on a PR? Is it like a separate comment is added in the PR with the commit hash, status, and target_url. Or does it like add the status right next the greyed out commit. Or maybe does it just show up in the Commits tab?

It will have the e2e suites show up on the Checks section like normal. There will also be an ApprovalComment workflow run that will show up there either skipped if the most recent review was not requesting the e2e tests to be run, or succeeded if it was.

• ApprovalComment doesn't make sense to me. Maybe I'm misunderstanding, but I would expect it to run before an approval is received rather than after, because I would want that information to help me to decide whether or not to approve. Maybe ReviewComment instead iiuc.

That's a fair question on naming. This naming comes from aws. I'm going to leave it for now, but could consider changing if folks find it to be confusing.