chore: Automatically add labels for all e2e-tests stages to PRs #3048

tomkerkhove · 2022-05-16T06:25:39Z

Automatically add labels for all e2e-tests stages to PRs:

Automatically add 'e2e-tests/not-run' label to new PRs
Automatically add 'e2e-tests/failed-run' when tests have failed for a PR
Automatically add 'e2e-tests/running' when tests are running for a PR
Rename ok-to-merge to e2e-tests/ok-to-merge so that it's clear to end-users what the label stands for.

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)

Signed-off-by: Tom Kerkhove <[email protected]>

Use different approach according to https://docs.github.com/en/actions/using-workflows/using-github-cli-in-workflows Signed-off-by: Tom Kerkhove <[email protected]>

Signed-off-by: Tom Kerkhove <[email protected]>

JorTurFer · 2022-05-16T06:33:39Z

I agree with the renaming of the label, but I'm not sure if adding a second label is worth 🤔
I mean, having two labels we have to manage both during the e2e tests

tomkerkhove · 2022-05-16T06:52:45Z

It allows for easier to see if the tests have ran or not and filtering IMO. Also, end-users know there are e2e tests that can be ran if they want to

Signed-off-by: Tom Kerkhove <[email protected]>

tomkerkhove · 2022-05-16T06:54:07Z

I would even introduce a e2e-tests/failed to indicate that tests failed.

Signed-off-by: Tom Kerkhove <[email protected]>

tomkerkhove · 2022-05-16T07:00:48Z

I went even further @JorTurFer to show all stages :)

Automatically add 'e2e-tests/not-run' label to new PRs
Automatically add 'e2e-tests/failed-run' when tests have failed for a PR
Automatically add 'e2e-tests/running' when tests are running for a PR
Rename ok-to-merge to e2e-tests/ok-to-merge so that it's clear to end-users what the label stands for.

That way, contributors have a better understanding of what is going on in addition to the comment that has the link.

This allows them to have a quick idea where it is, without having to go in details.

zroubalik · 2022-05-16T07:03:35Z

Is there any real benefit behind adding this complexity? Also I think that the original name was chosen because sometimes you'd like to mark the PR as ok even without running the e2e. For example if it is adding some stuff where it doesn't make sense to run e2e.

JorTurFer · 2022-05-16T07:02:45Z

.github/workflows/new-pr.yml

+on:
+  pull_request:
+    types:
+      - opened


Should we do this also in reopen?

I'd keep the current label state as the test might have already ran successfully, no?

JorTurFer · 2022-05-16T07:04:53Z

.github/workflows/pr-e2e.yml

+      - name: Remove '${{ env.E2E_LABELS_NOTRUN}}' label
+        id: remove-not-run-label
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          git config --global --add safe.directory "$GITHUB_WORKSPACE"
+          gh pr edit ${{ needs.triage.outputs.pr_num }} --remove-label '${{ env.E2E_LABELS_NOTRUN}}'
+
+      - name: Remove '${{ env.E2E_LABELS_PASSED}}' label
+        id: remove-pass-label
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          git config --global --add safe.directory "$GITHUB_WORKSPACE"
+          gh pr edit ${{ needs.triage.outputs.pr_num }} --remove-label '${{ env.E2E_LABELS_PASSED}}'
+
+      - name: Remove '${{ env.E2E_LABELS_FAILED }}' label
+        id: remove-failed-label
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: |
          git config --global --add safe.directory "$GITHUB_WORKSPACE"
-          gh pr edit ${{ needs.triage.outputs.pr_num }} --remove-label '${{ env.E2E_LABEL}}'
+          gh pr edit ${{ needs.triage.outputs.pr_num }} --remove-label '${{ env.E2E_LABELS_FAILED }}'
+
+      - name: Add '${{ env.E2E_LABELS_RUNNING }}' label
+        id: add-running-label
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          git config --global --add safe.directory "$GITHUB_WORKSPACE"
+          gh pr edit ${{ needs.triage.outputs.pr_num }} --add-label '${{ env.E2E_LABELS_RUNNING }}'


Should we merge these 3 steps into a single one? We can reduce a lot of lines just appending the gh lines to one of them

Good idea, but will see if we want this based on the main conversation

JorTurFer · 2022-05-16T07:06:17Z

.github/workflows/pr-e2e.yml

        id: remove-label
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: |
          git config --global --add safe.directory "$GITHUB_WORKSPACE"
-          gh pr edit ${{ needs.triage.outputs.pr_num }} --remove-label '${{ env.E2E_LABEL}}'
+          gh pr edit ${{ needs.triage.outputs.pr_num }} --remove-label '${{ env.E2E_LABELS_PASSED}}'


Should we add here other labels like not-run?

Good idea, but will see if we want this based on the main conversation

tomkerkhove · 2022-05-16T07:11:03Z

Is there any real benefit behind adding this complexity?

Yes, #3048 (comment) & #3048 (comment) - It's basically to make it more contributor-friendly as not everyone knows how the whole e2e tests work.

Also, if you are checking the PR list it doesn't give you an idea how they are doing in terms of tests.

But if I'm the only one then I'm happy to drop this.

Also I think that the original name was chosen because sometimes you'd like to mark the PR as ok even without running the e2e. For example if it is adding some stuff where it doesn't make sense to run e2e.

That's a fair ask - Happy to revert

