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

Add github action to run e2e command "on-demand" #2241

Merged
merged 24 commits into from
Nov 5, 2021

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Nov 1, 2021

Signed-off-by: jorturfer jorge_turrado@hotmail.es

We have been discussing how to run e2e test in PRs before merge them when anyone with writing permissions requests it.
There are several options to achieve this behavior, in this PR I propose to use several actions in a row to allow it.

When someone with writing permissions comment the PR with the message /e2e, automatically this action will be executed reacting the comment with 🚀 to refer that the pipeline is in process, and also with 👍 / 👎 to notify if the e2e tests have passed or not.

Also, the trigger supports setting the e2e test discovery regex using if you want, using this format /e2e REGEX, ex:

  • /run-e2e *.test.ts
  • /run-e2e cron*

Note: To use this feature, we should build and push this image ghcr.io/kedacore/build-tools:main because a few extra packages are needed like up-to-dated git, or hub pr
Note2: Because of this, a PAT with delete permissions in the packages should be added also in order to be able to drop the e2e-test-tag from ghcr. GITHUB_TOKEN is not enough. The expected secret name is GHCR_AUTH_PAT

Achievements:

  • Execute e2e test on demand using a command
  • Allow providing the regex to select a subset of the tests
  • Execute the process to ensure that it works

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

Fixes #2224

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@zroubalik
Copy link
Member

Looking great so far, minor nit, I'd change the trigger command e2e -> /e2e, so we avoid unintentional execution :)

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Makefile Outdated Show resolved Hide resolved
tests/run-all.sh Outdated Show resolved Hide resolved
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member Author

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
…t steps)

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 3, 2021

Another sample is this PR
The first comment (/e2e cron*) passed the tests and has 🚀 👍
The second comment (/e2e) didn't pass the tests and has 🚀 👎

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer marked this pull request as ready for review November 3, 2021 23:46
@JorTurFer JorTurFer requested a review from ahmelsayed as a code owner November 3, 2021 23:46
@JorTurFer JorTurFer changed the title WIP - Add github action to run e2e command "on-demand" Add github action to run e2e command "on-demand" Nov 3, 2021
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking really great!

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Finally caught up on the idea and it's great 👍

I'd suggest renaming the command to /run e2e-tests though to be open and feel more natural, it's just a suggestion.

@tomkerkhove
Copy link
Member

We should also have a follow-up item to refactor our actions and reduce the duplication by using reusable workflows: https://docs.github.com/en/actions/learn-github-actions/reusing-workflows

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 4, 2021

Finally caught up on the idea and it's great 👍

I'd suggest renaming the command to /run e2e-tests though to be open and feel more natural, it's just a suggestion.

Honestly, I'd like to keep the command shorter than possible to reduce the possibility of errors. But maybe we can rename to /test {regex}. WDYT?

  • /test
  • /test cron*

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 4, 2021

I think that now all the goals are achieved:
The PR
The comment (/e2e cron*) passed the tests and has 🚀 👍
The comment (/e2e azure-blob*) didn't pass the tests and has 🚀 👎

The images used during the process are deleted after the e2e tests have finished.
I'm going to update this PR with the latest changes from my working repository and I think that all is done except the setup of the final "triggering comment".

Please, take a look at the PR description, I updated it to add 2 important notes related with the requirements to do this usable

@zroubalik
Copy link
Member

@JorTurFer excellent!

ad Note1: the image will be automatically build once we merge this PR

ad Note2: @tomkerkhove might help?

@zroubalik
Copy link
Member

agree on making the command short, /e2e is fine for me or /run-e2e too.

@tomkerkhove
Copy link
Member

Let's make it very explicit and use dedicated images to fully isolate - Better safe than sorry.

@JorTurFer
Copy link
Member Author

@JorTurFer so it doens't mean that if any tag has > 5k downloads, the whole package (thus all the other tags) is protected?

I don't understand that. I understand that the package itself it's protected but not all the versions. You could delete one version or whole package.
We can do a test easily, you could create a tag manually and try to delete it

