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

Allow repeated ctp connects #434

Merged
merged 3 commits into from
Feb 29, 2024
Merged

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Feb 28, 2024

Do the full fix of #401.

@@ -62,9 +62,18 @@ func (c *connectCmd) Run(ctx context.Context, p pterm.TextPrinter, upCtx *upboun
}

// Check if the fs kubeconfig is already pointing to a control plane and return early if so.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think this comment is now wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@AlainRoy AlainRoy left a comment

Choose a reason for hiding this comment

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

One nit, one question about how this looks to a user (not sure about my question). Code looks okay, so I'll approve. If my question makes sense, I'll trust you to fix that bit without a second review. :)

if err := clientcmd.ModifyConfig(clientcmd.NewDefaultClientConfigLoadingRules(), kubeConfig, false); err != nil {
return err
}
p.Printfln("Switched back to context %q.", oldContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this will be confusing for a user. If I'm reading this correctly, they'll see two messages in a row:

Switched back to context
Current context set to

That looks confusing to me. Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. Not 100% sure which is better though.

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.com>
@sttts sttts merged commit b6daed1 into upbound:main Feb 29, 2024
6 checks passed
@sttts sttts deleted the sttts-multiple-connects branch February 29, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants