-
Notifications
You must be signed in to change notification settings - Fork 2
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
Option to disable keychain credentials #2579
base: sagerb-remove-unused-command-code
Are you sure you want to change the base?
Option to disable keychain credentials #2579
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.
It looks like this was branched off of the CLI removal. Can we change the base, or rebase this to isolate just the keychain credential changes? That will probably get CI passing again.
…o-disable-keychain-credentials-2
Oh yes, sorry about that. I've rebased the diff and updated from that updated branch as well. Thanks! |
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.
This is very clean. I just had a suggestion to trim up the description of the setting, and make it more similar to other VS Code setting descriptions.
Co-authored-by: Jordan Jensen <[email protected]>
To get CI to pass, we have to merge #2584 first. |
Intent
This PR introduces a configuration option to disable the use of the KeyRing for credential storage and instead use the file system. This is extremely useful for development as well as a good escape hatch for customers who may not have a working KeyRing environment (or preference there-towards).
Resolves #2519
Type of Change
Approach
I first added a new configuration option for the publisher extension, which is enabled by default:

I then added a corresponding command line option for the agent. This was a more straightforward change than passing the option for every credential-related call. The impact of this decision is that the user must restart the extension after changing the configuration setting. Since this won't be very common, I believe this was a reasonable optimization to keep the complexity low.
After those two changes were in place, I added a helper function within the extension to access the setting, then updated the service spawning code to reflect the current configuration value. The agent then stores that value and each time the agent's credential code initializes, it either bypasses or attempts to use the KeyChain dependency.
The agent's debug launch profile has been updated to support launching the agent with the command line set. I set the initial value to
false
as I found myself wanting to debug using the file-based credentials more often than the KeyChain alternative. If this is not desired by the developer, they can easily change it within their own config.User Impact
New feature, but default maintains the behavior they had previously.
Automated Tests
Unit tests added to validate the desired behavior.
Directions for Reviewers
Launching a debug version of the extension from your VSCode instance for the extension, will work for verification. The configuration value is respected and maintained after being set, when you then reload the window with
Developer: Reload Window
. I'd suggest verification of the functionality by:~.connect-credentials
file.Checklist