From 4e5987d8337d44166bbd81e54c39ffa8cb2e4a47 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sat, 30 Apr 2022 14:36:38 +0200 Subject: [PATCH 01/19] Add preapply hook Signed-off-by: Anton Bretting --- go.sum | 2 ++ pkg/app/app.go | 32 +++++++++++++++++++++++++++++++- pkg/state/state.go | 4 ++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/go.sum b/go.sum index f30e358a..2f409b6d 100644 --- a/go.sum +++ b/go.sum @@ -1373,6 +1373,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= +golang.org/x/exp v0.0.0-20220428152302-39d4317da171 h1:TfdoLivD44QwvssI9Sv1xwa5DcL5XQr4au4sZ2F2NV4= +golang.org/x/exp v0.0.0-20220428152302-39d4317da171/go.mod h1:lgLbSvA5ygNOMpwM/9anMpWVlVJ7Z+cHWq/eFuinpGE= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= diff --git a/pkg/app/app.go b/pkg/app/app.go index 5c58717a..dddf0d21 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1323,7 +1323,31 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { } } - if releasesToBeDeleted == nil && releasesToBeUpdated == nil { + var releasesWithPreApply []state.ReleaseSpec + for _, r := range toApplyWithNeeds { + release := r + if len(release.Hooks) > 0 { + for _, hook := range release.Hooks { + if slices.Contains(hook.Events, "preapply") { + releasesWithPreApply = append(releasesWithPreApply, release) + } + } + } + } + + if len(releasesWithPreApply) > 0 { + msg := "Releases with preapply hooks: \n" + if infoMsg != nil { + msg = fmt.Sprintf("%s%s", *infoMsg, msg) + } + infoMsg = &msg + } + for _, release := range releasesWithPreApply { + tmp := fmt.Sprintf("%s %s (%s)", *infoMsg, release.Name, release.Chart) + infoMsg = &tmp + } + + if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesWithPreApply == nil { if infoMsg != nil { logger := c.Logger() logger.Infof("") @@ -1352,6 +1376,12 @@ Do you really want to apply? if !interactive || interactive && r.askForConfirmation(confMsg) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) + for _, release := range st.Releases { + if _, err := st.TriggerPreapplyEvent(&release, "apply"); err != nil { + syncErrs = append(syncErrs, err) + } + } + // We deleted releases by traversing the DAG in reverse order if len(releasesToBeDeleted) > 0 { _, deletionErrs := withDAG(st, helm, a.Logger, state.PlanOptions{Reverse: true, SelectedReleases: toDelete, SkipNeeds: true}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { diff --git a/pkg/state/state.go b/pkg/state/state.go index 385ab903..c133905c 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -2251,6 +2251,10 @@ func (st *HelmState) triggerPostsyncEvent(r *ReleaseSpec, evtErr error, helmfile return st.triggerReleaseEvent("postsync", evtErr, r, helmfileCommand) } +func (st *HelmState) TriggerPreapplyEvent(r *ReleaseSpec, helmfileCommand string) (bool, error) { + return st.triggerReleaseEvent("preapply", nil, r, helmfileCommand) +} + func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpec, helmfileCmd string) (bool, error) { bus := &event.Bus{ Hooks: r.Hooks, From 1a3c11dffde8380a36c627b0b6720dd8d791a32e Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sun, 1 May 2022 11:11:53 +0200 Subject: [PATCH 02/19] Add unittests for preapply Signed-off-by: Anton Bretting --- pkg/app/app.go | 3 +- pkg/app/app_apply_hooks_test.go | 236 ++++++++++++++++++ pkg/app/app_apply_test.go | 32 ++- .../apply_release_with_preapply_hook#01/log | 10 + .../apply_release_with_preapply_hook/log | 10 + pkg/event/bus.go | 2 +- 6 files changed, 290 insertions(+), 3 deletions(-) create mode 100644 pkg/app/app_apply_hooks_test.go create mode 100644 pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log create mode 100644 pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log diff --git a/pkg/app/app.go b/pkg/app/app.go index dddf0d21..4af49d56 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1376,7 +1376,8 @@ Do you really want to apply? if !interactive || interactive && r.askForConfirmation(confMsg) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) - for _, release := range st.Releases { + for _, release := range releasesWithPreApply { + a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&release, "apply"); err != nil { syncErrs = append(syncErrs, err) } diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go new file mode 100644 index 00000000..eeb74cbb --- /dev/null +++ b/pkg/app/app_apply_hooks_test.go @@ -0,0 +1,236 @@ +package app + +import ( + "bufio" + "bytes" + "io" + "path/filepath" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/roboll/helmfile/pkg/exectest" + "github.com/roboll/helmfile/pkg/helmexec" + "github.com/roboll/helmfile/pkg/testhelper" + "github.com/variantdev/vals" +) + +func TestApply_hooks(t *testing.T) { + type fields struct { + skipNeeds bool + includeNeeds bool + includeTransitiveNeeds bool + } + + type testcase struct { + fields fields + ns string + concurrency int + skipDiffOnInstall bool + error string + files map[string]string + selectors []string + lists map[exectest.ListKey]string + diffs map[exectest.DiffKey]error + upgraded []exectest.Release + deleted []exectest.Release + log string + logLevel string + } + + check := func(t *testing.T, tc testcase) { + t.Helper() + + wantUpgrades := tc.upgraded + wantDeletes := tc.deleted + + var helm = &exectest.Helm{ + FailOnUnexpectedList: true, + FailOnUnexpectedDiff: true, + Lists: tc.lists, + Diffs: tc.diffs, + DiffMutex: &sync.Mutex{}, + ChartsMutex: &sync.Mutex{}, + ReleasesMutex: &sync.Mutex{}, + } + + bs := &bytes.Buffer{} + + func() { + t.Helper() + + logReader, logWriter := io.Pipe() + + logFlushed := &sync.WaitGroup{} + // Ensure all the log is consumed into `bs` by calling `logWriter.Close()` followed by `logFlushed.Wait()` + logFlushed.Add(1) + go func() { + scanner := bufio.NewScanner(logReader) + for scanner.Scan() { + bs.Write(scanner.Bytes()) + bs.WriteString("\n") + } + logFlushed.Done() + }() + + defer func() { + // This is here to avoid data-trace on bytes buffer `bs` to capture logs + if err := logWriter.Close(); err != nil { + panic(err) + } + logFlushed.Wait() + }() + + logger := helmexec.NewLogger(logWriter, tc.logLevel) + + valsRuntime, err := vals.New(vals.Options{CacheSize: 32}) + if err != nil { + t.Errorf("unexpected error creating vals runtime: %v", err) + } + + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + glob: filepath.Glob, + abs: filepath.Abs, + OverrideKubeContext: "default", + Env: "default", + Logger: logger, + helms: map[helmKey]helmexec.Interface{ + createHelmKey("helm", "default"): helm, + }, + valsRuntime: valsRuntime, + }, tc.files) + + if tc.ns != "" { + app.Namespace = tc.ns + } + + if tc.selectors != nil { + app.Selectors = tc.selectors + } + + syncErr := app.Apply(applyConfig{ + // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. + concurrency: tc.concurrency, + logger: logger, + skipDiffOnInstall: tc.skipDiffOnInstall, + skipNeeds: tc.fields.skipNeeds, + includeNeeds: tc.fields.includeNeeds, + includeTransitiveNeeds: tc.fields.includeTransitiveNeeds, + }) + + var gotErr string + if syncErr != nil { + gotErr = syncErr.Error() + } + + if d := cmp.Diff(tc.error, gotErr); d != "" { + t.Fatalf("unexpected error: want (-), got (+): %s", d) + } + + if len(wantUpgrades) > len(helm.Releases) { + t.Fatalf("insufficient number of upgrades: got %d, want %d", len(helm.Releases), len(wantUpgrades)) + } + + for relIdx := range wantUpgrades { + if wantUpgrades[relIdx].Name != helm.Releases[relIdx].Name { + t.Errorf("releases[%d].name: got %q, want %q", relIdx, helm.Releases[relIdx].Name, wantUpgrades[relIdx].Name) + } + for flagIdx := range wantUpgrades[relIdx].Flags { + if wantUpgrades[relIdx].Flags[flagIdx] != helm.Releases[relIdx].Flags[flagIdx] { + t.Errorf("releases[%d].flags[%d]: got %v, want %v", relIdx, flagIdx, helm.Releases[relIdx].Flags[flagIdx], wantUpgrades[relIdx].Flags[flagIdx]) + } + } + } + + if len(wantDeletes) > len(helm.Deleted) { + t.Fatalf("insufficient number of deletes: got %d, want %d", len(helm.Deleted), len(wantDeletes)) + } + + for relIdx := range wantDeletes { + if wantDeletes[relIdx].Name != helm.Deleted[relIdx].Name { + t.Errorf("releases[%d].name: got %q, want %q", relIdx, helm.Deleted[relIdx].Name, wantDeletes[relIdx].Name) + } + for flagIdx := range wantDeletes[relIdx].Flags { + if wantDeletes[relIdx].Flags[flagIdx] != helm.Deleted[relIdx].Flags[flagIdx] { + t.Errorf("releaes[%d].flags[%d]: got %v, want %v", relIdx, flagIdx, helm.Deleted[relIdx].Flags[flagIdx], wantDeletes[relIdx].Flags[flagIdx]) + } + } + } + }() + + if tc.log != "" { + actual := bs.String() + + diff, exists := testhelper.Diff(tc.log, actual, 3) + if exists { + t.Errorf("unexpected log:\nDIFF\n%s\nEOD", diff) + } + } else { + assertEqualsToSnapshot(t, "log", bs.String()) + } + } + + t.Run("apply release with preapply hook", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["preapply"] + command: echo + showlogs: true + args: ["foo"] +`, + }, + selectors: []string{"name=foo"}, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + + t.Run("apply release with preapply hook", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["preapply", "presync"] + command: echo + showlogs: true + args: ["foo"] +`, + }, + selectors: []string{"name=foo"}, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + +} diff --git a/pkg/app/app_apply_test.go b/pkg/app/app_apply_test.go index b0f8401f..a3512801 100644 --- a/pkg/app/app_apply_test.go +++ b/pkg/app/app_apply_test.go @@ -137,7 +137,7 @@ func TestApply_2(t *testing.T) { } for flagIdx := range wantUpgrades[relIdx].Flags { if wantUpgrades[relIdx].Flags[flagIdx] != helm.Releases[relIdx].Flags[flagIdx] { - t.Errorf("releaes[%d].flags[%d]: got %v, want %v", relIdx, flagIdx, helm.Releases[relIdx].Flags[flagIdx], wantUpgrades[relIdx].Flags[flagIdx]) + t.Errorf("releases[%d].flags[%d]: got %v, want %v", relIdx, flagIdx, helm.Releases[relIdx].Flags[flagIdx], wantUpgrades[relIdx].Flags[flagIdx]) } } } @@ -1340,4 +1340,34 @@ foo 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default concurrency: 1, }) }) + + t.Run("apply release with preapply hook", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["preapply"] + command: echo + showlogs: true + args: ["foo"] +`, + }, + selectors: []string{"name=foo"}, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + }) + }) } diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log new file mode 100644 index 00000000..5c8158c0 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log @@ -0,0 +1,10 @@ + +Running preapply hook for foo: + +hook[preapply] logs | foo +hook[preapply] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw + diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log new file mode 100644 index 00000000..5c8158c0 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log @@ -0,0 +1,10 @@ + +Running preapply hook for foo: + +hook[preapply] logs | foo +hook[preapply] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw + diff --git a/pkg/event/bus.go b/pkg/event/bus.go index 92b69bff..b3448720 100644 --- a/pkg/event/bus.go +++ b/pkg/event/bus.go @@ -88,7 +88,7 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]interface{} } } - bus.Logger.Debugf("hook[%s]: stateFilePath=%s, basePath=%s\n", name, bus.StateFilePath, bus.BasePath) + bus.Logger.Debugf("hook[%s]: stateFilePath=%s, basePath=%s", name, bus.StateFilePath, bus.BasePath) data := map[string]interface{}{ "Environment": bus.Env, From db81f1809595184e2a92f6b5ca1b8203876b9b6a Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Mon, 2 May 2022 23:18:47 +0200 Subject: [PATCH 03/19] Only run preapply or presync Signed-off-by: Anton Bretting --- pkg/app/app.go | 2 + pkg/app/app_apply_hooks_test.go | 33 ++++++++- pkg/app/app_apply_test.go | 30 -------- .../apply_release_with_preapply_hook/log | 68 +++++++++++++++++++ .../apply_release_with_preapply_hook#01/log | 5 ++ .../apply_release_with_preapply_hook#02/log | 8 +++ pkg/event/bus.go | 7 ++ pkg/event/bus_test.go | 26 +++---- pkg/state/state.go | 32 ++++++++- 9 files changed, 166 insertions(+), 45 deletions(-) create mode 100644 pkg/app/testdata/testapply_2/apply_release_with_preapply_hook/log create mode 100644 pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#02/log diff --git a/pkg/app/app.go b/pkg/app/app.go index 4af49d56..69217e0b 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1330,6 +1330,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { for _, hook := range release.Hooks { if slices.Contains(hook.Events, "preapply") { releasesWithPreApply = append(releasesWithPreApply, release) + break } } } @@ -1380,6 +1381,7 @@ Do you really want to apply? a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&release, "apply"); err != nil { syncErrs = append(syncErrs, err) + continue } } diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index eeb74cbb..50af7e2a 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -213,7 +213,38 @@ releases: labels: app: test hooks: - - events: ["preapply", "presync"] + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["foo"] +`, + }, + selectors: []string{"name=foo"}, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + + t.Run("apply release with preapply hook", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["presync"] command: echo showlogs: true args: ["foo"] diff --git a/pkg/app/app_apply_test.go b/pkg/app/app_apply_test.go index a3512801..e1e07827 100644 --- a/pkg/app/app_apply_test.go +++ b/pkg/app/app_apply_test.go @@ -1340,34 +1340,4 @@ foo 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default concurrency: 1, }) }) - - t.Run("apply release with preapply hook", func(t *testing.T) { - check(t, testcase{ - files: map[string]string{ - "/path/to/helmfile.yaml": ` -releases: -- name: foo - chart: incubator/raw - namespace: default - labels: - app: test - hooks: - - events: ["preapply"] - command: echo - showlogs: true - args: ["foo"] -`, - }, - selectors: []string{"name=foo"}, - upgraded: []exectest.Release{ - {Name: "foo"}, - }, - diffs: map[exectest.DiffKey]error{ - {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, - }, - error: "", - // as we check for log output, set concurrency to 1 to avoid non-deterministic test result - concurrency: 1, - }) - }) } diff --git a/pkg/app/testdata/testapply_2/apply_release_with_preapply_hook/log b/pkg/app/testdata/testapply_2/apply_release_with_preapply_hook/log new file mode 100644 index 00000000..2961d200 --- /dev/null +++ b/pkg/app/testdata/testapply_2/apply_release_with_preapply_hook/log @@ -0,0 +1,68 @@ +processing file "helmfile.yaml" in directory "." +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: foo + 3: chart: incubator/raw + 4: namespace: default + 5: labels: + 6: app: test + 7: hooks: + 8: - events: ["preapply"] + 9: command: echo +10: showlogs: true +11: args: ["foo"] +12: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: foo + 3: chart: incubator/raw + 4: namespace: default + 5: labels: + 6: app: test + 7: hooks: + 8: - events: ["preapply"] + 9: command: echo +10: showlogs: true +11: args: ["foo"] +12: + +merged environment: &{default map[] map[]} +1 release(s) matching name=foo found in helmfile.yaml + +Affected releases are: + foo (incubator/raw) UPDATED +Releases with preapply hooks: + foo (incubator/raw) + +Running preapply hook for foo: +hook[echo]: stateFilePath=helmfile.yaml, basePath=. +hook[echo]: triggered by event "preapply" + +echo:XlBWj> foo +hook[echo]: foo + + + +hook[preapply] logs | foo +hook[preapply] logs | +processing 1 groups of releases in this order: +GROUP RELEASES +1 default/default/foo + +processing releases in group 1/1: default/default/foo +getting deployed release version failed:unexpected list key: {^foo$ --kube-contextdefault--deleting--deployed--failed--pending} + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw + diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log index 5c8158c0..cbffb336 100644 --- a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log @@ -1,8 +1,13 @@ +hook[prepare] logs | foo +hook[prepare] logs | + Running preapply hook for foo: hook[preapply] logs | foo hook[preapply] logs | +hook[echo]: already executed by "preapply", skipping "presync" + UPDATED RELEASES: NAME CHART VERSION diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#02/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#02/log new file mode 100644 index 00000000..8f9089e2 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#02/log @@ -0,0 +1,8 @@ + +hook[presync] logs | foo +hook[presync] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw + diff --git a/pkg/event/bus.go b/pkg/event/bus.go index b3448720..cd0c273a 100644 --- a/pkg/event/bus.go +++ b/pkg/event/bus.go @@ -10,6 +10,8 @@ import ( "github.com/helmfile/helmfile/pkg/filesystem" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/tmpl" + "go.uber.org/zap" + "golang.org/x/exp/slices" ) type Hook struct { @@ -19,6 +21,7 @@ type Hook struct { Kubectl map[string]string `yaml:"kubectlApply,omitempty"` Args []string `yaml:"args"` ShowLogs bool `yaml:"showlogs"` + Executed bool `yaml:"-"` } type event struct { @@ -71,6 +74,10 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]interface{} } } + if slices.Contains(hook.Events, "preapply") && evt == "presync" && hook.Executed { + bus.Logger.Infof("hook[%s]: already executed by \"preapply\", skipping \"%s\"\n", name, evt) + continue + } if hook.Kubectl != nil { if hook.Command != "" { bus.Logger.Warnf("warn: ignoring command '%s' given within a kubectlApply hook", hook.Command) diff --git a/pkg/event/bus_test.go b/pkg/event/bus_test.go index f2cc2d89..2352fad1 100644 --- a/pkg/event/bus_test.go +++ b/pkg/event/bus_test.go @@ -41,21 +41,21 @@ func TestTrigger(t *testing.T) { }{ { "okhook1", - &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, true}, + &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, true, false}, "foo", true, "", }, { "okhooké", - &Hook{"okhook2", []string{"foo"}, "ok", nil, []string{}, false}, + &Hook{"okhook2", []string{"foo"}, "ok", nil, []string{}, false, false}, "foo", true, "", }, { "missinghook1", - &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, false}, + &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, false, false}, "bar", false, "", @@ -69,70 +69,70 @@ func TestTrigger(t *testing.T) { }, { "nghook1", - &Hook{"nghook1", []string{"foo"}, "ng", nil, []string{}, false}, + &Hook{"nghook1", []string{"foo"}, "ng", nil, []string{}, false, false}, "foo", false, "hook[nghook1]: command `ng` failed: cmd failed due to invalid cmd: ng", }, { "nghook2", - &Hook{"nghook2", []string{"foo"}, "ok", nil, []string{"ng"}, false}, + &Hook{"nghook2", []string{"foo"}, "ok", nil, []string{"ng"}, false, false}, "foo", false, "hook[nghook2]: command `ok` failed: cmd failed due to invalid arg: ng", }, { "okkubeapply1", - &Hook{"okkubeapply1", []string{"foo"}, "", map[string]string{"kustomize": "kustodir"}, []string{}, false}, + &Hook{"okkubeapply1", []string{"foo"}, "", map[string]string{"kustomize": "kustodir"}, []string{}, false, false}, "foo", true, "", }, { "okkubeapply2", - &Hook{"okkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{}, false}, + &Hook{"okkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{}, false, false}, "foo", true, "", }, { "kokubeapply", - &Hook{"kokubeapply", []string{"foo"}, "", map[string]string{"kustomize": "kustodir", "filename": "resource.yaml"}, []string{}, true}, + &Hook{"kokubeapply", []string{"foo"}, "", map[string]string{"kustomize": "kustodir", "filename": "resource.yaml"}, []string{}, true, false}, "foo", false, "hook[kokubeapply]: kustomize & filename cannot be used together", }, { "kokubeapply2", - &Hook{"kokubeapply2", []string{"foo"}, "", map[string]string{}, []string{}, true}, + &Hook{"kokubeapply2", []string{"foo"}, "", map[string]string{}, []string{}, true, false}, "foo", false, "hook[kokubeapply2]: either kustomize or filename must be given", }, { "kokubeapply3", - &Hook{"", []string{"foo"}, "", map[string]string{}, []string{}, true}, + &Hook{"", []string{"foo"}, "", map[string]string{}, []string{}, true, false}, "foo", false, "hook[kubectlApply]: either kustomize or filename must be given", }, { "warnkubeapply1", - &Hook{"warnkubeapply1", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{}, true}, + &Hook{"warnkubeapply1", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{}, true, false}, "foo", true, "", }, { "warnkubeapply2", - &Hook{"warnkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true}, + &Hook{"warnkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true, false}, "foo", true, "", }, { "warnkubeapply3", - &Hook{"warnkubeapply3", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true}, + &Hook{"warnkubeapply3", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true, false}, "foo", true, "", diff --git a/pkg/state/state.go b/pkg/state/state.go index c133905c..48815202 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -18,6 +18,15 @@ import ( "text/template" "github.com/imdario/mergo" + "github.com/variantdev/chartify" + "golang.org/x/exp/slices" + + "github.com/helmfile/helmfile/pkg/environment" + "github.com/helmfile/helmfile/pkg/event" + "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/remote" + "github.com/helmfile/helmfile/pkg/tmpl" + "github.com/tatsushid/go-prettytable" "github.com/variantdev/chartify" "github.com/variantdev/vals" @@ -2256,6 +2265,16 @@ func (st *HelmState) TriggerPreapplyEvent(r *ReleaseSpec, helmfileCommand string } func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpec, helmfileCmd string) (bool, error) { + + var stateRelease ReleaseSpec + + for id, release := range st.Releases { + if release.Name == r.Name { + stateRelease = st.Releases[id] + break + } + } + bus := &event.Bus{ Hooks: r.Hooks, StateFilePath: st.FilePath, @@ -2272,7 +2291,18 @@ func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpe "Release": r, "HelmfileCommand": helmfileCmd, } - return bus.Trigger(evt, evtErr, data) + + executed, err := bus.Trigger(evt, evtErr, data) + + if executed { + for id, hook := range stateRelease.Hooks { + if slices.Contains(hook.Events, evt) { + stateRelease.Hooks[id].Executed = true + } + } + } + + return executed, err } // ResolveDeps returns a copy of this helmfile state with the concrete chart version numbers filled in for remote chart dependencies From 32477e6cf2cce8a7d95ff10b2bfae8357199555e Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Wed, 25 May 2022 19:44:27 +0200 Subject: [PATCH 04/19] Fix merge conflicts Signed-off-by: Anton Bretting --- go.sum | 4 ++-- pkg/app/app_apply_hooks_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.sum b/go.sum index 2f409b6d..bd6763de 100644 --- a/go.sum +++ b/go.sum @@ -1373,8 +1373,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= -golang.org/x/exp v0.0.0-20220428152302-39d4317da171 h1:TfdoLivD44QwvssI9Sv1xwa5DcL5XQr4au4sZ2F2NV4= -golang.org/x/exp v0.0.0-20220428152302-39d4317da171/go.mod h1:lgLbSvA5ygNOMpwM/9anMpWVlVJ7Z+cHWq/eFuinpGE= +golang.org/x/exp v0.0.0-20220518171630-0b5c67f07fdf h1:oXVg4h2qJDd9htKxb5SCpFBHLipW6hXmL3qpUixS2jw= +golang.org/x/exp v0.0.0-20220518171630-0b5c67f07fdf/go.mod h1:yh0Ynu2b5ZUe3MQfp2nM0ecK7wsgouWTDN0FNeJuIys= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index 50af7e2a..06413cf1 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -9,9 +9,9 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/roboll/helmfile/pkg/exectest" - "github.com/roboll/helmfile/pkg/helmexec" - "github.com/roboll/helmfile/pkg/testhelper" + "github.com/helmfile/helmfile/pkg/exectest" + "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/testhelper" "github.com/variantdev/vals" ) From c6e8afd3d0d3831f1c461e9d106861585e309dcd Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sat, 11 Jun 2022 13:51:56 +0200 Subject: [PATCH 05/19] Remove logic that limits execution of hooks to only once Signed-off-by: Anton Bretting --- .../apply_release_with_preapply_hook#01/log | 3 ++- pkg/event/bus.go | 6 ----- pkg/event/bus_test.go | 26 +++++++++---------- pkg/state/state.go | 22 +--------------- 4 files changed, 16 insertions(+), 41 deletions(-) diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log index cbffb336..bcc2fb64 100644 --- a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log @@ -6,8 +6,9 @@ Running preapply hook for foo: hook[preapply] logs | foo hook[preapply] logs | -hook[echo]: already executed by "preapply", skipping "presync" +hook[presync] logs | foo +hook[presync] logs | UPDATED RELEASES: NAME CHART VERSION diff --git a/pkg/event/bus.go b/pkg/event/bus.go index cd0c273a..fcf18a2b 100644 --- a/pkg/event/bus.go +++ b/pkg/event/bus.go @@ -11,7 +11,6 @@ import ( "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/tmpl" "go.uber.org/zap" - "golang.org/x/exp/slices" ) type Hook struct { @@ -21,7 +20,6 @@ type Hook struct { Kubectl map[string]string `yaml:"kubectlApply,omitempty"` Args []string `yaml:"args"` ShowLogs bool `yaml:"showlogs"` - Executed bool `yaml:"-"` } type event struct { @@ -74,10 +72,6 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]interface{} } } - if slices.Contains(hook.Events, "preapply") && evt == "presync" && hook.Executed { - bus.Logger.Infof("hook[%s]: already executed by \"preapply\", skipping \"%s\"\n", name, evt) - continue - } if hook.Kubectl != nil { if hook.Command != "" { bus.Logger.Warnf("warn: ignoring command '%s' given within a kubectlApply hook", hook.Command) diff --git a/pkg/event/bus_test.go b/pkg/event/bus_test.go index 2352fad1..f2cc2d89 100644 --- a/pkg/event/bus_test.go +++ b/pkg/event/bus_test.go @@ -41,21 +41,21 @@ func TestTrigger(t *testing.T) { }{ { "okhook1", - &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, true, false}, + &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, true}, "foo", true, "", }, { "okhooké", - &Hook{"okhook2", []string{"foo"}, "ok", nil, []string{}, false, false}, + &Hook{"okhook2", []string{"foo"}, "ok", nil, []string{}, false}, "foo", true, "", }, { "missinghook1", - &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, false, false}, + &Hook{"okhook1", []string{"foo"}, "ok", nil, []string{}, false}, "bar", false, "", @@ -69,70 +69,70 @@ func TestTrigger(t *testing.T) { }, { "nghook1", - &Hook{"nghook1", []string{"foo"}, "ng", nil, []string{}, false, false}, + &Hook{"nghook1", []string{"foo"}, "ng", nil, []string{}, false}, "foo", false, "hook[nghook1]: command `ng` failed: cmd failed due to invalid cmd: ng", }, { "nghook2", - &Hook{"nghook2", []string{"foo"}, "ok", nil, []string{"ng"}, false, false}, + &Hook{"nghook2", []string{"foo"}, "ok", nil, []string{"ng"}, false}, "foo", false, "hook[nghook2]: command `ok` failed: cmd failed due to invalid arg: ng", }, { "okkubeapply1", - &Hook{"okkubeapply1", []string{"foo"}, "", map[string]string{"kustomize": "kustodir"}, []string{}, false, false}, + &Hook{"okkubeapply1", []string{"foo"}, "", map[string]string{"kustomize": "kustodir"}, []string{}, false}, "foo", true, "", }, { "okkubeapply2", - &Hook{"okkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{}, false, false}, + &Hook{"okkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{}, false}, "foo", true, "", }, { "kokubeapply", - &Hook{"kokubeapply", []string{"foo"}, "", map[string]string{"kustomize": "kustodir", "filename": "resource.yaml"}, []string{}, true, false}, + &Hook{"kokubeapply", []string{"foo"}, "", map[string]string{"kustomize": "kustodir", "filename": "resource.yaml"}, []string{}, true}, "foo", false, "hook[kokubeapply]: kustomize & filename cannot be used together", }, { "kokubeapply2", - &Hook{"kokubeapply2", []string{"foo"}, "", map[string]string{}, []string{}, true, false}, + &Hook{"kokubeapply2", []string{"foo"}, "", map[string]string{}, []string{}, true}, "foo", false, "hook[kokubeapply2]: either kustomize or filename must be given", }, { "kokubeapply3", - &Hook{"", []string{"foo"}, "", map[string]string{}, []string{}, true, false}, + &Hook{"", []string{"foo"}, "", map[string]string{}, []string{}, true}, "foo", false, "hook[kubectlApply]: either kustomize or filename must be given", }, { "warnkubeapply1", - &Hook{"warnkubeapply1", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{}, true, false}, + &Hook{"warnkubeapply1", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{}, true}, "foo", true, "", }, { "warnkubeapply2", - &Hook{"warnkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true, false}, + &Hook{"warnkubeapply2", []string{"foo"}, "", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true}, "foo", true, "", }, { "warnkubeapply3", - &Hook{"warnkubeapply3", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true, false}, + &Hook{"warnkubeapply3", []string{"foo"}, "ok", map[string]string{"filename": "resource.yaml"}, []string{"ng"}, true}, "foo", true, "", diff --git a/pkg/state/state.go b/pkg/state/state.go index 48815202..e880bee9 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -19,7 +19,6 @@ import ( "github.com/imdario/mergo" "github.com/variantdev/chartify" - "golang.org/x/exp/slices" "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/event" @@ -2266,15 +2265,6 @@ func (st *HelmState) TriggerPreapplyEvent(r *ReleaseSpec, helmfileCommand string func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpec, helmfileCmd string) (bool, error) { - var stateRelease ReleaseSpec - - for id, release := range st.Releases { - if release.Name == r.Name { - stateRelease = st.Releases[id] - break - } - } - bus := &event.Bus{ Hooks: r.Hooks, StateFilePath: st.FilePath, @@ -2292,17 +2282,7 @@ func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpe "HelmfileCommand": helmfileCmd, } - executed, err := bus.Trigger(evt, evtErr, data) - - if executed { - for id, hook := range stateRelease.Hooks { - if slices.Contains(hook.Events, evt) { - stateRelease.Hooks[id].Executed = true - } - } - } - - return executed, err + return bus.Trigger(evt, evtErr, data) } // ResolveDeps returns a copy of this helmfile state with the concrete chart version numbers filled in for remote chart dependencies From eb2419aa153f7b32df62f11a1f5530a1dfbac93a Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sat, 11 Jun 2022 17:24:13 +0200 Subject: [PATCH 06/19] Remove unnecessary if statement Signed-off-by: Anton Bretting --- pkg/app/app.go | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 69217e0b..4abb87ff 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1326,27 +1326,25 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { var releasesWithPreApply []state.ReleaseSpec for _, r := range toApplyWithNeeds { release := r - if len(release.Hooks) > 0 { - for _, hook := range release.Hooks { - if slices.Contains(hook.Events, "preapply") { - releasesWithPreApply = append(releasesWithPreApply, release) - break - } + for _, hook := range release.Hooks { + if slices.Contains(hook.Events, "preapply") { + releasesWithPreApply = append(releasesWithPreApply, release) + break } } } - if len(releasesWithPreApply) > 0 { - msg := "Releases with preapply hooks: \n" - if infoMsg != nil { - msg = fmt.Sprintf("%s%s", *infoMsg, msg) - } - infoMsg = &msg - } - for _, release := range releasesWithPreApply { - tmp := fmt.Sprintf("%s %s (%s)", *infoMsg, release.Name, release.Chart) - infoMsg = &tmp - } + // if len(releasesWithPreApply) > 0 { + // msg := "Releases with preapply hooks: \n" + // if infoMsg != nil { + // msg = fmt.Sprintf("%s%s", *infoMsg, msg) + // } + // infoMsg = &msg + // } + // for _, release := range releasesWithPreApply { + // tmp := fmt.Sprintf("%s %s (%s)", *infoMsg, release.Name, release.Chart) + // infoMsg = &tmp + // } if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesWithPreApply == nil { if infoMsg != nil { From ca7942c7513a4180c6ac4dd19fa7717350df0800 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sun, 12 Jun 2022 09:17:03 +0200 Subject: [PATCH 07/19] Uncomment code that was accidentally commented out Signed-off-by: Anton Bretting --- pkg/app/app.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 4abb87ff..d4417fcb 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1334,17 +1334,17 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { } } - // if len(releasesWithPreApply) > 0 { - // msg := "Releases with preapply hooks: \n" - // if infoMsg != nil { - // msg = fmt.Sprintf("%s%s", *infoMsg, msg) - // } - // infoMsg = &msg - // } - // for _, release := range releasesWithPreApply { - // tmp := fmt.Sprintf("%s %s (%s)", *infoMsg, release.Name, release.Chart) - // infoMsg = &tmp - // } + if len(releasesWithPreApply) > 0 { + msg := "Releases with preapply hooks: \n" + if infoMsg != nil { + msg = fmt.Sprintf("%s%s", *infoMsg, msg) + } + infoMsg = &msg + } + for _, release := range releasesWithPreApply { + tmp := fmt.Sprintf("%s %s (%s)", *infoMsg, release.Name, release.Chart) + infoMsg = &tmp + } if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesWithPreApply == nil { if infoMsg != nil { From ea5606160981da63dab59c940daf50506d5cbc56 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sun, 12 Jun 2022 11:54:31 +0200 Subject: [PATCH 08/19] Move preapply code to separate function Signed-off-by: Anton Bretting --- pkg/app/app.go | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index d4417fcb..7e5cbc1d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -790,7 +790,7 @@ func (a *App) visitStates(fileOrDir string, defOpts LoadOpts, converge func(*sta go func() { sig := <-sigs - errs := []error{fmt.Errorf("Received [%s] to shutdown ", sig)} + errs := []error{fmt.Errorf("received [%s] to shutdown ", sig)} _ = context{app: a, st: st, retainValues: defOpts.RetainValuesFiles}.clean(errs) // See http://tldp.org/LDP/abs/html/exitcodes.html switch sig { @@ -1323,28 +1323,9 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { } } - var releasesWithPreApply []state.ReleaseSpec - for _, r := range toApplyWithNeeds { - release := r - for _, hook := range release.Hooks { - if slices.Contains(hook.Events, "preapply") { - releasesWithPreApply = append(releasesWithPreApply, release) - break - } - } - } + releasesWithPreApply := getReleasesWithPreApply(toApplyWithNeeds) - if len(releasesWithPreApply) > 0 { - msg := "Releases with preapply hooks: \n" - if infoMsg != nil { - msg = fmt.Sprintf("%s%s", *infoMsg, msg) - } - infoMsg = &msg - } - for _, release := range releasesWithPreApply { - tmp := fmt.Sprintf("%s %s (%s)", *infoMsg, release.Name, release.Chart) - infoMsg = &tmp - } + infoMsg = preApplyInfoMsg(releasesWithPreApply, infoMsg) if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesWithPreApply == nil { if infoMsg != nil { From fcdd8521532bd5227251656ea6441682f8ed0ca5 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sun, 12 Jun 2022 12:56:34 +0200 Subject: [PATCH 09/19] Add unittests for new preapply functions Signed-off-by: Anton Bretting --- pkg/app/app_apply_hooks_test.go | 177 +++++++++++++++++++++++++++++++- 1 file changed, 176 insertions(+), 1 deletion(-) diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index 06413cf1..47083a55 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -9,10 +9,14 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/helmfile/helmfile/pkg/event" "github.com/helmfile/helmfile/pkg/exectest" "github.com/helmfile/helmfile/pkg/helmexec" + "github.com/helmfile/helmfile/pkg/state" "github.com/helmfile/helmfile/pkg/testhelper" + "github.com/stretchr/testify/require" "github.com/variantdev/vals" + "k8s.io/utils/pointer" ) func TestApply_hooks(t *testing.T) { @@ -263,5 +267,176 @@ releases: logLevel: "info", }) }) - +} + +func TestGetReleasesWithPreApply(t *testing.T) { + tests := []struct { + releases []state.ReleaseSpec + preApplyreleases []state.ReleaseSpec + }{ + { + []state.ReleaseSpec{ + { + Chart: "foo/bar", + Name: "foobar", + Namespace: "foobar", + Hooks: []event.Hook{ + { + Events: []string{ + "prepare", + "preapply", + }, + }, + }, + }, + { + Chart: "foo/foo", + Name: "foo", + Namespace: "foo", + Hooks: []event.Hook{ + { + Events: []string{ + "prepare", + "presync", + }, + }, + }, + }, + { + Chart: "bar/bar", + Name: "bar", + Namespace: "bar", + Hooks: []event.Hook{ + { + Events: []string{ + "preapply", + }, + }, + }, + }, + }, + []state.ReleaseSpec{ + { + Chart: "foo/bar", + Name: "foobar", + Namespace: "foobar", + Hooks: []event.Hook{ + { + Events: []string{ + "prepare", + "preapply", + }, + }, + }, + }, + { + Chart: "bar/bar", + Name: "bar", + Namespace: "bar", + Hooks: []event.Hook{ + { + Events: []string{ + "preapply", + }, + }, + }, + }, + }, + }, + } + + for _, test := range tests { + preApply := getReleasesWithPreApply(test.releases) + require.Equal(t, test.preApplyreleases, preApply) + } +} + +func TestPreApplyInfoMsg(t *testing.T) { + tests := []struct { + preApplyreleases []state.ReleaseSpec + infoMsg *string + expected *string + }{ + { + []state.ReleaseSpec{ + { + Chart: "foo/bar", + Name: "foobar", + Namespace: "foobar", + Hooks: []event.Hook{ + { + Events: []string{ + "prepare", + "preapply", + }, + }, + }, + }, + { + Chart: "bar/bar", + Name: "bar", + Namespace: "bar", + Hooks: []event.Hook{ + { + Events: []string{ + "preapply", + }, + }, + }, + }, + }, + pointer.String("infoMsg\n"), + pointer.String(`infoMsg + +Releases with preapply hooks: + foobar (foo/bar) + bar (bar/bar) +`), + }, + { + []state.ReleaseSpec{ + { + Chart: "foo/bar", + Name: "foobar", + Namespace: "foobar", + Hooks: []event.Hook{ + { + Events: []string{ + "prepare", + "preapply", + }, + }, + }, + }, + { + Chart: "bar/bar", + Name: "bar", + Namespace: "bar", + Hooks: []event.Hook{ + { + Events: []string{ + "preapply", + }, + }, + }, + }, + }, + pointer.String(""), + pointer.String(` +Releases with preapply hooks: + foobar (foo/bar) + bar (bar/bar) +`), + }, + { + []state.ReleaseSpec{}, + pointer.String(""), + pointer.String(""), + }, + } + + for _, test := range tests { + infoMsg := preApplyInfoMsg(test.preApplyreleases, test.infoMsg) + require.Equal(t, *test.expected, *infoMsg) + } } From f0b76e9e261e6e2ffa4e95a413f5c178f2c51185 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Wed, 14 Sep 2022 19:11:44 +0200 Subject: [PATCH 10/19] Fixes for updates from rebase Signed-off-by: Anton Bretting --- go.mod | 3 ++- go.sum | 4 ++-- pkg/app/app.go | 32 +++++++++++++++++++++++++++++++- pkg/app/app_apply_hooks_test.go | 3 --- pkg/event/bus.go | 3 +-- pkg/state/state.go | 9 +-------- 6 files changed, 37 insertions(+), 17 deletions(-) diff --git a/go.mod b/go.mod index d22ee5d1..24dc19cd 100644 --- a/go.mod +++ b/go.mod @@ -29,12 +29,14 @@ require ( github.com/variantdev/vals v0.18.0 go.uber.org/multierr v1.6.0 go.uber.org/zap v1.23.0 + golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 gopkg.in/yaml.v2 v2.4.0 gotest.tools v2.2.0+incompatible helm.sh/helm/v3 v3.8.1 k8s.io/apimachinery v0.24.4 + k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 ) require ( @@ -194,7 +196,6 @@ require ( k8s.io/client-go v0.23.4 // indirect k8s.io/klog/v2 v2.60.1 // indirect k8s.io/kube-openapi v0.0.0-20220328201542-3ee0da9b0b42 // indirect - k8s.io/utils v0.0.0-20220210201930-3a6ce19ff2f9 // indirect oras.land/oras-go v1.1.0 // indirect sigs.k8s.io/kustomize/api v0.10.1 // indirect sigs.k8s.io/kustomize/kyaml v0.13.0 // indirect diff --git a/go.sum b/go.sum index bd6763de..e32fb81f 100644 --- a/go.sum +++ b/go.sum @@ -1373,8 +1373,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= -golang.org/x/exp v0.0.0-20220518171630-0b5c67f07fdf h1:oXVg4h2qJDd9htKxb5SCpFBHLipW6hXmL3qpUixS2jw= -golang.org/x/exp v0.0.0-20220518171630-0b5c67f07fdf/go.mod h1:yh0Ynu2b5ZUe3MQfp2nM0ecK7wsgouWTDN0FNeJuIys= +golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561 h1:MDc5xs78ZrZr3HMQugiXOAkSZtfTpbJLDr/lwfgO53E= +golang.org/x/exp v0.0.0-20220909182711-5c715a9e8561/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= diff --git a/pkg/app/app.go b/pkg/app/app.go index 7e5cbc1d..d5c4e7a3 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -15,6 +15,7 @@ import ( "github.com/variantdev/vals" "go.uber.org/zap" + "golang.org/x/exp/slices" "github.com/helmfile/helmfile/pkg/argparser" "github.com/helmfile/helmfile/pkg/filesystem" @@ -1359,7 +1360,7 @@ Do you really want to apply? for _, release := range releasesWithPreApply { a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&release, "apply"); err != nil { - syncErrs = append(syncErrs, err) + applyErrs = append(applyErrs, err) continue } } @@ -1420,6 +1421,35 @@ Do you really want to apply? return true, true, applyErrs } +func getReleasesWithPreApply(releases []state.ReleaseSpec) []state.ReleaseSpec { + var releasesWithPreApply []state.ReleaseSpec + for _, r := range releases { + release := r + for _, hook := range release.Hooks { + if slices.Contains(hook.Events, "preapply") { + releasesWithPreApply = append(releasesWithPreApply, release) + break + } + } + } + return releasesWithPreApply +} + +func preApplyInfoMsg(releasesWithPreApply []state.ReleaseSpec, infoMsg *string) *string { + if len(releasesWithPreApply) > 0 { + msg := "Releases with preapply hooks: \n" + if infoMsg != nil { + msg = fmt.Sprintf("%s\n%s", *infoMsg, msg) + } + infoMsg = &msg + } + for _, release := range releasesWithPreApply { + tmp := fmt.Sprintf("%s %s (%s)\n", *infoMsg, release.Name, release.Chart) + infoMsg = &tmp + } + return infoMsg +} + func (a *App) delete(r *Run, purge bool, c DestroyConfigProvider) (bool, []error) { st := r.state helm := r.helm diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index 47083a55..bb6875d0 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -4,7 +4,6 @@ import ( "bufio" "bytes" "io" - "path/filepath" "sync" "testing" @@ -94,8 +93,6 @@ func TestApply_hooks(t *testing.T) { app := appWithFs(&App{ OverrideHelmBinary: DefaultHelmBinary, - glob: filepath.Glob, - abs: filepath.Abs, OverrideKubeContext: "default", Env: "default", Logger: logger, diff --git a/pkg/event/bus.go b/pkg/event/bus.go index fcf18a2b..92b69bff 100644 --- a/pkg/event/bus.go +++ b/pkg/event/bus.go @@ -10,7 +10,6 @@ import ( "github.com/helmfile/helmfile/pkg/filesystem" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/tmpl" - "go.uber.org/zap" ) type Hook struct { @@ -89,7 +88,7 @@ func (bus *Bus) Trigger(evt string, evtErr error, context map[string]interface{} } } - bus.Logger.Debugf("hook[%s]: stateFilePath=%s, basePath=%s", name, bus.StateFilePath, bus.BasePath) + bus.Logger.Debugf("hook[%s]: stateFilePath=%s, basePath=%s\n", name, bus.StateFilePath, bus.BasePath) data := map[string]interface{}{ "Environment": bus.Env, diff --git a/pkg/state/state.go b/pkg/state/state.go index e880bee9..dcbb8460 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -18,10 +18,10 @@ import ( "text/template" "github.com/imdario/mergo" - "github.com/variantdev/chartify" "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/event" + "github.com/helmfile/helmfile/pkg/filesystem" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/remote" "github.com/helmfile/helmfile/pkg/tmpl" @@ -31,13 +31,6 @@ import ( "github.com/variantdev/vals" "go.uber.org/zap" "gopkg.in/yaml.v2" - - "github.com/helmfile/helmfile/pkg/environment" - "github.com/helmfile/helmfile/pkg/event" - "github.com/helmfile/helmfile/pkg/filesystem" - "github.com/helmfile/helmfile/pkg/helmexec" - "github.com/helmfile/helmfile/pkg/remote" - "github.com/helmfile/helmfile/pkg/tmpl" ) const ( From 1e9cce7d3643fd8339ad015fdf689e4ede93010e Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sat, 17 Sep 2022 10:52:51 +0200 Subject: [PATCH 11/19] Fix lint errors Signed-off-by: Anton Bretting --- pkg/app/app_apply_hooks_test.go | 7 ++++--- pkg/state/state.go | 12 +++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index bb6875d0..abd57676 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -8,14 +8,15 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + "github.com/variantdev/vals" + "k8s.io/utils/pointer" + "github.com/helmfile/helmfile/pkg/event" "github.com/helmfile/helmfile/pkg/exectest" "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/state" "github.com/helmfile/helmfile/pkg/testhelper" - "github.com/stretchr/testify/require" - "github.com/variantdev/vals" - "k8s.io/utils/pointer" ) func TestApply_hooks(t *testing.T) { diff --git a/pkg/state/state.go b/pkg/state/state.go index dcbb8460..97d921ae 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -18,6 +18,11 @@ import ( "text/template" "github.com/imdario/mergo" + "github.com/tatsushid/go-prettytable" + "github.com/variantdev/chartify" + "github.com/variantdev/vals" + "go.uber.org/zap" + "gopkg.in/yaml.v2" "github.com/helmfile/helmfile/pkg/environment" "github.com/helmfile/helmfile/pkg/event" @@ -25,12 +30,6 @@ import ( "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/remote" "github.com/helmfile/helmfile/pkg/tmpl" - - "github.com/tatsushid/go-prettytable" - "github.com/variantdev/chartify" - "github.com/variantdev/vals" - "go.uber.org/zap" - "gopkg.in/yaml.v2" ) const ( @@ -2257,7 +2256,6 @@ func (st *HelmState) TriggerPreapplyEvent(r *ReleaseSpec, helmfileCommand string } func (st *HelmState) triggerReleaseEvent(evt string, evtErr error, r *ReleaseSpec, helmfileCmd string) (bool, error) { - bus := &event.Bus{ Hooks: r.Hooks, StateFilePath: st.FilePath, From 5b88006e868313a36aca235a64f7cde47ffddc37 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sat, 17 Sep 2022 12:03:32 +0200 Subject: [PATCH 12/19] Update documentation Signed-off-by: Anton Bretting --- docs/index.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/index.md b/docs/index.md index 72fd97bb..2ed5226c 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1183,6 +1183,7 @@ Once `events` are triggered, associated `hooks` are executed, by running the `co Currently supported `events` are: - `prepare` +- `preapply` - `presync` - `preuninstall` - `postuninstall` From 3c0456c577b7e13fe3e2e52075bb108312594835 Mon Sep 17 00:00:00 2001 From: Anton Bretting Date: Sun, 18 Sep 2022 09:42:15 +0200 Subject: [PATCH 13/19] Updates based on review comments Signed-off-by: Anton Bretting --- pkg/app/app.go | 40 +--- pkg/app/app_apply_hooks_test.go | 176 ------------------ .../apply_release_with_preapply_hook#01/log | 2 - .../apply_release_with_preapply_hook/log | 2 - 4 files changed, 3 insertions(+), 217 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index d5c4e7a3..b02e9370 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -15,7 +15,6 @@ import ( "github.com/variantdev/vals" "go.uber.org/zap" - "golang.org/x/exp/slices" "github.com/helmfile/helmfile/pkg/argparser" "github.com/helmfile/helmfile/pkg/filesystem" @@ -1324,11 +1323,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { } } - releasesWithPreApply := getReleasesWithPreApply(toApplyWithNeeds) - - infoMsg = preApplyInfoMsg(releasesWithPreApply, infoMsg) - - if releasesToBeDeleted == nil && releasesToBeUpdated == nil && releasesWithPreApply == nil { + if releasesToBeDeleted == nil && releasesToBeUpdated == nil { if infoMsg != nil { logger := c.Logger() logger.Infof("") @@ -1357,8 +1352,8 @@ Do you really want to apply? if !interactive || interactive && r.askForConfirmation(confMsg) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) - for _, release := range releasesWithPreApply { - a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) + for _, release := range st.Releases { + // a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&release, "apply"); err != nil { applyErrs = append(applyErrs, err) continue @@ -1421,35 +1416,6 @@ Do you really want to apply? return true, true, applyErrs } -func getReleasesWithPreApply(releases []state.ReleaseSpec) []state.ReleaseSpec { - var releasesWithPreApply []state.ReleaseSpec - for _, r := range releases { - release := r - for _, hook := range release.Hooks { - if slices.Contains(hook.Events, "preapply") { - releasesWithPreApply = append(releasesWithPreApply, release) - break - } - } - } - return releasesWithPreApply -} - -func preApplyInfoMsg(releasesWithPreApply []state.ReleaseSpec, infoMsg *string) *string { - if len(releasesWithPreApply) > 0 { - msg := "Releases with preapply hooks: \n" - if infoMsg != nil { - msg = fmt.Sprintf("%s\n%s", *infoMsg, msg) - } - infoMsg = &msg - } - for _, release := range releasesWithPreApply { - tmp := fmt.Sprintf("%s %s (%s)\n", *infoMsg, release.Name, release.Chart) - infoMsg = &tmp - } - return infoMsg -} - func (a *App) delete(r *Run, purge bool, c DestroyConfigProvider) (bool, []error) { st := r.state helm := r.helm diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index abd57676..ad880715 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -8,14 +8,10 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/stretchr/testify/require" "github.com/variantdev/vals" - "k8s.io/utils/pointer" - "github.com/helmfile/helmfile/pkg/event" "github.com/helmfile/helmfile/pkg/exectest" "github.com/helmfile/helmfile/pkg/helmexec" - "github.com/helmfile/helmfile/pkg/state" "github.com/helmfile/helmfile/pkg/testhelper" ) @@ -266,175 +262,3 @@ releases: }) }) } - -func TestGetReleasesWithPreApply(t *testing.T) { - tests := []struct { - releases []state.ReleaseSpec - preApplyreleases []state.ReleaseSpec - }{ - { - []state.ReleaseSpec{ - { - Chart: "foo/bar", - Name: "foobar", - Namespace: "foobar", - Hooks: []event.Hook{ - { - Events: []string{ - "prepare", - "preapply", - }, - }, - }, - }, - { - Chart: "foo/foo", - Name: "foo", - Namespace: "foo", - Hooks: []event.Hook{ - { - Events: []string{ - "prepare", - "presync", - }, - }, - }, - }, - { - Chart: "bar/bar", - Name: "bar", - Namespace: "bar", - Hooks: []event.Hook{ - { - Events: []string{ - "preapply", - }, - }, - }, - }, - }, - []state.ReleaseSpec{ - { - Chart: "foo/bar", - Name: "foobar", - Namespace: "foobar", - Hooks: []event.Hook{ - { - Events: []string{ - "prepare", - "preapply", - }, - }, - }, - }, - { - Chart: "bar/bar", - Name: "bar", - Namespace: "bar", - Hooks: []event.Hook{ - { - Events: []string{ - "preapply", - }, - }, - }, - }, - }, - }, - } - - for _, test := range tests { - preApply := getReleasesWithPreApply(test.releases) - require.Equal(t, test.preApplyreleases, preApply) - } -} - -func TestPreApplyInfoMsg(t *testing.T) { - tests := []struct { - preApplyreleases []state.ReleaseSpec - infoMsg *string - expected *string - }{ - { - []state.ReleaseSpec{ - { - Chart: "foo/bar", - Name: "foobar", - Namespace: "foobar", - Hooks: []event.Hook{ - { - Events: []string{ - "prepare", - "preapply", - }, - }, - }, - }, - { - Chart: "bar/bar", - Name: "bar", - Namespace: "bar", - Hooks: []event.Hook{ - { - Events: []string{ - "preapply", - }, - }, - }, - }, - }, - pointer.String("infoMsg\n"), - pointer.String(`infoMsg - -Releases with preapply hooks: - foobar (foo/bar) - bar (bar/bar) -`), - }, - { - []state.ReleaseSpec{ - { - Chart: "foo/bar", - Name: "foobar", - Namespace: "foobar", - Hooks: []event.Hook{ - { - Events: []string{ - "prepare", - "preapply", - }, - }, - }, - }, - { - Chart: "bar/bar", - Name: "bar", - Namespace: "bar", - Hooks: []event.Hook{ - { - Events: []string{ - "preapply", - }, - }, - }, - }, - }, - pointer.String(""), - pointer.String(` -Releases with preapply hooks: - foobar (foo/bar) - bar (bar/bar) -`), - }, - { - []state.ReleaseSpec{}, - pointer.String(""), - pointer.String(""), - }, - } - - for _, test := range tests { - infoMsg := preApplyInfoMsg(test.preApplyreleases, test.infoMsg) - require.Equal(t, *test.expected, *infoMsg) - } -} diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log index bcc2fb64..0c6b5308 100644 --- a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook#01/log @@ -2,8 +2,6 @@ hook[prepare] logs | foo hook[prepare] logs | -Running preapply hook for foo: - hook[preapply] logs | foo hook[preapply] logs | diff --git a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log index 5c8158c0..e6233234 100644 --- a/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log +++ b/pkg/app/testdata/testapply_hooks/apply_release_with_preapply_hook/log @@ -1,6 +1,4 @@ -Running preapply hook for foo: - hook[preapply] logs | foo hook[preapply] logs | From 5b1606df7530401863d676852a605b782505eaf6 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sun, 18 Sep 2022 23:53:50 +0000 Subject: [PATCH 14/19] fixup! Updates based on review comments Signed-off-by: Yusuke Kuoka --- pkg/app/app.go | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index b02e9370..8708514d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1342,7 +1342,7 @@ Do you really want to apply? a.Logger.Debug(*infoMsg) } - applyErrs := []error{} + var applyErrs []error affectedReleases := state.AffectedReleases{} @@ -1352,26 +1352,31 @@ Do you really want to apply? if !interactive || interactive && r.askForConfirmation(confMsg) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) - for _, release := range st.Releases { - // a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) - if _, err := st.TriggerPreapplyEvent(&release, "apply"); err != nil { - applyErrs = append(applyErrs, err) - continue - } - } - // We deleted releases by traversing the DAG in reverse order if len(releasesToBeDeleted) > 0 { _, deletionErrs := withDAG(st, helm, a.Logger, state.PlanOptions{Reverse: true, SelectedReleases: toDelete, SkipNeeds: true}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { - var rs []state.ReleaseSpec + var ( + rs []state.ReleaseSpec + preapplyErrors []error + ) for _, r := range subst.Releases { release := r if r2, ok := releasesToBeDeleted[state.ReleaseToID(&release)]; ok { + a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) + if _, err := st.TriggerPreapplyEvent(&r2, "apply"); err != nil { + preapplyErrors = append(applyErrs, err) + continue + } + rs = append(rs, r2) } } + if len(preapplyErrors) > 0 { + return preapplyErrors + } + subst.Releases = rs return subst.DeleteReleasesForSync(&affectedReleases, helm, c.Concurrency()) @@ -1385,15 +1390,28 @@ Do you really want to apply? // We upgrade releases by traversing the DAG if len(releasesToBeUpdated) > 0 { _, updateErrs := withDAG(st, helm, a.Logger, state.PlanOptions{SelectedReleases: toUpdate, Reverse: false, SkipNeeds: true, IncludeTransitiveNeeds: c.IncludeTransitiveNeeds()}, a.WrapWithoutSelector(func(subst *state.HelmState, helm helmexec.Interface) []error { - var rs []state.ReleaseSpec + var ( + rs []state.ReleaseSpec + preapplyErrors []error + ) for _, r := range subst.Releases { release := r if r2, ok := releasesToBeUpdated[state.ReleaseToID(&release)]; ok { + a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) + if _, err := st.TriggerPreapplyEvent(&r2, "apply"); err != nil { + preapplyErrors = append(applyErrs, err) + continue + } + rs = append(rs, r2) } } + if len(preapplyErrors) > 0 { + return preapplyErrors + } + subst.Releases = rs syncOpts := state.SyncOpts{ From 9e673ca90217ab3eb02a2f4fc40250e4f8b2749b Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 19 Sep 2022 00:25:59 +0000 Subject: [PATCH 15/19] Write preapply notice to debug log instead of info Signed-off-by: Yusuke Kuoka --- pkg/app/app.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 8708514d..1ce6f5c5 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1363,7 +1363,7 @@ Do you really want to apply? for _, r := range subst.Releases { release := r if r2, ok := releasesToBeDeleted[state.ReleaseToID(&release)]; ok { - a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) + a.Logger.Debugf("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&r2, "apply"); err != nil { preapplyErrors = append(applyErrs, err) continue @@ -1398,7 +1398,7 @@ Do you really want to apply? for _, r := range subst.Releases { release := r if r2, ok := releasesToBeUpdated[state.ReleaseToID(&release)]; ok { - a.Logger.Infof("\nRunning preapply hook for %s:", release.Name) + a.Logger.Debugf("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&r2, "apply"); err != nil { preapplyErrors = append(applyErrs, err) continue From 793050cc18b2217c4b9b718d3a38316a29083cc2 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 19 Sep 2022 00:44:31 +0000 Subject: [PATCH 16/19] Defer implementing preapply hook start logs to another commit/pr Signed-off-by: Yusuke Kuoka --- pkg/app/app.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 1ce6f5c5..e4141468 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1363,7 +1363,6 @@ Do you really want to apply? for _, r := range subst.Releases { release := r if r2, ok := releasesToBeDeleted[state.ReleaseToID(&release)]; ok { - a.Logger.Debugf("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&r2, "apply"); err != nil { preapplyErrors = append(applyErrs, err) continue @@ -1398,7 +1397,6 @@ Do you really want to apply? for _, r := range subst.Releases { release := r if r2, ok := releasesToBeUpdated[state.ReleaseToID(&release)]; ok { - a.Logger.Debugf("\nRunning preapply hook for %s:", release.Name) if _, err := st.TriggerPreapplyEvent(&r2, "apply"); err != nil { preapplyErrors = append(applyErrs, err) continue From dc40ccde2e0e4c19301fa1e2da055e8a544103db Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 19 Sep 2022 02:19:42 +0000 Subject: [PATCH 17/19] Add more testcases for hooks Signed-off-by: Yusuke Kuoka --- pkg/app/app_apply_hooks_test.go | 221 ++++++++++++++++++ .../log | 14 ++ .../hooks_are_not_run_on_disabled_release/log | 14 ++ .../hooks_are_run_on_enabled_release/log | 24 ++ .../log | 24 ++ .../hooks_for_no-diff_release/log | 6 + pkg/exectest/helm.go | 2 +- 7 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_alreadyd_uninstalled_release/log create mode 100644 pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_disabled_release/log create mode 100644 pkg/app/testdata/testapply_hooks/hooks_are_run_on_enabled_release/log create mode 100644 pkg/app/testdata/testapply_hooks/hooks_are_run_on_to-be-uninstalled_release/log create mode 100644 pkg/app/testdata/testapply_hooks/hooks_for_no-diff_release/log diff --git a/pkg/app/app_apply_hooks_test.go b/pkg/app/app_apply_hooks_test.go index ad880715..846efdad 100644 --- a/pkg/app/app_apply_hooks_test.go +++ b/pkg/app/app_apply_hooks_test.go @@ -261,4 +261,225 @@ releases: logLevel: "info", }) }) + + t.Run("hooks for no-diff release", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + # only prepare and cleanup are run + - events: ["prepare", "preapply", "presync", "cleanup"] + command: echo + showlogs: true + args: ["foo"] +`, + }, + selectors: []string{"app=test"}, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: nil, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + + t.Run("hooks are run on enabled release", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +values: +- bar: + enabled: true + +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["foo"] +- name: bar + condition: bar.enabled + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["bar"] +`, + }, + selectors: []string{"app=test"}, + upgraded: []exectest.Release{ + {Name: "foo"}, + {Name: "bar"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + {Name: "bar", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + + t.Run("hooks are not run on disabled release", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +values: +- bar: + enabled: false + +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["foo"] +- name: bar + condition: bar.enabled + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["bar"] +`, + }, + selectors: []string{"app=test"}, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + + t.Run("hooks are run on to-be-uninstalled release", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["foo"] +- name: bar + installed: false + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["bar"] +`, + }, + selectors: []string{"app=test"}, + lists: map[exectest.ListKey]string{ + {Filter: "^foo$", Flags: helmV2ListFlags}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +foo 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default +`, + {Filter: "^bar$", Flags: helmV2ListFlags}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +bar 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default +`, + }, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) + + t.Run("hooks are not run on alreadyd uninstalled release", func(t *testing.T) { + check(t, testcase{ + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["foo"] +- name: bar + installed: false + chart: incubator/raw + namespace: default + labels: + app: test + hooks: + - events: ["prepare", "preapply", "presync"] + command: echo + showlogs: true + args: ["bar"] +`, + }, + selectors: []string{"app=test"}, + lists: map[exectest.ListKey]string{ + {Filter: "^foo$", Flags: helmV2ListFlags}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +foo 4 Fri Nov 1 08:40:07 2019 DEPLOYED raw-3.1.0 3.1.0 default +`, + {Filter: "^bar$", Flags: helmV2ListFlags}: ``, + }, + upgraded: []exectest.Release{ + {Name: "foo"}, + }, + diffs: map[exectest.DiffKey]error{ + {Name: "foo", Chart: "incubator/raw", Flags: "--kube-contextdefault--namespacedefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + error: "", + // as we check for log output, set concurrency to 1 to avoid non-deterministic test result + concurrency: 1, + logLevel: "info", + }) + }) } diff --git a/pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_alreadyd_uninstalled_release/log b/pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_alreadyd_uninstalled_release/log new file mode 100644 index 00000000..ac333ffe --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_alreadyd_uninstalled_release/log @@ -0,0 +1,14 @@ + +hook[prepare] logs | foo +hook[prepare] logs | + +hook[preapply] logs | foo +hook[preapply] logs | + +hook[presync] logs | foo +hook[presync] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw 3.1.0 + diff --git a/pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_disabled_release/log b/pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_disabled_release/log new file mode 100644 index 00000000..0c6b5308 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/hooks_are_not_run_on_disabled_release/log @@ -0,0 +1,14 @@ + +hook[prepare] logs | foo +hook[prepare] logs | + +hook[preapply] logs | foo +hook[preapply] logs | + +hook[presync] logs | foo +hook[presync] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw + diff --git a/pkg/app/testdata/testapply_hooks/hooks_are_run_on_enabled_release/log b/pkg/app/testdata/testapply_hooks/hooks_are_run_on_enabled_release/log new file mode 100644 index 00000000..6da2c966 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/hooks_are_run_on_enabled_release/log @@ -0,0 +1,24 @@ + +hook[prepare] logs | foo +hook[prepare] logs | + +hook[prepare] logs | bar +hook[prepare] logs | + +hook[preapply] logs | foo +hook[preapply] logs | + +hook[preapply] logs | bar +hook[preapply] logs | + +hook[presync] logs | foo +hook[presync] logs | + +hook[presync] logs | bar +hook[presync] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw +bar incubator/raw + diff --git a/pkg/app/testdata/testapply_hooks/hooks_are_run_on_to-be-uninstalled_release/log b/pkg/app/testdata/testapply_hooks/hooks_are_run_on_to-be-uninstalled_release/log new file mode 100644 index 00000000..c79da665 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/hooks_are_run_on_to-be-uninstalled_release/log @@ -0,0 +1,24 @@ + +hook[prepare] logs | foo +hook[prepare] logs | + +hook[preapply] logs | bar +hook[preapply] logs | + +hook[presync] logs | bar +hook[presync] logs | + +hook[preapply] logs | foo +hook[preapply] logs | + +hook[presync] logs | foo +hook[presync] logs | + +UPDATED RELEASES: +NAME CHART VERSION +foo incubator/raw 3.1.0 + + +DELETED RELEASES: +NAME +bar diff --git a/pkg/app/testdata/testapply_hooks/hooks_for_no-diff_release/log b/pkg/app/testdata/testapply_hooks/hooks_for_no-diff_release/log new file mode 100644 index 00000000..1643d0d3 --- /dev/null +++ b/pkg/app/testdata/testapply_hooks/hooks_for_no-diff_release/log @@ -0,0 +1,6 @@ + +hook[prepare] logs | foo +hook[prepare] logs | + +hook[cleanup] logs | foo +hook[cleanup] logs | diff --git a/pkg/exectest/helm.go b/pkg/exectest/helm.go index d37965c8..6c42bab2 100644 --- a/pkg/exectest/helm.go +++ b/pkg/exectest/helm.go @@ -161,7 +161,7 @@ func (helm *Helm) List(context helmexec.HelmContext, filter string, flags ...str for k := range helm.Lists { keys = append(keys, k.String()) } - return "", fmt.Errorf("unexpected list key: %v in %v", key, strings.Join(keys, ", ")) + return "", fmt.Errorf("unexpected list key: %v not found in %v", key, strings.Join(keys, ", ")) } return res, nil } From 385c3e80ef40dfcd15e82fee8c5f31a2b0f5988f Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 19 Sep 2022 02:28:11 +0000 Subject: [PATCH 18/19] Add documentation about the new preapply hook Signed-off-by: Yusuke Kuoka --- docs/index.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/index.md b/docs/index.md index 2ed5226c..887cd839 100644 --- a/docs/index.md +++ b/docs/index.md @@ -1193,9 +1193,12 @@ Currently supported `events` are: Hooks associated to `prepare` events are triggered after each release in your helmfile is loaded from YAML, before execution. `prepare` hooks are triggered on the release as long as it is not excluded by the helmfile selector(e.g. `helmfile -l key=value`). -Hooks associated to `presync` events are triggered before each release is applied to the remote cluster. +Hooks associated to `presync` events are triggered before each release is installed or upgraded on the remote cluster. This is the ideal event to execute any commands that may mutate the cluster state as it will not be run for read-only operations like `lint`, `diff` or `template`. +`preapply` hooks are triggered before a release is uninstalled, installed, or upgraded as part of `helmfile apply`. +This is the ideal event to hook into when you are going to use `helmfile apply` for every kind of change, and you want the hook to be called only when any kind of change is being made. + `preuninstall` hooks are triggered immediately before a release is uninstalled as part of `helmfile apply`, `helmfile sync`, `helmfile delete`, and `helmfile destroy`. `postuninstall` hooks are triggered immediately after successful uninstall of a release while running `helmfile apply`, `helmfile sync`, `helmfile delete`, `helmfile destroy`. From bb13ef68d7b1876072e308a7212d2094fdaffc96 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 19 Sep 2022 02:31:07 +0000 Subject: [PATCH 19/19] fixup! Add more testcases for hooks Signed-off-by: Yusuke Kuoka --- pkg/app/testdata/testapply/install/log | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/app/testdata/testapply/install/log b/pkg/app/testdata/testapply/install/log index a8f02dc9..f340ce7b 100644 --- a/pkg/app/testdata/testapply/install/log +++ b/pkg/app/testdata/testapply/install/log @@ -47,10 +47,10 @@ GROUP RELEASES 2 default//foo processing releases in group 1/2: default//baz, default//bar -getting deployed release version failed:unexpected list key: listkey(filter=^baz$,flags=--kube-contextdefault--deleting--deployed--failed--pending) in -getting deployed release version failed:unexpected list key: listkey(filter=^bar$,flags=--kube-contextdefault--deleting--deployed--failed--pending) in +getting deployed release version failed:unexpected list key: listkey(filter=^baz$,flags=--kube-contextdefault--deleting--deployed--failed--pending) not found in +getting deployed release version failed:unexpected list key: listkey(filter=^bar$,flags=--kube-contextdefault--deleting--deployed--failed--pending) not found in processing releases in group 2/2: default//foo -getting deployed release version failed:unexpected list key: listkey(filter=^foo$,flags=--kube-contextdefault--deleting--deployed--failed--pending) in +getting deployed release version failed:unexpected list key: listkey(filter=^foo$,flags=--kube-contextdefault--deleting--deployed--failed--pending) not found in UPDATED RELEASES: NAME CHART VERSION