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

Fixes cilium clustermesh disconnect #2185

Merged
merged 1 commit into from Feb 1, 2024
Merged

Fixes cilium clustermesh disconnect #2185

merged 1 commit into from Feb 1, 2024

Conversation

littlejo
Copy link
Contributor

cilium clustermesh disconnect never work with helm version:
remote cluster has cluster ID %d out of acceptable range (1-0)

So i fix it.

@littlejo littlejo requested a review from a team as a code owner December 14, 2023 11:45
thorn3r
thorn3r requested changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

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

Thanks for the submission @littlejo! Left a suggestion here on a simpler approach to fixing this bug

clustermesh/clustermesh.go Show resolved Hide resolved
@littlejo littlejo requested a review from thorn3r January 4, 2024 08:11
thorn3r
thorn3r approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

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

Looks good! Sorry for the delay, I was away for the holidays.

@littlejo
Copy link
Contributor Author

@thorn3r Do you know why some checks failed?

@thorn3r
Copy link
Contributor

thorn3r commented Jan 10, 2024

@littlejo Looks like the perf tests were moved to their own subcommand in #2168. The GH workflows run on main which have those changes, but your branch doesn't. If you rebase your PR it should fix these failures

@kariya-mitsuru
Copy link

I am eagerly awaiting this fix to be merged, what are the blocking factors?

@littlejo
Copy link
Contributor Author

@thorn3r do you have news about this PR? Thank you.

@thorn3r
Copy link
Contributor

thorn3r commented Jan 30, 2024

Hey @littlejo it looks like your failure is related to this issue, which was fixed here.

Sorry to ask for this again, but can you rebase once more? The upload-artifact action we're using in that workflow was updated to v4.0.0, which included breaking changes. It's now on v4.3.0, which I believe includes resolution for this - but we've also given the artifacts unique names as per the PR I linked above.

@littlejo
fix: Remove check function validateInfoForConnect for DisconnectWithH…
8a8428a 
…elm function

Signed-off-by: Joseph Ligier <joseph.ligier@gmail.com>
@thorn3r thorn3r mentioned this pull request Jan 31, 2024
Open
@thorn3r thorn3r added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 1, 2024
@michi-covalent michi-covalent merged commit 0b031ca into cilium:main Feb 1, 2024
13 checks passed
@michi-covalent
Copy link
Contributor

this will be included in the upcoming v0.15.22 release 🚀

@BrewTestBot BrewTestBot mentioned this pull request Feb 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thorn3r thorn3r thorn3r approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@littlejo @thorn3r @kariya-mitsuru @michi-covalent