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

Add skipPush option to build image configuration (#1243) #1368

Merged
merged 5 commits into from
Sep 1, 2020

Conversation

mattnelson
Copy link
Contributor

Add skipPush option to build image configuration from #1243

Was getting test failures locally with AuthConfigFactoryTest due to my ~/.kube/config setup, wasn't sure what needed to be done to get those to pass.

Example of the output from my build which consumed these proposed plugin changes.

[INFO] --- docker-maven-plugin:0.33-SNAPSHOT:push (push-image) @ my-module ---
[INFO] DOCKER> [my-registry/my-image:image1] : Skipped pushing
[INFO] DOCKER> Credentials helper reply for "docker-credential-desktop" is 0.6.0
[INFO] DOCKER> The push refers to repository [my-registry/my-image]
[INFO] DOCKER> image2: digest: sha256:abcd size: 1234
[INFO] DOCKER> Temporary image tag skipped. Target image 'my-registry/my-image:image2' already has registry set or no registry is available
[INFO] DOCKER> Pushed my-registry/my-image:image2 in 18 seconds

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #1368 into master will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1368      +/-   ##
============================================
+ Coverage     56.83%   57.13%   +0.29%     
- Complexity     1842     1844       +2     
============================================
  Files           159      159              
  Lines          8730     8723       -7     
  Branches       1337     1336       -1     
============================================
+ Hits           4962     4984      +22     
+ Misses         3299     3270      -29     
  Partials        469      469              
Impacted Files Coverage Δ Complexity Δ
...8/maven/docker/config/BuildImageConfiguration.java 80.56% <100.00%> (+1.22%) 80.00 <3.00> (+1.00)
...aven/docker/config/handler/property/ConfigKey.java 100.00% <100.00%> (ø) 10.00 <0.00> (ø)
...config/handler/property/PropertyConfigHandler.java 76.47% <100.00%> (ø) 123.00 <0.00> (ø)
.../fabric8/maven/docker/service/RegistryService.java 82.05% <100.00%> (+18.05%) 14.00 <0.00> (+3.00)
.../io/fabric8/maven/docker/service/BuildService.java 50.69% <0.00%> (-0.44%) 25.00% <0.00%> (-1.00%)
...a/io/fabric8/maven/docker/access/BuildOptions.java 96.15% <0.00%> (-0.28%) 19.00% <0.00%> (-2.00%)
...abric8/maven/docker/config/ImageConfiguration.java 60.75% <0.00%> (+2.53%) 20.00% <0.00%> (ø%)
... and 2 more

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.

Looks good, but I would add a test for pushImages, too in RegistryServiceTest if possible.

If you dont have time for that, we can merge this PR nevertheless

Thanks a lot for your contribution !

@@ -45,6 +45,11 @@ public void pushImages(Collection<ImageConfiguration> imageConfigs,
int retries, RegistryConfig registryConfig, boolean skipTag) throws DockerAccessException, MojoExecutionException {
for (ImageConfiguration imageConfig : imageConfigs) {
BuildImageConfiguration buildConfig = imageConfig.getBuildConfiguration();
if (buildConfig.skipPush()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know there is a test missing in RegistryServiceTest for pushImages, but it would be cool if you could add one to test this change, too (then the Sonar check would be happy, too). It shouldn't be hard to add, as it should work the same as the pull test, too.

@rhuss
Copy link
Collaborator

rhuss commented Aug 30, 2020

Ah, and could you please also add an enty to the changelog.md ? thanks !

@mattnelson
Copy link
Contributor Author

Looks good, but I would add a test for pushImages, too in RegistryServiceTest if possible.

Ah, and could you please also add an enty to the changelog.md ? thanks !

both done in 2dba1fb

@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 3 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

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.

Cool, thanks a ton !

@rhuss rhuss merged commit 493aa8b into fabric8io:master Sep 1, 2020
@mattnelson mattnelson deleted the 1243_image_skip_push branch September 1, 2020 13:57
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