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.

Sign up for GitHub

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

Jump to bottom

Bump cilium-cli to v0.8.4 #16799

Merged
merged 2 commits into from Jul 9, 2021
Merged

Bump cilium-cli to v0.8.4 #16799

merged 2 commits into from Jul 9, 2021

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Jul 5, 2021

No description provided.

@tklauser tklauser added dont-merge/preview-only Only for preview or testing, don't merge it. release-note/ci This PR makes changes to the CI. needs-backport/1.10 labels Jul 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.3 Jul 6, 2021
@gandro
Copy link
Member

gandro commented Jul 6, 2021

With v0.8.4 we should be able to remove these FIXME lines and the timeout 5s part here:

cilium/.github/workflows/conformance-multicluster.yaml

Lines 281 to 285 in 394c084

# FIXME: Use of timeout is a workaround for cilium/cilium-cli#384
timeout 5s cilium clustermesh status --context ${{ steps.contexts.outputs.context1 }}
cilium status --context ${{ steps.contexts.outputs.context2 }}
# FIXME: Use of timeout is a workaround for cilium/cilium-cli#384
timeout 5s cilium clustermesh status --context ${{ steps.contexts.outputs.context2 }}

I can do it in a separate PR though if it's already too late.

@tklauser
Copy link
Member Author

tklauser commented Jul 6, 2021

With v0.8.4 we should be able to remove these FIXME lines and the timeout 5s part here:

cilium/.github/workflows/conformance-multicluster.yaml

Lines 281 to 285 in 394c084

# FIXME: Use of timeout is a workaround for cilium/cilium-cli#384
timeout 5s cilium clustermesh status --context ${{ steps.contexts.outputs.context1 }}
cilium status --context ${{ steps.contexts.outputs.context2 }}
# FIXME: Use of timeout is a workaround for cilium/cilium-cli#384
timeout 5s cilium clustermesh status --context ${{ steps.contexts.outputs.context2 }}

I can do it in a separate PR though if it's already too late.

Thanks for pointing this out. I'll add a separate commit removing these FIXMEs as I likely will have to cherry-pick a fix for the multicluster workflow anyway.

@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch from 7a3ada2 to 51d95f0 Compare July 6, 2021 14:10
@tklauser
Copy link
Member Author

tklauser commented Jul 6, 2021

Added a commit to address #16799 (comment) and cherry-picked @nbusseneau's commit from #16787 fixing the multicluster workflow. Hopefully all required actions should pass now.

@tklauser
Copy link
Member Author

tklauser commented Jul 6, 2021

Waiting for #16787 (which should now fix the failing multicluster test) to be reviewed and merged and then will rebase this PR.

@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch from 51d95f0 to 5c7da94 Compare July 8, 2021 12:38
nbusseneau
nbusseneau reviewed Jul 8, 2021
View reviewed changes
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.

Same remark as Sebastian, we need both cilium clustermesh status back in, otherwise LGTM :D

@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch 2 times, most recently from b476b8c to 7f83571 Compare July 8, 2021 14:32
@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch from 7f83571 to c1de163 Compare July 8, 2021 14:56
@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch from c1de163 to 2fbdb61 Compare July 8, 2021 15:06
@tklauser
Copy link
Member Author

tklauser commented Jul 8, 2021

Blocked on #16831

@tklauser
.github/workflows: bump cilium-cli to v0.8.4
b68e480 
Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser
.github/workflows: remove timeout workaround
c3103bb 
This was introduced in commit 2641808 ("ci-multicluster: Fix
post-test information gathering") to work around cilium/cilium-cli#384.
That issue is fixed in the v0.8.4 cilium-cli release, so drop the
workaround.

Suggested-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch from 2fbdb61 to e9767d2 Compare July 9, 2021 11:48
@tklauser
Copy link
Member Author

tklauser commented Jul 9, 2021

Rebased to pick up #16787 and #16831, also addressed Sebastian's remark. Tests are all passing now 🎉

Removing the temporary test commit and moving out of draft.

@tklauser tklauser force-pushed the pr/tklauser/bump-clium-cli-v0.8.4 branch from e9767d2 to c3103bb Compare July 9, 2021 13:05
@tklauser tklauser removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Jul 9, 2021
@tklauser tklauser marked this pull request as ready for review July 9, 2021 13:05
@tklauser tklauser requested review from a team as code owners July 9, 2021 13:05
@tklauser tklauser requested a review from christarazi July 9, 2021 13:05
@tklauser tklauser requested a review from aanm July 9, 2021 13:05
nbusseneau
nbusseneau approved these changes Jul 9, 2021
View reviewed changes
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.

:shipit:

@nbusseneau
Copy link
Member

All reviews are in, PR only touches CI 3.0 workflows, and all of them have been manually tested via pull_request triggers (cf. above). Marking as ready-to-merge.

@nbusseneau nbusseneau added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. area/CI-improvement Topic or proposal to improve the Continuous Integration workflow labels Jul 9, 2021
@joestringer joestringer merged commit cb9ecdc into master Jul 9, 2021
@joestringer joestringer deleted the pr/tklauser/bump-clium-cli-v0.8.4 branch July 9, 2021 20:27
@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
@aanm aanm mentioned this pull request Jul 14, 2021
Merged
@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
@aanm aanm mentioned this pull request Jul 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro left review comments

@christarazi christarazi christarazi approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

@aanm aanm aanm approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tklauser @gandro @nbusseneau @christarazi @aanm @michi-covalent @joestringer