-
Notifications
You must be signed in to change notification settings - Fork 644
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 #541: Allow @sha256 digest for tags in FROM #1131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1131 +/- ##
============================================
+ Coverage 52.23% 52.29% +0.06%
- Complexity 1468 1472 +4
============================================
Files 150 150
Lines 7840 7852 +12
Branches 1166 1168 +2
============================================
+ Hits 4095 4106 +11
Misses 3347 3347
- Partials 398 399 +1
|
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.
Looks good except that I doubt that the registry parsing works as expected when using a digest. Could you please double check this ? thanks !
if(fullName.contains("@sha256")) { // Of it contains digest | ||
String[] digestParts = fullName.split("@"); | ||
digest = digestParts[1]; | ||
repository = digestParts[0]; |
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.
What if the user specified a registry/respository@sha245:digest ? What happens with the registry ?
@@ -125,6 +125,8 @@ public void testRegistryNamingExtended() throws Exception { | |||
new ImageName("docker.jolokia.org/org/jolokia/jolokia_demo").getFullName("another.registry.org")); | |||
assertEquals("docker.jolokia.org/org/jolokia/jolokia_demo:latest", | |||
new ImageName("docker.jolokia.org/org/jolokia/jolokia_demo").getFullName(null)); | |||
assertEquals("docker.jolokia.org/org/jolokia/jolokia_demo@sha256:2781907cc3ae9bb732076f14392128d4b84ff3ebb66379d268e563b10fbfb9da", | |||
new ImageName("docker.jolokia.org/org/jolokia/jolokia_demo@sha256:2781907cc3ae9bb732076f14392128d4b84ff3ebb66379d268e563b10fbfb9da").getFullName(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.
Could add a check here that ImageName.getRegistry() returns indeed docker.jolokia.org ? (i doubt that)
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.
Yep, you're right
30a3a36
to
0c8682a
Compare
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.
Looks good now. Thanks ! let's merge, I'm going to make a release tomorrow ...
Fixes #541