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.

Sign up for GitHub

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

Correctly parse build tags that contain port numbers #1001

Merged
merged 6 commits into from
Sep 24, 2018
Merged

Correctly parse build tags that contain port numbers #1001

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb626f1
Correctly parse build tags that contain port numbers
maddisondavid Sep 20, 2018
621e7e6
Fix go fmt issues
maddisondavid Sep 20, 2018
0d079ae
Switch to using docker.parseReference
maddisondavid Sep 20, 2018
86c746f
Update docker.ParseReference to return the image tag
maddisondavid Sep 20, 2018
b4bf293
Handle error when parsing docker image reference
maddisondavid Sep 21, 2018
face328
Switch to using CheckDeepEqual
maddisondavid Sep 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/skaffold/deploy/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r v1alp
for k, v := range params {
setOpts = append(setOpts, "--set")
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, ":")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

imageRepositoryTag := fmt.Sprintf("%s.repository=%s,%s.tag=%s", k, v.Tag[0:tagIndex], k, v.Tag[tagIndex+1:len(v.Tag)-1])
setOpts = append(setOpts, imageRepositoryTag)
} else {
setOpts = append(setOpts, fmt.Sprintf("%s=%s", k, v.Tag))
Expand Down
7 changes: 4 additions & 3 deletions pkg/skaffold/deploy/helm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import (
var testBuilds = []build.Artifact{
{
ImageName: "skaffold-helm",
Tag: "skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184",
Tag: "docker.io:5000/skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184",
},
}

Expand Down Expand Up @@ -292,8 +292,9 @@ func TestHelmDeploy(t *testing.T) {
t: t,
getResult: fmt.Errorf("not found"),
installMatcher: func(cmd *exec.Cmd) bool {
builds := strings.Split(testBuilds[0].Tag, ":")
expected := map[string]bool{fmt.Sprintf("image.repository=%s,image.tag=%s", builds[0], builds[1]): true}
buildTag := testBuilds[0].Tag
tagIndex := strings.LastIndex(buildTag, ":")
expected := map[string]bool{fmt.Sprintf("image.repository=%s,image.tag=%s", buildTag[0:tagIndex], buildTag[tagIndex+1:len(buildTag)-1]): true}
for _, arg := range cmd.Args {
if expected[arg] {
return true
Expand Down