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
Add support for Docker credential helpers #460
Add support for Docker credential helpers #460
Conversation
This adds the ability to retrieve registry credentials from a credential helper specified in `credHelpers` or `credsStore` inside ~/.docker/config.json.
@microsoft-github-policy-service agree company="Netflix" |
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.
Nice, thanks for putting up this PR! Left some minor comments
Thanks for the review! I made the suggested changes. |
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.
Thanks, left a few comments.
Thanks! I've made these changes. |
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.
Great, thanks! Left another question and ran the linter.
Thanks, addressed this latest round of comments. |
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 awesome, thanks for taking the time (triggered the existing unit tests to run on this PR).
Do you have any suggestions on how to craft unit tests for this? Most of our unit tests run in linux, and we do have a subset of them running in windows. Theoretically we could run against macOS too (if we wanted to...)
Currently familiarizing myself with how this works
Great question. The credential helper "protocol" is very simple and it would be easy to mock a credential helper. But to ensure the returned creds are actually used, we'd need an authenticated registry to connect to, and I'm not sure what kind of test infra exists for that. |
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.
+1 to some of the earlier feedback (which you've addressed). I don't want to block on the tests - I can make it an action item for myself if you don't have the time. There's one minor linting error in CI.
Validated this works well on MacOS with AzureCR creds (this branch is ./devcontainer.js
and the current release is on my PATH)
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.
Thanks for the update. Left another comment on the use of a sync call. Otherwise 👍
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.
Thanks for your contribution!
Thanks so much for the help geting this through! It's been a great experience. |
This adds the ability to retrieve registry credentials from a credential helper specified in
credHelpers
orcredsStore
inside ~/.docker/config.json.