@zroubalik
Copy link
Member

Let's make it very explicit and use dedicated images to fully isolate - Better safe than sorry.

💯

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 5, 2021

do you want to use other organization or only other package?
I could do something like this:

# If E2E_IMAGE_TAG is defined, we are on pr e2e test and we have to use the new tag and append -test to the repository
ifeq '${E2E_IMAGE_TAG}' ''
VERSION = main
SUFFIX = ''
endif

ifneq '${E2E_IMAGE_TAG}' ''
VERSION = ${E2E_IMAGE_TAG}
SUFFIX = '-test'
endif

IMAGE_REGISTRY ?= ghcr.io
IMAGE_REPO     ?= kedacore

IMAGE_CONTROLLER = $(IMAGE_REGISTRY)/$(IMAGE_REPO)/keda$(SUFFIX):$(VERSION)
IMAGE_ADAPTER    = $(IMAGE_REGISTRY)/$(IMAGE_REPO)/keda-metrics-apiserver$(SUFFIX):$(VERSION)

This change will append -test to the package name if we set E2E_IMAGE_TAG.

In this second case, we have to create at least 1 tag manual because it's not possible to keep the repository without any version (tag), you will get an error if you try it saying that you should delete the whole package. We can create an empty image manually and push to only to have one tag always.

WDYT?

@zroubalik
Copy link
Member

@JorTurFer yeah, that would be probably the best approach.

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 5, 2021

I'll try it and I hope this afternoon the PR will be ready :)
Testing this changes is a bit tedious because I have to adapt them in other organization (and honestly, I start to having hype about see this running xD)

@zroubalik
Copy link
Member

@JorTurFer no worries, if you change the target images we can merge this PR and do the testing direclty in here.

Jorge Turrado added 3 commits November 5, 2021 12:58
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@JorTurFer
Copy link
Member Author

@zroubalik FYI

Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM

@JorTurFer
Copy link
Member Author

could you create the *-test packages with one dummy tag? Have I got enough permissions to do that?

@zroubalik
Copy link
Member

@tomkerkhove could you please help here, I am afk

@tomkerkhove
Copy link
Member

tomkerkhove commented Nov 5, 2021

could you create the *-test packages with one dummy tag? Have I got enough permissions to do that?

Why is that? Can't you just push them through a test run?

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 5, 2021

In this second case, we have to create at least 1 tag manual because it's not possible to keep the repository without any version (tag), you will get an error if you try it saying that you should delete the whole package. We can create an empty image manually and push to only to have one tag always.

Yes, but we can't delete a tag if it's the only tag that keeps there, so I think that could be better if we create a dummy tag in order to be able to delete others

@tomkerkhove
Copy link
Member

Frankly, if it's a test image we don't have to delete the tags perse which allow us to reduce the scope of the PAT anyway.

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 5, 2021

Frankly, if it's a test image we don't have to delete the tags perse which allow us to reduce the scope of the PAT anyway.

If we don't want to remove the test images, we don't need the PAT, for pushing proposes it's enough the GITHUB_TOKEN available in the runner
It's also an option :)

@tomkerkhove
Copy link
Member

Yes, that was what I was suggesting indeed - This feels better.

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 5, 2021

For me it's totally enough. If it's another package, we could delete them manually from time to time if we want
Do you agree with this @zroubalik (with not delete them during the execution)?

@zroubalik
Copy link
Member

For me it's totally enough. If it's another package, we could delete them manually from time to time if we want Do you agree with this @zroubalik (with not delete them during the execution)?

Ack, It's not my storage 😄

Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@JorTurFer
Copy link
Member Author

I've just deleted the steps of removing both images, I don't have any other change pending

@zroubalik
Copy link
Member

:shipit:

@zroubalik zroubalik merged commit bedf867 into kedacore:main Nov 5, 2021
@zroubalik
Copy link
Member

zroubalik commented Nov 5, 2021

Let's wait until the build-tools image is being built.

This is awesome @JorTurFer!!! I have been hoping to have something like this for more than a year :)

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.

3 participants