-
Notifications
You must be signed in to change notification settings - Fork 2k
Allow absolute path in credential store/helpers #6104
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
base: master
Are you sure you want to change the base?
Conversation
This would allow users to use a helper that does not live in path. The underlying credentials helper library uses os/exec exec.Command so this should be safe to do. I should add a test, please advice for the best location to do that. Signed-off-by: Erlend Hamnaberg <[email protected]>
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.
Just a nit:
Co-authored-by: Laura Brehm <[email protected]> Signed-off-by: Erlend Hamnaberg <[email protected]>
a7c17c1
to
e5ff2f8
Compare
Thanks @laurazard. I am not really a go programmer |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6104 +/- ##
==========================================
- Coverage 55.04% 55.02% -0.03%
==========================================
Files 361 361
Lines 30142 30145 +3
==========================================
- Hits 16591 16586 -5
- Misses 12594 12601 +7
- Partials 957 958 +1 🚀 New features to boost your workflow:
|
In general, I wonder if you could provide more context on why you need this change; ultimately, the CLI would look for binaries in That's not a "hard reject" for this change, but we're also working on various changes around authentication (and credential stores); I want to avoid introducing more complexity if it can get in the way of those changes (especially if it's for a niche-case that already could have workarounds (like |
Currently i am using docker in a setting where I need to generate the configuration file and set credentials to docker repos as part of this generation. Since I have to then also generate a shell script to piggyback to my own tooling and manipulate the path is annoying, but not a showstopper. Since I cant enforce how my users set their PATH variables, I have to build a shim on top of docker to set the PATH correctly. This would help by making this interaction a bit less annoying. I also think users would like to be able to specify absolute paths to these authentication tools. |
Signed-off-by: Erlend Hamnaberg <[email protected]>
f66b6ce
to
b198fa4
Compare
Implements #6103
Allows users to use a credential helper that does not live in path. The underlying credentials helper library uses os/exec exec.Command so this should be safe to do.
I should add a test, please advice for the best location to do that.
- What I did
Checks if the path is absolute, then return that instead of prepending docker-credential-
- How I did it
uses filepath to check if the path is absolute
- How to verify it
use an absolute path to a binary file which implements the docker-credential protocol.
- Human readable description for the release notes