Correctly parse build tags that contain port numbers#1001
Correctly parse build tags that contain port numbers#1001r2d4 merged 6 commits intoGoogleContainerTools:masterfrom
Conversation
maddisondavid
requested review from
balopat,
dgageot,
nkubala and
r2d4
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1001 +/- ##
==========================================
- Coverage 40.61% 40.21% -0.41%
==========================================
Files 68 68
Lines 2969 3009 +40
==========================================
+ Hits 1206 1210 +4
- Misses 1639 1672 +33
- Partials 124 127 +3
Continue to review full report at Codecov.
|
pkg/skaffold/deploy/helm.go
Outdated
| if r.ImageStrategy.HelmImageConfig.HelmConventionConfig != nil { | ||
| tagSplit := strings.Split(v.Tag, ":") | ||
| imageRepositoryTag := fmt.Sprintf("%s.repository=%s,%s.tag=%s", k, tagSplit[0], k, tagSplit[1]) | ||
| tagIndex := strings.LastIndex(v.Tag, ":") |
There was a problem hiding this comment.
Would it make sense to parse the image tag with something like docker.ParseReference()? If it works, it would handle all the edge cases better than any string parsing
There was a problem hiding this comment.
Sounds good, I wasn't aware of docker.ParseReference(), it's annoying that it only gives you the baseName and not also the tag.
Code updated to use docker.ParseReference() which leads to much nicer code.
I followed the style of the helm_test.go to fix up the test, but I'm not super happy about it because the test uses the SAME logic as the code it's testing.
There was a problem hiding this comment.
Doh, I realise that docker.ParseReference() is in a skaffold package, so probably makes more sense for me to update docker.ParseReference() to include the Tag.
|
The Travis build is failing but only on the Linux build with some weird missing packages. I'm leaning towards it just being an environmental issue blip during the build but I don't have permission to re-run the build. |
|
Let me restart the build |
dgageot
left a comment
dgageot
left a comment
There was a problem hiding this comment.
Thank you @maddisondavid for updating the code.! There's a few nits to be fixed then it can be merged
|
I've just run into an issue using MASTER whereby the last letter of the tag is missing when Helm is deployed. Please don't merge this until I can confirm it's not a problem with the code I put in! |
|
OK, seems like it must have been something around my environment. Everything appears to be working fine now and deployment is getting and using the correct tag. I just wanted to be sure it wasn't something caused by my code! |
|
Thank you @maddisondavid! |
5 participants
Fixes issue #994
When a build tag contains a port number i.e. docker.io:5000/skaffold-helm:my-tag the Helm Strategy was incorrectly performing a basic split and ending up with repository=docker.io,tag=5000/skaffold-helm:my-tag.
This PR fixes the parsing so that the repository is correctly extracted, i.e. repository=docker.io:5000/skaffold-helm,image=my-tag