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

feat: improve e2e on PR process #3033

Merged
merged 10 commits into from
May 12, 2022
Merged

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented May 10, 2022

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

This PR introduces 2 big changes to improve the proces:

  • All PRs will be blocked till e2e tests pass or till the label ok-to-merge is manually added
  • Every member of the team @kedacore/keda-e2e-test-executors will be able to trigger the e2e tests

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 #3004

@JorTurFer JorTurFer marked this pull request as ready for review May 10, 2022 22:41
@JorTurFer JorTurFer requested a review from a team as a code owner May 10, 2022 22:41
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@JorTurFer JorTurFer changed the title WIP - feat: improve e2e on PR process feat: improve e2e on PR process May 11, 2022
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
@tomkerkhove
Copy link
Member

  • All PRs will be blocked till e2e tests pass or till the label ok-to-merge is manually added

I love this idea! Good thinking!

Some follow-up ideas for this/next PR:

  • Would it make sense to use tests/ok-to-merge or so that we are more flexible for the future?
  • Can we add a similar label for when the tests have failed?
  • We might want to add a new workflow to automatically tag tests/not-run for all new PRs (happy to help here)

Thoughts?

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, but added some suggestions in the overall comments

.github/workflows/pr-e2e.yml Outdated Show resolved Hide resolved
.github/workflows/pr-e2e.yml Outdated Show resolved Hide resolved
.github/workflows/pr-e2e.yml Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Added @v-shenoy to contributors who can run tests

@tomkerkhove
Copy link
Member

  • We might want to add a new workflow to automatically tag tests/not-run for all new PRs (happy to help here)

We can use https://github.com/marketplace/actions/labeler so that we don't have to maintain anything other than config

@tomkerkhove
Copy link
Member

We can use https://github.com/marketplace/actions/verify-pull-request-labels to only allow to merge when the tests are NOT failing. (so ran or not ran)

@JorTurFer
Copy link
Member Author

We can use github.com/marketplace/actions/verify-pull-request-labels to only allow to merge when the tests are NOT failing. (so ran or not ran)

Which is the difference with the current approach?
I mean, the already existing task works quite similar

@JorTurFer
Copy link
Member Author

We can use github.com/marketplace/actions/labeler so that we don't have to maintain anything other than config

I checked this and I'm not sure if it's worth because this labels based on paths (or am I missing something?). Even if we use this task for adding labels, we need a mechanism to remove them in the case of triggering several times the e2e tests

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

Love this, will make contributions easier :)

@tomkerkhove
Copy link
Member

We can use github.com/marketplace/actions/verify-pull-request-labels to only allow to merge when the tests are NOT failing. (so ran or not ran)

Which is the difference with the current approach? I mean, the already existing task works quite similar

The difference is that I'm a moron and looked over it, haha. Sorry about that!

We can use github.com/marketplace/actions/labeler so that we don't have to maintain anything other than config

I checked this and I'm not sure if it's worth because this labels based on paths (or am I missing something?). Even if we use this task for adding labels, we need a mechanism to remove them in the case of triggering several times the e2e tests

This was to cover my suggestion to add tests/not-run label for new PRs

Love this, will make contributions easier :)

We strive to make things super simple, and there's always room for improvement. Happy to see you're liking it.

@JorTurFer
Copy link
Member Author

JorTurFer commented May 11, 2022

This was to cover my suggestion to add tests/not-run label for new PRs

The problem is that once it passes the first time, the label is removed, we still need a mechanism to add the label again if we want to trigger the e2e test again. The workflow in the PR it's idempotent and you can run it several times, blocking the PR on every execution because label is added/removed on every execution

I think that this labeler action doesn't allow adding labels from a workflow with comment trigger because the parameters don't contain any reference to PR info, so I guess that it's implicitly get from the github.context
image

TBH, I have 0 experience with this and I'm could be missing any important point

Jorge Turrado added 2 commits May 12, 2022 18:56
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@JorTurFer JorTurFer enabled auto-merge (squash) May 12, 2022 17:03
@JorTurFer JorTurFer disabled auto-merge May 12, 2022 17:06
@JorTurFer JorTurFer enabled auto-merge (squash) May 12, 2022 17:09
@JorTurFer JorTurFer merged commit d1894fc into kedacore:main May 12, 2022
@JorTurFer JorTurFer deleted the improve-e2e-pr branch May 12, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow PR authors to run E2E tests.
3 participants