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

github: Misc improvements for the L4LB test suite #17005

Merged
merged 2 commits into from Aug 4, 2021
Merged

Conversation

brb
Copy link
Member

@brb brb commented Jul 28, 2021

See commit msgs.

Fix #16832
Fix #16870
Fix #17002

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 28, 2021
@brb brb force-pushed the pr/brb/ci-l4lb-misc branch 2 times, most recently from a7e0e77 to a710680 Compare July 28, 2021 11:00
@nbusseneau nbusseneau self-requested a review July 28, 2021 11:48
@brb brb force-pushed the pr/brb/ci-l4lb-misc branch 4 times, most recently from 1f8baa4 to be5122f Compare July 28, 2021 13:45
@brb brb changed the title test: L4LB GH action improvements github: Improvements for L4LB test suite Jul 28, 2021
@brb brb added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow needs-backport/1.10 labels Jul 28, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.4 Jul 28, 2021
@brb brb added the release-note/ci This PR makes changes to the CI. label Jul 28, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 28, 2021
@brb brb changed the title github: Improvements for L4LB test suite github: Misc improvements for the L4LB test suite Jul 28, 2021
@brb brb marked this pull request as ready for review July 28, 2021 13:47
@brb brb requested review from a team as code owners July 28, 2021 13:47
@brb brb requested review from a team and jibi July 28, 2021 13:47
@gandro
Copy link
Member

gandro commented Jul 29, 2021

Note: This has a needs-backport/1.10 label, but the corresponding 1.10 workflow is actually now present in master:
https://github.com/cilium/cilium/blob/master/.github/workflows/tests-l4lb-v1.10.yaml

@brb
Copy link
Member Author

brb commented Jul 30, 2021

This has a needs-backport/1.10 label, but the corresponding 1.10 workflow is actually now present in master:

@gandro Does this mean that for backports we will run the workflow from the main branch but not from a v1.10 backport branch?

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

Does this mean that for backports we will run the workflow from the main branch but not from a v1.10 backport branch?

Kinda both: the workflows for issue_comment triggers must be present on the default branch (so yes, we can remove the backport label), however in the workflow itself the Set up job variables step is responsible for fetching the actual PR where the comment was posted. The SHA of this PR is then used for retrieving the right Cilium image to run in the workflow, so it does run in the context of the backport branch :)

 

LGTM, some comments below! I am not familiar with bash traps in the context of a GitHub workflow, did you try to manually fail the script while testing with the pull_request trigger, to see if it hit the trap as expected?

.github/workflows/tests-l4lb-v1.10.yaml Show resolved Hide resolved
.github/workflows/tests-l4lb-v1.10.yaml Show resolved Hide resolved
.github/workflows/tests-l4lb.yaml Show resolved Hide resolved
.github/workflows/tests-l4lb-v1.10.yaml Outdated Show resolved Hide resolved
.github/workflows/tests-l4lb.yaml Outdated Show resolved Hide resolved
.github/workflows/tests-l4lb-v1.10.yaml Outdated Show resolved Hide resolved
.github/workflows/tests-l4lb.yaml Outdated Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.10.4 Aug 3, 2021
Don't trigger the job if a PR changes unrelated to the standalone L4LB
files.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Aug 3, 2021

I am not familiar with bash traps in the context of a GitHub workflow, did you try to manually fail the script while testing with the pull_request trigger, to see if it hit the trap as expected?

@nbusseneau The trap will run in the context of the VM instance, so all fine here.

@brb brb requested a review from nbusseneau August 3, 2021 11:02
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

LGTM once we fix the following 👍🏻

.github/workflows/tests-l4lb-v1.10.yaml Outdated Show resolved Hide resolved
.github/workflows/tests-l4lb.yaml Outdated Show resolved Hide resolved
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 4, 2021
@gandro gandro merged commit 95cba0a into master Aug 4, 2021
@gandro gandro deleted the pr/brb/ci-l4lb-misc branch August 4, 2021 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
5 participants