Fix broken selector and DAG calculation logic after --{include,skip}-needs addition with correct Release IDs (#1823)

#1772 broke `--selector` with `needs` in many ways.

The two biggest problems I've encountered were:

- duplicate releases even if you've provided a proper `selector` to deduplicate
- sync/deletion ordering broken when you have `needs`

For the first issue, we had to update `getSelectedReleases` function to also calculate the "selected releases and releases needed by the selected releases", and use that to calculate the DAG. That should have been done in #1772.

The latter started happening after I've fixed the first issue. The source of the issue was that `needs` turned out to be ambiguous in a few cases.

Previously, `needs: ["foo/bar"]` had two meanings. One for "needs release bar in kubecontext foo", another for "needs release bar in namespace foo".

Moreover, `needs: ["foo/bar/baz"]` had three meanings.

- `needs release baz in tiller namespace foo and namespace baz`
- `needs release baz in namespace bar in kubecontext foo`
- `needs release baz in tiller namespace bar in kubecontext foo`.

Especially, the first meaning doesn't make sense at all. Helm 2 solely use tillerNamespace for namespacing the release and Helm 3 uses namespace for that.

This fix sorts all the bugs and issues I've found so far around that, by changing the meanings of the above two examples as follows:

- `foo/bar` means `namespace=foo,name=bar` for Helm 3 and `tillerNamespace=foo,name=bar` for Helm 2
  - `needs release bar in kubecontext foo` is now `foo//bar`. Notice the extra `/` between `foo` and `bar`.
- `foo/bar/baz` means `kubecontext=foo,namespace=bar,name=baz` for Helm 3 and `kubecontext=foo,tillerNamespace=bar,name=baz` in Helm 2

Fixes #1818

pkg/state/state.go

@@ -338,14 +338,56 @@ func (st *HelmState) ApplyOverrides(spec *ReleaseSpec) {
 	}
 	if st.OverrideNamespace != "" {
 		spec.Namespace = st.OverrideNamespace
-
-		for i := 0; i < len(spec.Needs); i++ {
-			n := spec.Needs[i]
-			if len(strings.Split(n, "/")) == 1 {
-				spec.Needs[i] = st.OverrideNamespace + "/" + n
-			}
-		}
 	}
+
+	var needs []string
+
+	// Since the representation differs between needs and id,
+	// correct it by prepending Namespace and KubeContext.
+	for i := 0; i < len(spec.Needs); i++ {
+		n := spec.Needs[i]
+
+		var kubecontext, ns, name string
+
+		components := strings.Split(n, "/")
+
+		name = components[len(components)-1]
+
+		if len(components) > 1 {
+			ns = components[len(components)-2]
+		} else if spec.TillerNamespace != "" {
+			ns = spec.TillerNamespace
+		} else {
+			ns = spec.Namespace
+		}
+
+		if len(components) > 2 {
+			kubecontext = components[len(components)-3]
+		} else {
+			kubecontext = spec.KubeContext
+		}
+
+		var componentsAfterOverride []string
+
+		if kubecontext != "" {
+			componentsAfterOverride = append(componentsAfterOverride, kubecontext)
+		}
+
+		// This is intentionally `kubecontext != "" || ns != ""`, but "ns != ""
+		// To avoid conflating kubecontext=,namespace=foo,name=bar and kubecontext=foo,namespace=,name=bar
+		// as they are both `foo/bar`, we explicitly differentiate each with `foo//bar` and `foo/bar`.
+		// Note that `foo//bar` is not always a equivalent to `foo/default/bar` as the default namespace is depedent on
+		// the user's kubeconfig.
+		if kubecontext != "" || ns != "" {
+			componentsAfterOverride = append(componentsAfterOverride, ns)
+		}
+
+		componentsAfterOverride = append(componentsAfterOverride, name)
+
+		needs = append(needs, strings.Join(componentsAfterOverride, "/"))
+	}
+
+	spec.Needs = needs
 }
 
 type RepoUpdater interface {
@@ -624,13 +666,25 @@ func ReleaseToID(r *ReleaseSpec) string {
 	}
 
 	tns := r.TillerNamespace
+	ns := r.Namespace
+
 	if tns != "" {
 		id += tns + "/"
+	} else if ns != "" {
+		id += ns + "/"
 	}
 
-	ns := r.Namespace
-	if ns != "" {
-		id += ns + "/"
+	if kc != "" {
+		if tns == "" && ns == "" {
+			// This is intentional to avoid conflating kc=,ns=foo,name=bar and kc=foo,ns=,name=bar.
+			// Before https://github.com/roboll/helmfile/pull/1823 they were both `foo/bar` which turned out to break `needs` in many ways.
+			//
+			// We now explicitly differentiate each with `foo//bar` and `foo/bar`.
+			// Note that `foo//bar` is not always a equivalent to `foo/default/bar` as the default namespace is depedent on
+			// the user's kubeconfig.
+			// That's why we use `foo//bar` even if it looked unintuitive.
+			id += "/"
+		}
 	}
 
 	id += r.Name

pkg/state/state_run.go

@@ -151,20 +151,20 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas
 
 		// Only compute dependencies from non-filtered releases
 		if !r.Filtered {
-			// Since the representation differs between needs and id,
-			// correct it by prepending KubeContext.
 			var needs []string
 			for i := 0; i < len(r.Needs); i++ {
 				n := r.Needs[i]
-				if r.KubeContext != "" {
-					n = r.KubeContext + "/" + n
-				}
 				needs = append(needs, n)
 			}
 			d.Add(id, dag.Dependencies(needs))
 		}
 	}
 
+	var ids []string
+	for id := range idToReleases {
+		ids = append(ids, id)
+	}
+
 	var selectedReleaseIDs []string
 
 	for _, r := range opts.SelectedReleases {
@@ -243,11 +243,11 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas
 		})
 
 		for _, id := range idsInGroup {
-			releases, ok := idToReleases[id]
+			rs, ok := idToReleases[id]
 			if !ok {
-				panic(fmt.Errorf("bug: unexpectedly failed to get releases for id %q", id))
+				panic(fmt.Errorf("bug: unexpectedly failed to get releases for id %q: %v", id, ids))
 			}
-			releasesInGroup = append(releasesInGroup, releases...)
+			releasesInGroup = append(releasesInGroup, rs...)
 		}
 
 		result = append(result, releasesInGroup)