chore: Use GH Checks API for manual e2e tests

[JorTurFer]

Signed-off-by: Jorge Turrado jorge_turrado@hotmail.es

Instead of using labels/emojis for the e2e on PR, this PR updates the workflow to use Github's check API. The main changes are:

• Instead of monitoring labels, a pull_request_target trigger creates an empty check on every new commit in queued status
• The e2e workflow updates the check created by the previous step, setting the status to in-progress when we start to build the image and setting to complated + success/failure at the end of the e2e execution.
• The new label to skip e2e tests is skip-e2e.

You can check how it works just opening a PR to this repository and commenting /run-e2e in the PR (it doesn't check author permissions, only the command)

NOTE: I have noticed that the test suite seems not idempotent, so there is a possibility of not doing anything if we try to execute more than once e2e tests for the same commit

Checklist

• Commits are signed with Developer Certificate of Origin (DCO - learn more)
• Tests have been added
• A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• A PR is opened to update the documentation on (repo) (if applicable)
• Changelog has been updated and is aligned with our changelog requirements

Fixes #2567

[tomkerkhove]

Instead of using labels/emojis for the e2e on PR, this PR updates the workflow to use Github's check API. The main changes are:

• Instead of monitoring labels, a pull_request_target trigger creates an empty check on every new commit in queued status
• The e2e workflow updates the check created by the previous step, setting the status to in-progress when we start to build the image and setting to complated + success/failure at the end of the e2e execution.

How do we get it passing if we don't run the tests? Can we override it with a label?

[tomkerkhove]

Maybe we should have an additional check where our ok-to-merge label wins regardless of the test running?

The workflow would check for changes to labels and do this when it gets an ok-to-merge label and the e2e workflow is allowed to update if tests fail?

- name: Set status success
  uses: LouisBrunner/checks-action@v1.1.1
  with:
    token: ${{ secrets.GITHUB_TOKEN }}
    sha: ${{ needs.triage.outputs.commit_sha }}
    name: e2e tests
    conclusion: skipped by maintainer
    details_url: https://github.com/${{github.repository}}/actions/runs/${{github.run_id}}

[zroubalik]

Maybe we should have an additional check where our ok-to-merge label wins regardless of the test running?

The workflow would check for changes to labels and do this when it gets an ok-to-merge label and the e2e workflow is allowed to update if tests fail?

- name: Set status success
  uses: LouisBrunner/checks-action@v1.1.1
  with:
    token: ${{ secrets.GITHUB_TOKEN }}
    sha: ${{ needs.triage.outputs.commit_sha }}
    name: e2e tests
    conclusion: skipped by maintainer
    details_url: https://github.com/${{github.repository}}/actions/runs/${{github.run_id}}

I like this

[JorTurFer]

WDYT about an extra step in the pull_request_target workflow which just set the CI as passed in case of labeled? Using that, every new commit will be marked as passed.
wrt to the message, I have to dig in it, IDK if it's possible

[tomkerkhove]

WDYT about an extra step in the pull_request_target workflow which just set the CI as passed in case of labeled? Using that, every new commit will be marked as passed.

I'm not sure I would label it with ok-to-merge then given I might want to run the e2e tests later on, based on recent changes - Or is this just me?

[JorTurFer]

it can be, skip-e2e for instance

[JorTurFer]

the label to skip e2e in current commit (and future commits in the PR) is skip-e2e

[zroubalik]

@JorTurFer so we no longer call this label ok-to-merge?

[JorTurFer]

At this point I think that skip-e2e makes more sense, but I'm open to change it.
What do you prefer?

[tomkerkhove]

As discussed previously, is ok-to-merge not better given we could add more criteria to it later on that goes beyond e2e?

For example, if the changelog is not done I'd like to block the PR

[JorTurFer]

As discussed previously, is ok-to-merge not better given we could add more criteria to it later on that goes beyond e2e?

For example, if the changelog is not done I'd like to block the PR

Sorry but I'm not getting your point. Do you want to use this check for more things than e2e tests?
I mean, this check is only for e2e tests (at least right now because the check name is e2e-test), if we want we can add more checks for different things, but I wouldn't mix all in one. That's why IMO makes sense to use skip-e2e if we want to just skip e2e tests requirement.

As you said, ok-to-merge can include other things not related with e2e tests

[tomkerkhove]

I want the current checking on ok-to-merge to be for everything where e2e tests is currently automated but if I see that changelog/docs are not good then I want to remove the label since it's no longer OK to merge imo.

[JorTurFer]

I'd say that it's not possible, for doing complex things we need to keep the checkid stored for updating them properly. Without a real backend, we are a bit limited in what things we can to. For instance, once the check for the commit has finished (it's concluded), we cannot create another check in the same commit with the same name, we have to update it or the change will not be reflected.
let me check if I can get that information on the fly for doing that, but I'm not sure.

For that case, we could just create another different CI/job that checks for another different label like not-ready or similar. It's more simple because that CI will only check this label, and that's all. We can reach the same behavior (I can add it in this PR also). WDYT?

[JorTurFer]

After dozens of tries (trust me xD), I have discovered how to keep the check alive despite the s***y implementation of checks on GH side.

For achieving this, I have to create one workflow just for creating the check (it must be split to keep the check-run visible because on every execution, the check created on-the-fly is lost), and another workflow which monitors labels and modifies the previously created check. I can all another extra step in this latest CI (label triggers) and just fail in case of existing any label like not-ready or whatever (not modifying the e2e test check, failing the CI triggered with labels).

I think that with it, we can achieve both goals, having a semantic label to skip e2e test (skip-e2e) and also a good way to block the PR (failing the CI) in case of exiting a label like not-ready, pending-docs, or whatever.

With this approach, once the gap is fixed, you can just remove the label and then, this check will be green so if others (including e2e test check) are green, the PR is mergeable.

Could this fit with your ideas @kedacore/keda-contributors ?

[zroubalik]

Awesome job on this @JorTurFer !!!

This is huge

[JorTurFer]

let's wait till @tomkerkhove thoughts about #3071 (comment) to implement it in this PR
(I know you cannot wait to try this xD)

[tomkerkhove]

If both of you are OK, then I am as well.

[tomkerkhove]

Thanks a ton!

[JorTurFer]

Lol, the change for blocking the PR with the label not-ready wasn't done... xD
Let me open another PR