-
Notifications
You must be signed in to change notification settings - Fork 491
Conversation
This must be merged! |
This looks reasonable but I would like the input from somebody else e.g. @davidxia wrt. the correct use of the docker-client. |
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.
LGTM overall. Thanks for the PR!
.password(server.getPassword()) | ||
.build(); | ||
} | ||
return null; |
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.
Might be nice to log something here in case the user is expecting a Server to be found
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.
Sure.
@rlodge would you be willing to add something to the README and/or changelog describing how to use this? |
Yes, I can give you another pull request against master with that. Something like: In CHANGELOG.md 1.3.4 (unreleased)
and in README.md Authenticating with maven settings.xmlSince version 1.3.4, you can authenticate using your maven settings.xml instead <configuration>
<repository>docker-repo.example.com:8080/organization/image</repository>
<tag>latest</tag>
<useMavenSettingsForAuth>true</useMavenSettingsForAuth>
</configuration> You can also use Then, in your maven settings file, add configuration for the server: <servers>
<server>
<id>docker-repo.example.com:8080</id>
<username>me</username>
<password>mypassword</password>
</server>
</servers> exactly as you would for any other server configuration. Let me know if that sounds like what you're looking for. |
Looks perfect to me! |
Ok. Added pull request #72 with the documentation. |
A proposed fix for issue #64 . Also adds the ability to configure the location of the docker configuration file if it's in a non-standard location.