- Notifications
You must be signed in to change notification settings - Fork927
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Should we link it with any issue?
cli/configssh.go Outdated
| // 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. |
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.
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.
?
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.
Is this something we can 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.
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 toall 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.
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.
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.
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.
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.
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.
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.
fb1d0d9 LGTM 👍 thanks
54055dc intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.