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

Fix logic bug to remove dangling images #1522

Merged
merged 1 commit into from
Feb 6, 2022

Conversation

romank8k
Copy link
Contributor

Hi @rohanKanojia,
As a follow up to #1513, I've been testing a private build of the changes on our CI machines, and discovered a logic bug.
The condition to remove the image only if there are no other associated tags/repos should be reversed.

Additionally I'm looking to address one other scenario of dangling images.
That happens when pushing tags.
For example, the following code could result in a dangling image that will never be cleaned up, so I'm thinking of extending the logic to clean it up here as well.

        if (alreadyHasImage) {
            log.warn("Target image '%s' already exists. Tagging of '%s' will replace existing image",
                targetImage, fullName);
        }

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #1522 (4bb9a78) into master (1a8a9ad) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1522      +/-   ##
============================================
+ Coverage     61.93%   62.01%   +0.07%     
- Complexity     2186     2189       +3     
============================================
  Files           167      167              
  Lines          9645     9645              
  Branches       1455     1455              
============================================
+ Hits           5974     5981       +7     
+ Misses         3145     3139       -6     
+ Partials        526      525       -1     
Impacted Files Coverage Δ
.../io/fabric8/maven/docker/service/BuildService.java 57.08% <100.00%> (+2.68%) ⬆️

@rohanKanojia
Copy link
Member

hmm makes sense. Could you please add a test for this too?

@romank8k
Copy link
Contributor Author

hi @rohanKanojia, sure I'll work on adding some test cases too.

@rohanKanojia rohanKanojia force-pushed the cleanup-dangling-images branch from fb05458 to 6fc6413 Compare February 6, 2022 13:07
@rohanKanojia
Copy link
Member

@rkhmelichek : I plan to cut 0.39.0 probably today or tomorrow. Do you think we should include this PR in that?

@rohanKanojia rohanKanojia force-pushed the cleanup-dangling-images branch 3 times, most recently from a37aa44 to 590b3ca Compare February 6, 2022 14:38
… other tags/is not tagged to any other repositories prior to removing

Signed-off-by: Roman Khmelichek <[email protected]>
@rohanKanojia rohanKanojia force-pushed the cleanup-dangling-images branch from 590b3ca to 4bb9a78 Compare February 6, 2022 14:39
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@rohanKanojia rohanKanojia merged commit 561c6ff into fabric8io:master Feb 6, 2022
@romank8k
Copy link
Contributor Author

romank8k commented Feb 7, 2022

@rohanKanojia, yes thanks for merging!
I will open a new pull request for handling dangling images as a result of pushing tags (and work on tests too).

@romank8k romank8k deleted the cleanup-dangling-images branch February 7, 2022 22:34
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