8000 fix(cli): prevent asynchronous print of version mismatch in config-ssh by mafredri · Pull Request #13845 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

fix(cli): prevent asynchronous print of version mismatch in config-ssh #13845

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

Merged
merged 2 commits into from
Jul 9, 2024
Merged

fix(cli): prevent asynchronous print of version mismatch in config-ssh #13845

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a76fd7d
fix(cli): prevent asynchronous print of version mismatch in config-ssh
mafredri Jul 9, 2024
fb1d0d9
clarify
mafredri Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions cli/configssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,8 @@ func (r *RootCmd) configSSH() *serpent.Command {
r.InitClient(client),
),
Handler: func(inv *serpent.Invocation) error {
ctx := inv.Context()

if sshConfigOpts.waitEnum != "auto" && skipProxyCommand {
// The wait option is applied to the ProxyCommand. If the user
// specifies skip-proxy-command, then wait cannot be applied.
Expand All @@ -244,7 +246,13 @@ func (r *RootCmd) configSSH() *serpent.Command {
sshConfigOpts.header = r.header
sshConfigOpts.headerCommand = r.headerCommand

recvWorkspaceConfigs := sshPrepareWorkspaceConfigs(inv.Context(), client)
// Talk to the API early to prevent bad placement of the
// version mismatch warning. The asynchronous requests
// issued by sshPrepareWorkspaceConfigs may trigger the
// warning at any time.
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to clarify what "bad placement" means.

How will this change address the issue if

The asynchronous requests issued by sshPrepareWorkspaceConfigs may trigger the warning at any time.

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we can test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can clarify.

How will this change address the issue if

By talking early. The warning is printed only once/on first API communication. Understanding this context requires knowledge of the version mismatch message, not sure it's worth expanding upon here. 🤔

Is this something we can test?

Not really, since version mismatch is a http transport middleware triggered on first API comms, it applies essentially to all CLI commands and testing this in a general way doesn't seem feasible or would require rewriting most of our tests to be run both with a matched and mismatched client/server.

Copy link
Member Author

Choose a reason for hiding this comment

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

fb1d0d9

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, thanks for the clarification; I think this detail would be relevant in the comment.
"Early" doesn't really explain why this fixes the issue, IMO.

Copy link
Member Author
@mafredri mafredri Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do see your point Danny, and how that could be valuable information for the reader. I just don't think repeating the behavior about how version mismatch works in every place there needs to be an exception is the right way to go about it.

The curious reader can search for "version mismatch" and find it/its doc:

// wrapTransportWithVersionMismatchCheck adds a middleware to the HTTP transport
// that checks for version mismatches between the client and server. If a mismatch
// is detected, a warning is printed to the user.

I'm open to ideas for improvement, if you think it really needs it. I just want to avoid documenting another components logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

fb1d0d9 LGTM 👍 thanks

_, _ = client.BuildInfo(ctx)

recvWorkspaceConfigs := sshPrepareWorkspaceConfigs(ctx, client)

out := inv.Stdout
if dryRun {
Expand Down Expand Up @@ -366,7 +374,7 @@ func (r *RootCmd) configSSH() *serpent.Command {
return xerrors.Errorf("fetch workspace configs failed: %w", err)
}

coderdConfig, err := client.SSHConfiguration(inv.Context())
coderdConfig, err := client.SSHConfiguration(ctx)
if err != nil {
// If the error is 404, this deployment does not support
// this endpoint yet. Do not error, just assume defaults.
Expand Down
Loading
0