Fix helm deployer with imageStrategy helm and fix test runner

[michaelbeaumont]

This PR fixes #2902 and another bug revealed after fixing the first.

The first is deploying with helm with the following config

skaffold.yaml

...
deploy:
  helm:
    releases:
      - name: skaffold-helm
        chartPath: k8s/skaffold-helm
        values:
          image: skaffold-helm
        imageStrategy:
          helm: {}
      ...

when we deploy with helm we get

helm install --set image=skaffold-helm,tag=skaffold:helm

because of a bug introduced changing the format string from 4899ae5#diff-5c13a6850c742c6b4fa07cf1f6aeb088L210 into 4899ae5#diff-5c13a6850c742c6b4fa07cf1f6aeb088R291

After processing a bracketed expression [n], subsequent verbs will use arguments n+1, n+2, etc. unless otherwise directed.
https://golang.org/pkg/fmt/

After fixing this bug, we get the following incorrect helm command

helm install --set image=skaffold-helm,tag=skaffold-helm:latest

because of the exact same change where extractTag was removed.

These were both missed because of 4899ae5#diff-a1535a4f321ed1903fdc7f27ac166432L534
where some tests were no longer executed.

ORIGINAL:

To be honest, I'm not totally sure whether the behavior is correct in all possible cases here, given #2697 and #1379 and given the PR #2624 but the tests are all passing (and running) now and skaffold is working for us again.

[codecov]

Codecov Report

Merging #2887 into master will increase coverage by 0.06%.
The diff coverage is 92%.

Impacted Files	Coverage Δ
pkg/skaffold/deploy/helm.go	76.22% <92%> (+2.02%)	⬆️

[tejal29]

Thank you for our change @michaelbeaumont.
Can you please list the output lines change in this PR.
e.g. #2811

[tejal29]

Please highlight the output line changes in the PR like
#2811

[michaelbeaumont]

I've edited the PR description to better describe the two bugs. Again, note that in my PR I've reactivated two tests so that they completely run. which fail on master without my first 2 commits. The effect isn't readily visible in the skaffold output unless we have -v=debug:

Before

DEBU[0004] Running command: [helm --kube-context minikube install --name tartarus k8s/tartarus --set postgresql.image.registry=docker.pkg.github.com,postgresql.image.repository=docker.pkg.github.com,postgresql.image.tag=docker.pkg.github.com]

After

DEBU[0004] Running command: [helm --kube-context minikube install --name tartarus k8s/tartarus --set postgresql.image.registry=docker.pkg.github.com,postgresql.image.repository=gastromatic/dataservice/tartarus,postgresql.image.tag=44de73a55041f0c31cf79fbda53cfdad92591621eccee0900a4e51ffb80fd40b] 

[tejal29]

@michaelbeaumont would you be up for writing unit test for this?
If not, i can refactor and add some tests on your PR.

Thanks
Tejal

[michaelbeaumont]

@tejal29 as mentioned, unit tests already exist that do catch this bug and would have caught it:

skaffold/pkg/skaffold/deploy/helm_test.go

Lines 363 to 373 in 95d28f4

	description: "get failure should install not upgrade with helm image strategy",
	commands: &MockHelm{
	getResult: fmt.Errorf("not found"),
	installMatcher: func(cmd *exec.Cmd) bool {
	dockerRef, err := docker.ParseReference(testBuilds[0].Tag)
	if err != nil {
	return false
	}
	expected := fmt.Sprintf("image.repository=%s,image.tag=%s", dockerRef.BaseName, dockerRef.Tag)
	return util.StrSliceContains(cmd.Args, expected)

skaffold/pkg/skaffold/deploy/helm_test.go

Lines 381 to 386 in 95d28f4

	description: "helm image strategy with explicit registry should set the Helm registry value",
	commands: &MockHelm{
	getResult: fmt.Errorf("not found"),
	installMatcher: func(cmd *exec.Cmd) bool {
	expected := fmt.Sprintf("image.registry=%s,image.repository=%s,image.tag=%s", "docker.io:5000", "skaffold-helm", "3605e7bc17cf46e53f4d81c4cbc24e5b4c495184")
	return util.StrSliceContains(cmd.Args, expected)

but they were turned off because of a bug in the test runner itself causing these assertions not to be checked.

skaffold/pkg/skaffold/deploy/helm_test.go

Lines 544 to 545 in 95d28f4

	if m.upgradeMatcher != nil && !m.installMatcher(c) {
	m.t.Errorf("install matcher failed to match commands: %+v", c.Args)

so I'm not sure there's any unit tests to write here...

[tejal29]

@michaelbeaumont, Thanks for the clarification. I actually want to write a test to give different strategy, no strategy and then test actual set Values generated.
The way the tests are written right now with mock, its a little bit hard to test small functionalities especially like this.
This is not your fault and just the way we wrote this.
My plan was to, add a function getImageSetValueFromHelmStrategy

+func getImageSetValueFromHelmStrategy(cfg *latest.HelmConventionConfig, image string, tag string) (string, error) {
+       if cfg != nil {
+               dockerRef, err := docker.ParseReference(tag)
+               if err != nil {
+                       return "", errors.Wrapf(err, "cannot parse the image reference %s", tag)
+               }
+
+               if cfg.ExplicitRegistry {
+                       if dockerRef.Domain == "" {
+                               return "", errors.Wrapf(err, "image reference %s has no domain", tag)
+                       }
+                       return fmt.Sprintf(
+                               "%[1]s.registry=%[2]s,%[1]s.repository=%[3]s,%[1]s.tag=%[4]s",
+                               image,
+                               dockerRef.Domain,
+                               dockerRef.Path,
+                               dockerRef.Tag,
+                               ), nil
+               } else {
+                       return fmt.Sprintf(
+                               "%[1]s.repository=%[2]s,%[1]s.tag=%[3]s",
+                               image, dockerRef.BaseName,
+                               dockerRef.Tag,
+                               ), nil
+               }
+       } else {
+               return fmt.Sprintf("%s=%s", image, tag), nil
+       }
+}

And then add tests for this small function with all possible HelmConventionConfig

[tejal29]

@michaelbeaumont Our next skaffold release is this Thursday. We really want to get this fix in. Would you be able to merge this in by Wednesday?

Thanks
Tejal

[michaelbeaumont]

@tejal29 I gave it a shot!

[tejal29]

Thanks a ton! I have to head out right now. i will take a look tomorrow.

[tejal29]

@michaelbeaumont Looks beautiful! The tests are so much easy to follow. Thank you for doing this!

[gregod-com]

@tejal29 @michaelbeaumont
TLDR:
Is the imageStrategy: helm: {} supposed to omit the sha256 hash from tags?

SETUP:
When I deploy my image with skaffold dev and without imageStrategy: helm: {} (and a very similar setup as described above) my deployed images on Kubernetes look something like 'registry:latest@sha256[somehash]'.
This allows for Skaffold to

• interpret the logs correctly
• do syncing of files correctly

But in this scenario I have to write a _helper.tpl entry that checks if the field imageName is set (by Skaffold) and if not I use image.repository and image.tag to populate the imageName value. (imageName is then used inside the actual deployment)

If on the other hand I deploy using imageStrategy: helm: {} (which would cleanup my charts a bit), the final image on Kubernetes looks like 'registry:latest' (without the hash)
With that change Skaffold still searches for the image with the tag including a sha. This renders Skaffold unable to

• interpret the logs (nothing is shown)
• syncing files

Is this the intended way or should the hash somehow be appended to the tag? (as it currently is without the imageStrategy: helm: {})

[michaelbeaumont]

@GregorPirolt check out #2956