feat: `helmfile diff --detailed-exitcode` should also detect deletions (#1186)

Resolves #499
Resolves #1072

pkg/app/app.go

@@ -116,12 +116,20 @@ func (a *App) DeprecatedSyncCharts(c DeprecatedChartsConfigProvider) error {
 }
 
 func (a *App) Diff(c DiffConfigProvider) error {
-	var deferredErrs []error
+	var allDiffDetectedErrs []error
 
-	err := a.ForEachStateFiltered(func(run *Run) []error {
+	var affectedAny bool
+
+	err := a.ForEachState(func(run *Run) (bool, []error) {
 		var criticalErrs []error
 
-		errs := run.Diff(c)
+		msg, matched, affected, errs := run.Diff(c)
+
+		if msg != nil {
+			a.Logger.Info(*msg)
+		}
+
+		affectedAny = affectedAny || affected
 
 		for i := range errs {
 			switch e := errs[i].(type) {
@@ -129,7 +137,7 @@ func (a *App) Diff(c DiffConfigProvider) error {
 				switch e.Code {
 				case 2:
 					// See https://github.com/roboll/helmfile/issues/874
-					deferredErrs = append(deferredErrs, e)
+					allDiffDetectedErrs = append(allDiffDetectedErrs, e)
 				default:
 					criticalErrs = append(criticalErrs, e)
 				}
@@ -138,14 +146,14 @@ func (a *App) Diff(c DiffConfigProvider) error {
 			}
 		}
 
-		return criticalErrs
+		return matched, criticalErrs
 	})
 
 	if err != nil {
 		return err
 	}
 
-	if len(deferredErrs) > 0 {
+	if c.DetailedExitcode() && (len(allDiffDetectedErrs) > 0 || affectedAny) {
 		// We take the first release error w/ exit status 2 (although all the defered errs should have exit status 2)
 		// to just let helmfile itself to exit with 2
 		// See https://github.com/roboll/helmfile/issues/749
@@ -806,84 +814,28 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) {
 		Set:     c.Set(),
 	}
 
-	var changedReleases []state.ReleaseSpec
+	infoMsg, releasesToBeUpdated, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts)
-	var deletingReleases []state.ReleaseSpec
+	if len(errs) > 0 {
-	var planningErrs []error
+		return false, false, errs
+	}
 
-	// TODO Better way to detect diff on only filtered releases
+	if releasesToBeDeleted == nil && releasesToBeUpdated == nil {
-	{
+		if infoMsg != nil {
-		changedReleases, planningErrs = st.DiffReleases(helm, c.Values(), c.Concurrency(), detailedExitCode, c.IncludeTests(), c.SuppressSecrets(), c.SuppressDiff(), false, diffOpts)
+			logger := c.Logger()
-
+			logger.Infof("")
-		var err error
+			logger.Infof(*infoMsg)
-		deletingReleases, err = st.DetectReleasesToBeDeletedForSync(helm, st.Releases)
-		if err != nil {
-			planningErrs = append(planningErrs, err)
 		}
-	}
-
-	fatalErrs := []error{}
-
-	for _, e := range planningErrs {
-		switch err := e.(type) {
-		case *state.ReleaseError:
-			if err.Code != 2 {
-				fatalErrs = append(fatalErrs, e)
-			}
-		default:
-			fatalErrs = append(fatalErrs, e)
-		}
-	}
-
-	if len(fatalErrs) > 0 {
-		return false, false, fatalErrs
-	}
-
-	releasesToBeDeleted := map[string]state.ReleaseSpec{}
-	for _, r := range deletingReleases {
-		id := state.ReleaseToID(&r)
-		releasesToBeDeleted[id] = r
-	}
-
-	releasesToBeUpdated := map[string]state.ReleaseSpec{}
-	for _, r := range changedReleases {
-		id := state.ReleaseToID(&r)
-
-		// If `helm-diff` detected changes but it is not being `helm delete`ed, we should run `helm upgrade`
-		if _, ok := releasesToBeDeleted[id]; !ok {
-			releasesToBeUpdated[id] = r
-		}
-	}
-
-	// sync only when there are changes
-	if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 {
-		// TODO better way to get the logger
-		logger := c.Logger()
-		logger.Infof("")
-		logger.Infof("No affected releases")
 		return true, false, nil
 	}
 
-	names := []string{}
-	for _, r := range releasesToBeUpdated {
-		names = append(names, fmt.Sprintf("  %s (%s) UPDATED", r.Name, r.Chart))
-	}
-	for _, r := range releasesToBeDeleted {
-		names = append(names, fmt.Sprintf("  %s (%s) DELETED", r.Name, r.Chart))
-	}
-	// Make the output deterministic for testing purpose
-	sort.Strings(names)
-
-	infoMsg := fmt.Sprintf(`Affected releases are:
-%s
-`, strings.Join(names, "\n"))
 	confMsg := fmt.Sprintf(`%s
 Do you really want to apply?
   Helmfile will apply all your changes, as shown above.
 
-`, infoMsg)
+`, *infoMsg)
 	interactive := c.Interactive()
 	if !interactive {
-		a.Logger.Debug(infoMsg)
+		a.Logger.Debug(*infoMsg)
 	}
 
 	syncErrs := []error{}

pkg/app/app_test.go

@@ -2524,6 +2524,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
 54: 
 
 merged environment: &{default map[] map[]}
+10 release(s) found in helmfile.yaml
+
 worker 1/1 started
 worker 1/1 finished
 worker 1/1 started
@@ -2711,6 +2713,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
 10: 
 
 merged environment: &{default map[] map[]}
+3 release(s) found in helmfile.yaml
+
 worker 1/1 started
 worker 1/1 finished
 worker 1/1 started
@@ -2989,6 +2993,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
 12: 
 
 merged environment: &{default map[] map[]}
+2 release(s) found in helmfile.yaml
+
 worker 1/1 started
 worker 1/1 finished
 worker 1/1 started
@@ -3335,6 +3341,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
 23: 
 
 merged environment: &{default map[] map[]}
+2 release(s) matching app=test found in helmfile.yaml
+
 worker 1/1 started
 worker 1/1 finished
 worker 1/1 started
@@ -3473,6 +3481,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
 23: 
 
 merged environment: &{default map[] map[]}
+0 release(s) matching app=test_non_existent found in helmfile.yaml
+
 `,
 		},
 		//
@@ -3535,6 +3545,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
  9: 
 
 merged environment: &{default map[] map[]}
+2 release(s) found in helmfile.yaml
+
 worker 1/1 started
 worker 1/1 finished
 worker 1/1 started

pkg/app/destroy_test.go

@@ -284,6 +284,8 @@ second-pass rendering result of "helmfile.yaml.part.0":
 54: 
 
 merged environment: &{default map[] map[]}
+10 release(s) found in helmfile.yaml
+
 processing 5 groups of releases in this order:
 GROUP RELEASES
 1     frontend-v3, frontend-v2, frontend-v1

pkg/app/run.go

@@ -1,9 +1,12 @@
 package app
 
 import (
+	"fmt"
 	"github.com/roboll/helmfile/pkg/argparser"
 	"github.com/roboll/helmfile/pkg/helmexec"
 	"github.com/roboll/helmfile/pkg/state"
+	"sort"
+	"strings"
 )
 
 type Run struct {
@@ -61,21 +64,36 @@ func (r *Run) Status(c StatusesConfigProvider) []error {
 	return r.state.ReleaseStatuses(r.helm, workers)
 }
 
-func (r *Run) Diff(c DiffConfigProvider) []error {
+func (r *Run) Diff(c DiffConfigProvider) (*string, bool, bool, []error) {
 	st := r.state
 	helm := r.helm
 	ctx := r.ctx
 
+	allReleases := st.GetReleasesWithOverrides()
+
+	toDiff, err := st.GetSelectedReleasesWithOverrides()
+	if err != nil {
+		return nil, false, false, []error{err}
+	}
+
+	if len(toDiff) == 0 {
+		return nil, false, false, nil
+	}
+
+	// Do build deps and prepare only on selected releases so that we won't waste time
+	// on running various helm commands on unnecessary releases
+	st.Releases = toDiff
+
 	if !c.SkipDeps() {
 		if errs := ctx.SyncReposOnce(st, helm); errs != nil && len(errs) > 0 {
-			return errs
+			return nil, false, false, errs
 		}
 		if errs := st.BuildDeps(helm); errs != nil && len(errs) > 0 {
-			return errs
+			return nil, false, false, errs
 		}
 	}
 	if errs := st.PrepareReleases(helm, "diff"); errs != nil && len(errs) > 0 {
-		return errs
+		return nil, false, false, errs
 	}
 
 	r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...)
@@ -85,9 +103,28 @@ func (r *Run) Diff(c DiffConfigProvider) []error {
 		NoColor: c.NoColor(),
 		Set:     c.Set(),
 	}
-	_, errs := st.DiffReleases(helm, c.Values(), c.Concurrency(), c.DetailedExitcode(), c.IncludeTests(), c.SuppressSecrets(), c.SuppressDiff(), true, opts)
 
-	return errs
+	// Validate all releases for missing `needs` targets
+	st.Releases = allReleases
+
+	if _, err := st.PlanReleases(false); err != nil {
+		return nil, false, false, []error{err}
+	}
+
+	// Diff only targeted releases
+
+	st.Releases = toDiff
+
+	filtered := &Run{
+		state: st,
+		helm:  r.helm,
+		ctx:   r.ctx,
+		Ask:   r.Ask,
+	}
+
+	infoMsg, updated, deleted, errs := filtered.diff(true, c.DetailedExitcode(), c, opts)
+
+	return infoMsg, true, len(deleted) > 0 || len(updated) > 0, errs
 }
 
 func (r *Run) Test(c TestConfigProvider) []error {
@@ -124,3 +161,83 @@ func (r *Run) Lint(c LintConfigProvider) []error {
 	}
 	return st.LintReleases(helm, values, args, workers, opts)
 }
+
+func (run *Run) diff(triggerCleanupEvent bool, detailedExitCode bool, c DiffConfigProvider, diffOpts *state.DiffOpts) (*string, map[string]state.ReleaseSpec, map[string]state.ReleaseSpec, []error) {
+	st := run.state
+	helm := run.helm
+
+	var changedReleases []state.ReleaseSpec
+	var deletingReleases []state.ReleaseSpec
+	var planningErrs []error
+
+	// TODO Better way to detect diff on only filtered releases
+	{
+		changedReleases, planningErrs = st.DiffReleases(helm, c.Values(), c.Concurrency(), detailedExitCode, c.IncludeTests(), c.SuppressSecrets(), c.SuppressDiff(), triggerCleanupEvent, diffOpts)
+
+		var err error
+		deletingReleases, err = st.DetectReleasesToBeDeletedForSync(helm, st.Releases)
+		if err != nil {
+			planningErrs = append(planningErrs, err)
+		}
+	}
+
+	fatalErrs := []error{}
+
+	for _, e := range planningErrs {
+		switch err := e.(type) {
+		case *state.ReleaseError:
+			if err.Code != 2 {
+				fatalErrs = append(fatalErrs, e)
+			}
+		default:
+			fatalErrs = append(fatalErrs, e)
+		}
+	}
+
+	if len(fatalErrs) > 0 {
+		return nil, nil, nil, fatalErrs
+	}
+
+	releasesToBeDeleted := map[string]state.ReleaseSpec{}
+	for _, r := range deletingReleases {
+		id := state.ReleaseToID(&r)
+		releasesToBeDeleted[id] = r
+	}
+
+	releasesToBeUpdated := map[string]state.ReleaseSpec{}
+	for _, r := range changedReleases {
+		id := state.ReleaseToID(&r)
+
+		// If `helm-diff` detected changes but it is not being `helm delete`ed, we should run `helm upgrade`
+		if _, ok := releasesToBeDeleted[id]; !ok {
+			releasesToBeUpdated[id] = r
+		}
+	}
+
+	// sync only when there are changes
+	if len(releasesToBeUpdated) == 0 && len(releasesToBeDeleted) == 0 {
+		var msg *string
+		if c.DetailedExitcode() {
+			// TODO better way to get the logger
+			m := "No affected releases"
+			msg = &m
+		}
+		return msg, nil, nil, nil
+	}
+
+	names := []string{}
+	for _, r := range releasesToBeUpdated {
+		names = append(names, fmt.Sprintf("  %s (%s) UPDATED", r.Name, r.Chart))
+	}
+	for _, r := range releasesToBeDeleted {
+		names = append(names, fmt.Sprintf("  %s (%s) DELETED", r.Name, r.Chart))
+	}
+	// Make the output deterministic for testing purpose
+	sort.Strings(names)
+
+	infoMsg := fmt.Sprintf(`Affected releases are:
+%s
+`, strings.Join(names, "\n"))
+
+	return &infoMsg, releasesToBeUpdated, releasesToBeDeleted, nil
+}

pkg/state/state.go

@@ -1327,6 +1327,15 @@ func (st *HelmState) GetSelectedReleasesWithOverrides() ([]ReleaseSpec, error) {
 			releases = append(releases, r.ReleaseSpec)
 		}
 	}
+
+	var extra string
+
+	if len(st.Selectors) > 0 {
+		extra = " matching " + strings.Join(st.Selectors, ",")
+	}
+
+	st.logger.Debugf("%d release(s)%s found in %s\n", len(releases), extra, st.FilePath)
+
 	return releases, nil
 }
 
@@ -1337,7 +1346,6 @@ func (st *HelmState) FilterReleases() error {
 		return err
 	}
 	st.Releases = releases
-	st.logger.Debugf("%d release(s) matching %s found in %s\n", len(releases), strings.Join(st.Selectors, ","), st.FilePath)
 	return nil
 }