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

Remove e2e tests not required label #11183

Merged
merged 13 commits into from
Sep 18, 2024
Merged

Conversation

agnesgaroux
Copy link
Contributor

@agnesgaroux agnesgaroux commented Sep 17, 2024

What does this change?

Part of wellcomecollection/platform#5740
We're setting up alternatives to weco-bot everywhere it's used because it doesn't conform the the SSO standard we're set. Weco-bot is a "fake" team member that has its own credentials, used to perform automated tasks.

Before this change, one of these tasks was to call Github API from a Buildkite pipeline run, in order to add the "e2e not required" label to the PR. This label was only used to "remember" on later commits if the developer had asked that e2e tests never be run on the PR.

In the absence of this "e2e not required" label, Buildkite will ask if the dev wants to run the e2e tests or not every time a commit is built.

As agreed with the experience team, we're removing the option to add the "e2e tests not required" label on PRs from Buildkite.
After this change, every time a commit is pushed to a branch and triggers a build & test in Buildkite, the dev will need to select whether or not they want e2e tests run in order to complete the pipeline.

How to test

Checkout a new branch, make any trivial change and push to the remote. In https://buildkite.com/wellcomecollection/wc-dot-org-build-plus-test you should see you commit building, with a step asking you whether you want e2e tests or not.
Select Yes and see that the tests run and the pipeline completes.
Push another commit to the branch, select No and see that the pipeline completes without running the e2e tests

How can we measure success?

Weco-bot is not involved in running/managing e2e tests on PR branches

Have we considered potential risks?

The dev still has to make the deliberate decision to NOT run the e2e tests for the pipeline to complete

@agnesgaroux agnesgaroux requested a review from a team September 17, 2024 12:36
Copy link
Contributor

@rcantin-w rcantin-w left a comment

Choose a reason for hiding this comment

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

I'd love to understand a bit more about where the yes/no options used to be because them being added suprises me a little! Can you run me through it?

And if we could add a "why we're doing this" in the description I reckon it'd be useful for legacy's sake 🙏

@agnesgaroux
Copy link
Contributor Author

agnesgaroux commented Sep 17, 2024

It used to be:

  • yes: run e2e test now
  • maybe later: no e2e tests, complete build but ask me again on later commits
  • no: no e2e tests, complete build + add "e2e not required" label to the PR so I'm not asked again on later commits

Now it's:

  • yes: run e2e test now
  • maybe later: no e2e tests, complete build but ask me again on later commits

@agnesgaroux agnesgaroux marked this pull request as draft September 17, 2024 15:38
@agnesgaroux agnesgaroux marked this pull request as ready for review September 17, 2024 15:38
@rcantin-w rcantin-w mentioned this pull request Sep 17, 2024
Copy link
Contributor

@rcantin-w rcantin-w left a comment

Choose a reason for hiding this comment

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

I love the documentation update it's really good.

I ran it in this branch here https://buildkite.com/wellcomecollection/wc-dot-org-build-plus-test/builds/14794#_ and it works nicely 🙌

Only question is that the "Add steps for e2e tests (if necessary)" step didn't use to wait until all the steps above were finished (I'm pretty sure anyway but might be wrong) and now it does, which might slow down things a minute or two. Which is pretty minimal and might've been a one-off, so very much not a blocker.

if [[ "$BUILDKITE_PULL_REQUEST" != "false" ]]
then
buildkite-agent pipeline upload << EOF
- wait
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rcantin-w I carried over this wait (and the one below on line 75) into the main pipeline.yml so the behaviour should be the same as before. I think we actually have to wait until everything is built before deploying into the e2e env

@agnesgaroux agnesgaroux merged commit b4e0a2d into main Sep 18, 2024
7 checks passed
@agnesgaroux agnesgaroux deleted the remove-e2e-tests-not-required-label branch September 18, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants