- Notifications
You must be signed in to change notification settings - Fork1.1k
fix(cli): remove defaulting to keyring when --global-config set#20943
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
This fixes a regression that caused the VS code extension to be unableto authenticate after making keyring usage on by default. This isbecause the VS code extension assumes the CLI will always use the sessiontoken stored on disk, specifically in the directory specified by--global-config.This fix makes keyring usage enabled when the --global-config directoryis not set. This is a bit wonky but necessary to allow the extension tocontinue working without modification and without backwards compat concerns.In the future we should modify these extensions to either access thecredential in the keyring (like Coder Desktop) or some other approach thatdoesn't rely on the session token being stored on disk.
ibetitsmike commentedNov 26, 2025
@codex review |
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
!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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think I might be able to set theHOME env var in keyring tests to work around this. Checking.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
💡 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".
Uh oh!
There was an error while loading.Please reload this page.
bbf7b13 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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