HelmFQNConfig is not used anywhere

[balopat]

What is the HelmFQNConfig good for? Can we just remove it?

[tejal29]

It was introduced in commit acc1927

commit acc1927b17c949d880766f952599426ef0fdb66d
Author: Eliran Bivas <eliran.bivas@gmail.com>
Date:   Tue Jul 17 12:26:01 2018 +0300

    add support for helm image convention vs fqn setting

diff --git a/pkg/skaffold/deploy/helm.go b/pkg/skaffold/deploy/helm.go
index ff95dec7..eb141f47 100644
--- a/pkg/skaffold/deploy/helm.go
+++ b/pkg/skaffold/deploy/helm.go
@@ -128,7 +128,14 @@ func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, buil
        var setOpts []string
        for k, v := range params {
                setOpts = append(setOpts, "--set")
-               setOpts = append(setOpts, fmt.Sprintf("%s=%s", k, v.Tag))
+               if r.ImageStrategy.HelmImageConfig.HelmFQNConfig != nil {
+                       setOpts = append(setOpts, fmt.Sprintf("%s=%s", k, v.Tag))
+               } else {
+                       tagSplit := strings.Split(v.Tag, ":")
+                       imageRepository := fmt.Sprintf("%s.repository=%s", k, tagSplit[0])
+                       imageTag := fmt.Sprintf("%s.tag=%s", k, tagSplit[1])
+                       setOpts = append(setOpts, imageRepository, imageTag)
+               }
        }
 
        // First build dependencies.
diff --git a/pkg/skaffold/schema/v1alpha2/config.go b/pkg/skaffold/schema/v1alpha2/config.go
index 39dbb6b0..c02a53aa 100644
--- a/pkg/skaffold/schema/v1alpha2/config.go
+++ b/pkg/skaffold/schema/v1alpha2/config.go
@@ -150,6 +150,7 @@ type HelmRelease struct {
        Wait              bool                   `yaml:"wait"`
        Overrides         map[string]interface{} `yaml:"overrides"`
        Packaged          *HelmPackaged          `yaml:"packaged"`
+       ImageStrategy     HelmImageStrategy      `yaml:"imageStrategy"`
 }
 
 // HelmPackaged represents parameters for packaging helm chart.
@@ -161,6 +162,24 @@ type HelmPackaged struct {
        AppVersion string `yaml:"appVersion"`
 }
 
+type HelmImageStrategy struct {
+       HelmImageConfig `yaml:",inline"`
+}
+
+type HelmImageConfig struct {
+       HelmFQNConfig        *HelmFQNConfig        `yaml:"fqn"`
+       HelmConventionConfig *HelmConventionConfig `yaml:"helm"`
+}

Later in commit, 6e72f84 @bivas changed the default.

tejaldesai@tejaldesai~/go/src/github.com/GoogleContainerTools/skaffold: (master)$ git show 6e72f8463
commit 6e72f8463533b9c50567ef8941da03528fbca173
Author: Eliran Bivas <eliran.bivas@gmail.com>
Date:   Mon Jul 23 10:29:31 2018 +0300

    at the moment, default to FQN instead of Helm image conventions

diff --git a/examples/helm-deployment/skaffold-helm/templates/deployment.yaml b/examples/helm-deployment/skaffold-helm/templates/deployment.yaml
index 713b7692..c66fb3d3 100644
--- a/examples/helm-deployment/skaffold-helm/templates/deployment.yaml
+++ b/examples/helm-deployment/skaffold-helm/templates/deployment.yaml
@@ -17,7 +17,7 @@ spec:
     spec:
       containers:
         - name: {{ .Chart.Name }}
-          image: {{ .Values.image.repository }}:{{ .Values.image.tag }}
+          image: {{ .Values.image }}
           imagePullPolicy: {{ .Values.pullPolicy }}
           command: ["/bin/bash", "-c", "--" ]
           args: ["while true; do sleep 30; done;"]
diff --git a/examples/helm-deployment/skaffold-helm/values.yaml b/examples/helm-deployment/skaffold-helm/values.yaml
index 2be4a9f8..5ac1a982 100644
--- a/examples/helm-deployment/skaffold-helm/values.yaml
+++ b/examples/helm-deployment/skaffold-helm/values.yaml
@@ -2,10 +2,12 @@
 # This is a YAML-formatted file.
 # Declare variables to be passed into your templates.
 replicaCount: 1
-image:
-  repository: nginx
-  tag: stable
-  pullPolicy: IfNotPresent
+image: nginx:stable
+# This is the helm convention on declaring images
+# image:
+#   repository: nginx
+#   tag: stable
+#   pullPolicy: IfNotPresent
 service:
   name: nginx
   type: ClusterIP
diff --git a/pkg/skaffold/deploy/helm.go b/pkg/skaffold/deploy/helm.go
index ec0dddec..9dd518d3 100644
--- a/pkg/skaffold/deploy/helm.go
+++ b/pkg/skaffold/deploy/helm.go
@@ -128,12 +128,12 @@ func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, buil
        var setOpts []string
        for k, v := range params {
                setOpts = append(setOpts, "--set")
-               if r.ImageStrategy.HelmImageConfig.HelmFQNConfig != nil {
-                       setOpts = append(setOpts, fmt.Sprintf("%s=%s", k, v.Tag))
-               } else {
+               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])
                        setOpts = append(setOpts, imageRepositoryTag)
+               } else {
+                       setOpts = append(setOpts, fmt.Sprintf("%s=%s", k, v.Tag))
                }
        }
 
diff --git a/pkg/skaffold/deploy/helm_test.go b/pkg/skaffold/deploy/helm_test.go
index 8b95e177..8028def7 100644
--- a/pkg/skaffold/deploy/helm_test.go
+++ b/pkg/skaffold/deploy/helm_test.go
@@ -62,11 +62,6 @@ var testDeployConfig = &v1alpha2.HelmDeploy{
                        SetValues: map[string]string{
                                "some.key": "somevalue",
                        },
-                       ImageStrategy: v1alpha2.HelmImageStrategy{
-                               HelmImageConfig: v1alpha2.HelmImageConfig{
-                                       HelmFQNConfig: &v1alpha2.HelmFQNConfig{},
-                               },
-                       },
                },
        },
 }
@@ -85,6 +80,11 @@ var testDeployHelmStyleConfig = &v1alpha2.HelmDeploy{
                        SetValues: map[string]string{
                                "some.key": "somevalue",
                        },
+                       ImageStrategy: v1alpha2.HelmImageStrategy{
+                               HelmImageConfig: v1alpha2.HelmImageConfig{
+                                       HelmConventionConfig: &v1alpha2.HelmConventionConfig{},
+                               },
+                       },
                },
        },
 }

Looks like, this should be oneof fields i.e. either set to "fqn" or "helm"

[tejal29]

Its part of ImageStrategy which is optional.

[balopat]

Thank you @tejal29 - yeah I think it should be a oneOf field. I'll change it in #1904 - as it turns out, due to the #1900 bug, the schema was actually correct for ImageStrategy.