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

workflows: fix L4LB test missing PR reporting on issue_comment #16830

Merged
merged 1 commit into from Jul 8, 2021

Conversation

nbusseneau
Copy link
Member

@nbusseneau nbusseneau commented Jul 8, 2021

Unlike pull_request-triggered runs, issue_comment-triggered runs cannot automatically report back to the PR they were triggered from.

Adding manual reporting as per the other workflows.

@nbusseneau nbusseneau added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. labels Jul 8, 2021
@nbusseneau nbusseneau requested review from a team as code owners July 8, 2021 15:24
@nbusseneau nbusseneau requested review from tklauser and aanm July 8, 2021 15:24
@maintainer-s-little-helper maintainer-s-little-helper bot assigned tklauser and aanm and unassigned aanm Jul 8, 2021
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

🚀

Unlike `pull_request`-triggered runs, `issue_comment`-triggered runs
cannot automatically report back to the PR they were triggered from.

Adding manual reporting as per the other workflows.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
@nbusseneau
Copy link
Member Author

L4LB test failed: https://github.com/cilium/cilium/actions/runs/1012162379
But this is the expected unrelated failure that's been happening in all L4LB runs:

+ kubectl -n kube-system wait --for=condition=Ready pod cilium-2kpf2 --timeout=5m
error: timed out waiting for the condition on pods/cilium-2kpf2

The manual PR check did get added properly to the PR:
image
So I would say the changes are validated.

Removing temp commit.

@nbusseneau nbusseneau force-pushed the pr/workflow-l4lb-missing-check branch from e492016 to c580397 Compare July 8, 2021 16:19
@aditighag
Copy link
Member

aditighag commented Jul 8, 2021

Btw I see this note in the workflow yaml [1] -

### FOR TESTING PURPOSES
  # This workflow runs in the context of `master`, and ignores changes to
  # workflow files in PRs. For testing changes to this workflow from a PR:
  # - Make sure the PR uses a branch from the base repository (requires write
  #   privileges). It will not work with a branch from a fork (missing secrets).
  # - Uncomment the `pull_request` event below, commit separately with a `DO
  #   NOT MERGE` message, and push to the PR. As long as the commit is present,
  #   any push to the PR will trigger this workflow.
  # - Don't forget to remove the `DO NOT MERGE` commit once satisfied. The run
  #   will disappear from the PR checks: please provide a direct link to the
  #   successful workflow run (can be found from Actions tab) in a comment.
  # 
  # pull_request: {}
  ###

Edit : Nvm, saw this comment - #16830 (comment).

[1] https://github.com/cilium/cilium/blob/master/.github/workflows/tests-l4lb.yaml

@aditighag
Copy link
Member

@nbusseneau @brb Does this GH action need to be run as a cron job if it's run on ever PR?

@nbusseneau
Copy link
Member Author

@nbusseneau @brb Does this GH action need to be run as a cron job if it's run on ever PR?

It's there for testing master every 6 hours so that we can detect when merging multiple concurrent PRs might be breaking things :)

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 8, 2021
@aditighag aditighag merged commit d2da30a into master Jul 8, 2021
@aditighag aditighag deleted the pr/workflow-l4lb-missing-check branch July 8, 2021 20:38
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.3 Jul 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.3 Jul 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.3 Jul 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Needs backport from master in 1.10.3 Jul 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.3 Jul 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.3 Jul 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.3 Jul 15, 2021
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
No open projects
1.10.3
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

4 participants