-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix cli disconnect error message #38545
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
Fix cli disconnect error message #38545
Conversation
Commit 8a1d1ab does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
8a1d1ab
to
93dcce6
Compare
Commit 93dcce6 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
93dcce6
to
716ebf6
Compare
Commit 93dcce6 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
716ebf6
to
032b059
Compare
Commit 032b059 does not match "(?m)^Signed-off-by:". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
032b059
to
f9f0894
Compare
f9f0894
to
b19cf34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
Please fix the issues reported by CI (you need to shorten the commit subject, and change/drop the extra assertion)
b19cf34
to
5dfc822
Compare
@samsonkolge Could you please squash the two commits together? I'll re-run CI afterwards. |
Add a clear error message when the clustermesh disconnect command is executed without a destination context, improving user experience by explaining what is missing. Fixes: cilium#36994 Signed-off-by: Samson S. Kolge <eglok1980@gmail.com>
5dfc822
to
0487b72
Compare
@giorio94 Done. |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Description
This PR fixes issue #36994 where the
cilium clustermesh disconnect
command doesn't provide any feedback when run without a destination context.Problem
When running
cilium clustermesh disconnect
without specifying a destination context via the--destination-context
flag, the command silently executes but doesn't display any message or error to the user. This creates a confusing user experience as the user doesn't know what happened.Current behavior:
Solution
This PR adds a check at the beginning of the
DisconnectWithHelm
function to verify if a destination context was provided. If not, it returns a clear error message explaining that a destination context is required.New behavior:
This makes the CLI behavior consistent with the case where an invalid destination context is provided, which already shows an error message.
Testing Information
This PR includes a unit test to verify the error behavior:
TestClusterMeshDisconnectWithoutDestination
incilium-cli/cli/clustermesh_test.go
directly tests the DisconnectWithHelm function and verifies that it returns the correct error message when no destination context is provided.This test ensures that the error message is displayed correctly to users when they attempt to use the
clustermesh disconnect
command without specifying a destination context.To run the test
The test validates our fix by confirming the error message is exactly:
"no destination context specified, use --destination-context to specify which cluster to disconnect from"
Fixes: #36994