JorTurFer · 2022-05-16T08:37:26Z

I have to recognize that seeing how it looks in k8s repo I get better your point, but I still have doubts about if this is overkill for us.
I mean, the amount of contributors we have right now is small, but it's true this could be a reason for contributing more than once... My only concert is how much complex the workflows will turn into if we have to manage 4 different labels right now. When we move the e2e test to Golang, probably this will be easier because it could be managed by code (I hope) instead of by actions in the workflow

tomkerkhove · 2022-05-16T09:22:51Z

I have to recognize that seeing how it looks in k8s repo I get better your point, but I still have doubts about if this is overkill for us. I mean, the amount of contributors we have right now is small, but it's true this could be a reason for contributing more than once...

This is indeed to provide a more robust approach as we've seen growth in PRs and contributors lately.

My only concert is how much complex the workflows will turn into if we have to manage 4 different labels right now.

I think this PR is doing an OK job in managing them, no? I think it's fairly simple to do this at the moment.

When we move the e2e test to Golang, probably this will be easier because it could be managed by code (I hope) instead of by actions in the workflow

Why would this be different if we have our tests in Go vs JS? The PR experience is still the same, or am I missing something?

tomkerkhove · 2022-05-31T07:34:01Z

#3071 is a lot better, abandon 🚢

tomkerkhove added 3 commits May 16, 2022 08:23

chore: Automatically add 'e2e-tests/not-run' label to new PRs

bcb5f34

Signed-off-by: Tom Kerkhove <[email protected]>

chore: Use label prefix

a46423e

Signed-off-by: Tom Kerkhove <[email protected]>

chore: Use label prefix

bb52a9c

Signed-off-by: Tom Kerkhove <[email protected]>

tomkerkhove requested a review from a team as a code owner May 16, 2022 06:25

tomkerkhove requested a review from JorTurFer May 16, 2022 06:25

tomkerkhove added 2 commits May 16, 2022 08:28

Use different approach

1aa94ec

Use different approach according to https://docs.github.com/en/actions/using-workflows/using-github-cli-in-workflows Signed-off-by: Tom Kerkhove <[email protected]>

fix: Use new env var

fe060ff

Signed-off-by: Tom Kerkhove <[email protected]>

fix: Remove spaces

cc28bd1

Signed-off-by: Tom Kerkhove <[email protected]>

tomkerkhove added 2 commits May 16, 2022 08:56

chore: Remove not run label once tests start

d3df360

Signed-off-by: Tom Kerkhove <[email protected]>

Add e2e-tests/running & e2e-tests/failed

34e9e87

Signed-off-by: Tom Kerkhove <[email protected]>

tomkerkhove changed the title ~~chore: Automatically add 'e2e-tests/not-run' label to new PRs~~ chore: Automatically add labels for all e2e-tests stages to PRs May 16, 2022

JorTurFer reviewed May 16, 2022

View reviewed changes

tomkerkhove closed this May 31, 2022

tomkerkhove deleted the tomkerkhove-patch-1 branch May 31, 2022 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Automatically add labels for all e2e-tests stages to PRs #3048

chore: Automatically add labels for all e2e-tests stages to PRs #3048

tomkerkhove commented May 16, 2022 •

edited

Loading

JorTurFer commented May 16, 2022 •

edited

Loading

tomkerkhove commented May 16, 2022

tomkerkhove commented May 16, 2022

tomkerkhove commented May 16, 2022 •

edited

Loading

zroubalik commented May 16, 2022 •

edited

Loading

JorTurFer May 16, 2022

tomkerkhove May 16, 2022

JorTurFer May 16, 2022

tomkerkhove May 16, 2022 •

edited

Loading

JorTurFer May 16, 2022

tomkerkhove May 16, 2022

tomkerkhove commented May 16, 2022

JorTurFer commented May 16, 2022

tomkerkhove commented May 16, 2022

tomkerkhove commented May 31, 2022

chore: Automatically add labels for all e2e-tests stages to PRs #3048

chore: Automatically add labels for all e2e-tests stages to PRs #3048

Conversation

tomkerkhove commented May 16, 2022 • edited Loading

Checklist

JorTurFer commented May 16, 2022 • edited Loading

tomkerkhove commented May 16, 2022

tomkerkhove commented May 16, 2022

tomkerkhove commented May 16, 2022 • edited Loading

zroubalik commented May 16, 2022 • edited Loading

JorTurFer May 16, 2022

Choose a reason for hiding this comment

tomkerkhove May 16, 2022

Choose a reason for hiding this comment

JorTurFer May 16, 2022

Choose a reason for hiding this comment

tomkerkhove May 16, 2022 • edited Loading

Choose a reason for hiding this comment

JorTurFer May 16, 2022

Choose a reason for hiding this comment

tomkerkhove May 16, 2022

Choose a reason for hiding this comment

tomkerkhove commented May 16, 2022

JorTurFer commented May 16, 2022

tomkerkhove commented May 16, 2022

tomkerkhove commented May 31, 2022

tomkerkhove commented May 16, 2022 •

edited

Loading

JorTurFer commented May 16, 2022 •

edited

Loading

tomkerkhove commented May 16, 2022 •

edited

Loading

zroubalik commented May 16, 2022 •

edited

Loading

tomkerkhove May 16, 2022 •

edited

Loading