Skip to content
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

Increase interoperability with docker-java #1245

Merged
merged 4 commits into from
Aug 10, 2019
Merged

Increase interoperability with docker-java #1245

merged 4 commits into from
Aug 10, 2019

Conversation

tobiasstadler
Copy link
Contributor

docker-java users registry.* instead docker.* for registry auth config. Provide a fall back to registry.* when docker.* auth config is not found in order to increase interoperability to docker-java

…g. Provide a fallback to registry.* when docker.* auth config is not found in order to increase interoperability with docker-java

Signed-off-by: Tobias Stadler <[email protected]>
@codecov
Copy link

codecov bot commented Jul 13, 2019

Codecov Report

Merging #1245 into master will decrease coverage by 1.59%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master    #1245     +/-   ##
===========================================
- Coverage     55.51%   53.92%   -1.6%     
+ Complexity     1760     1638    -122     
===========================================
  Files           156      155      -1     
  Lines          8471     8251    -220     
  Branches       1301     1264     -37     
===========================================
- Hits           4703     4449    -254     
- Misses         3314     3359     +45     
+ Partials        454      443     -11
Impacted Files Coverage Δ Complexity Δ
...o/fabric8/maven/docker/util/AuthConfigFactory.java 62.97% <100%> (+8.9%) 64 <7> (+1) ⬆️
.../main/java/io/fabric8/maven/docker/RemoveMojo.java 0% <0%> (-96.73%) 0% <0%> (-33%)
...rc/main/java/io/fabric8/maven/docker/StopMojo.java 0% <0%> (-65.75%) 0% <0%> (-33%)
.../io/fabric8/maven/docker/util/NamePatternUtil.java 84.61% <0%> (-13.57%) 12% <0%> (-16%)
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java 77.02% <0%> (-10.82%) 34% <0%> (-4%)
...va/io/fabric8/maven/docker/AbstractDockerMojo.java 2.94% <0%> (-8.83%) 3% <0%> (-5%)
...aven/docker/model/ImageArchiveManifestAdapter.java 83.33% <0%> (-8.34%) 5% <0%> (-1%)
...io/fabric8/maven/docker/util/ImageArchiveUtil.java 86.95% <0%> (-7.25%) 31% <0%> (-2%)
...abric8/maven/docker/config/ImageConfiguration.java 53.52% <0%> (-6.22%) 15% <0%> (-5%)
...in/java/io/fabric8/maven/docker/util/GavLabel.java 70.58% <0%> (-5.89%) 6% <0%> (-1%)
... and 15 more

@rohanKanojia rohanKanojia requested a review from rhuss July 15, 2019 05:29
@rhuss
Copy link
Collaborator

rhuss commented Jul 22, 2019

Thanks for the PR ! Could you please help me to understand the use case for this request ? As d-m-p doesn't use docker-java I can't see where docker-java comes into play. Are you using another plugin which uses docker-java internally (which one ?) and you don't want to duplicate the properties ?

@tobiasstadler
Copy link
Contributor Author

Yes, I am also using Arquillian and Testcontainers in my project (both use docker-java). And it would be nice to set the properties only once.

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! I see you use point and I think it doesn't harm. So happy to integrate it.

Could you please fix the comments below and add an entry to the changelog.md ?

}

@Test
public void testRegistrySystemProperty() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we please avoid copy & pasting the test ? Just extract a common method which is called once with "docker" and once with "registry" from each individual test.

Helpful would be also a test that ensures that "docker" has precedence over "registry" to keep the default behaviour.

- Test that the docker.* properties are evaluated before registry.*
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rhuss rhuss merged commit 4e3d17f into fabric8io:master Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants