-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adds parser for Docker config. #102
Conversation
@@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file. | |||
|
|||
### Fixed | |||
|
|||
## 0.1.2 | |||
### Added | |||
- Use credentials from Docker config if none can be found 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.
This should be populated with the next PR that adds this functionality to RetrieveCredentialsStep
.
@@ -35,5 +37,10 @@ public static Authorization withBasicCredentials(String username, String secret) | |||
return new Authorization("Basic", token); | |||
} | |||
|
|||
/** Creates an {@link Authorization} with a base64-encoded {@code username:password} string. */ |
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.
do we/can we ensure this is only used for docker-config consumed credentials?
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 intended to be used with any basic authorization token, which are in the form of username:password
.
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.
oh yeah, I didn't really remember if Authorization was part of the api, but I mean are we only creating Authorizations withBasicToken when using docker-config?
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.
Yea, the only place that uses this now is in the DockerConfigCredentialRetriever
.
public class DockerConfigCredentialRetriever { | ||
|
||
private static final Path DOCKER_CONFIG_FILE = | ||
Paths.get(System.getProperty("user.home")).resolve(".docker").resolve("config.json"); |
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.
is this true for windows/mac too?
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.
Yes, on windows, the user.home
would give %USERPROFILE%
, whereas on the unix systems, it gives $HOME
.
https://docs.docker.com/engine/reference/commandline/login/#privileged-user-requirement states the location of the config file.
* | ||
* <p>Example: | ||
* | ||
* <pre{@code |
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.
do you need to close this pre
tag with a >
? It's rendering funny here.
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.
Oh yes
Part of #101.
Need to still add the portion to actually use this in
RetrieveCredentialsStep
.