Improve removal of temporary tags.

[thoschaefer]

Make sure to notify the user if removal of temporary image failed. Also set the force parameter when trying to remove temporary image.

We observed at our Jenkins instance that build job 'n' succeeded but failed to remove the docker tag. Build job 'n+1' then failed with

"Cannot temporarily tag %s with %s because target image already exists. " +
"Please remove this and retry.",

from method tagTemporaryImage. I also wonder if tagTemporaryImage should not try to simply overwrite existing target tags as docker would do it this way itself. That could be another PR I guess.

[codecov]

Codecov Report

Merging #725 into master will decrease coverage by 0.31%.
The diff coverage is 33.6%.

@@             Coverage Diff              @@
##             master     #725      +/-   ##
============================================
- Coverage     47.82%   47.51%   -0.32%     
- Complexity     1010     1023      +13     
============================================
  Files           125      126       +1     
  Lines          6605     6699      +94     
  Branches        851      872      +21     
============================================
+ Hits           3159     3183      +24     
- Misses         3176     3244      +68     
- Partials        270      272       +2

Impacted Files	Coverage Δ	Complexity Δ
...8/maven/docker/assembly/DockerAssemblyManager.java	14.14% <ø> (ø)	6 <0> (ø)	⬇️
...ric8/maven/docker/config/BuildImageSelectMode.java	0% <0%> (ø)	0 <0> (?)
...ic8/maven/docker/access/DockerAccessException.java	36.36% <0%> (-13.64%)	2 <0> (ø)
...rc/main/java/io/fabric8/maven/docker/SaveMojo.java	0% <0%> (ø)	0 <0> (?)
...c/main/java/io/fabric8/maven/docker/StartMojo.java	0% <0%> (ø)	0 <0> (ø)	⬇️
.../main/java/io/fabric8/maven/docker/SourceMojo.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...va/io/fabric8/maven/docker/AbstractDockerMojo.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...ava/io/fabric8/maven/docker/access/UrlBuilder.java	68.37% <100%> (+0.54%)	21 <1> (+1)	⬆️
...8/maven/docker/config/BuildImageConfiguration.java	81.81% <100%> (+3.24%)	45 <0> (+1)	⬆️
...abric8/maven/docker/config/ImageConfiguration.java	44.92% <100%> (ø)	11 <2> (ø)	⬇️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4f5291...e5ee4d8. Read the comment docs.

[rhuss]

thanks for the PR and sorry for the delay. quite busy weeks ;-(

The reason why tags are not overwritten forcefully is, that there could be an equally named image which has been created outside of and not related to d-m-p, too. Unlikely, I know, but overwriting this tag is probably not very polite.

I agreed that the information that a temporary tag couldn't be removed should bubble up. But maybe we could do it without chaining the DockerAccess exception but merging it into one:

sth like

finally {
    if (!removeTag(...)) {
         if (dae == null) {
             throw new DockerAccessException("Image %s could be pushed, but the temporary tag could not be removed", ...);
         }  else {
            throw new DockerAccessException(dae.getCause(), dae.getMessage() + " and the temporary tag could not be removed, too");
        }
   }

}

Still not sure whether the removing of the temporary tag should be considered an fatal error breaking call as the image already could be pushed. Would be awesome if there would be a rollback ;-), but there's nothing like removing an image from a registry ....

[rhuss]

@thoschaefer what's you opinion on an adapted error handling as mentioned above ?

[thoschaefer]

I think your proposal regarding adapting error handling sounds valid. I will update my PR accordingly.

Regarding you thought about "whether removing of the temporary tag should be considered a fatal error": I am not sure anymore if it should be a fatal error. On the other hand it's confusing if a later build fails because it stumbles upon the non-removed temporary image. Don't understand yet why the image could not be removed. Maybe also worth a retry.

[rhuss]

looks good to me. Let's fail the build if the temporary image could not be removed.

[rhuss]

[merge]