fix(cli): remove defaulting to keyring when --global-config set #20943
Conversation
This fixes a regression that caused the VS code extension to be unable to authenticate after making keyring usage on by default. This is because the VS code extension assumes the CLI will always use the session token stored on disk, specifically in the directory specified by --global-config. This fix makes keyring usage enabled when the --global-config directory is not set. This is a bit wonky but necessary to allow the extension to continue working without modification and without backwards compat concerns. In the future we should modify these extensions to either access the credential in the keyring (like Coder Desktop) or some other approach that doesn't rely on the session token being stored on disk.
|
@codex review |
cli/root.go
Outdated
| // either access the credential in the keyring (like Coder Desktop) or some other | ||
| // approach that doesn't rely on the session token being stored on disk. We set the | ||
| // global config directory in most CLI tests, so we need to skip this check for tests. | ||
| assumeExtensionInUse := r.globalConfig != config.DefaultDir() && !testing.Testing() |
There was a problem hiding this comment.
!testing.Testing()
this one really rubs me the wrong way (I'd rather have logic in tests that enforce using keyring, but I'll let @deansheather be the final judge
There was a problem hiding this comment.
Same. I don't see an easy way around this right now. If I unset --global-config in the tests, I think parallel CLI invocations will be reading/writing to the same directory (e.g. URL) and won't have isolation. Any ideas?
There was a problem hiding this comment.
I think I might be able to set the HOME env var in keyring tests to work around this. Checking.
There was a problem hiding this comment.
That didn't work, but I made a way to override this behavior for tests that need to exercise the keyring even though --global-config is set. I also added test coverage for when --global-config is set by modifying how existing CLI tests are setup (removed --use-keyring=false so only --global-config remains).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This fixes a regression that caused the VS code extension to be unable to authenticate after making keyring usage on by default. This is because the VS code extension assumes the CLI will always use the session token stored on disk, specifically in the directory specified by --global-config.
This fix makes keyring usage enabled when the --global-config directory is not set. This is a bit wonky but necessary to allow the extension to continue working without modification and without backwards compat concerns. In the future we should modify these extensions to either access the credential in the keyring (like Coder Desktop) or some other approach that doesn't rely on the session token being stored on disk.
Tests:
coder login dev.coder.com-> token stored in keyringcoder login --global-config=/tmp/ dev.coder.com-> token stored in/tmp/session