From ad777cbde161f3f1c100818fd2994e7ff92e12f3 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Tue, 26 Aug 2025 21:12:34 -0600 Subject: [PATCH] Include Jobs in Wait When --wait-for-jobs was introduced it was added as a new flag so the behavior on --wait didn't change in a minor version. This happened in #8363. In a major version of Helm we can merge these into a single behavior behind the --wait flag. Signed-off-by: Matt Farina --- pkg/action/install.go | 7 +----- pkg/action/install_test.go | 16 ------------- pkg/action/rollback.go | 21 +++++------------ pkg/action/upgrade.go | 19 ++++----------- pkg/action/upgrade_test.go | 23 ------------------- pkg/cmd/install.go | 1 - pkg/cmd/install_test.go | 6 ----- pkg/cmd/rollback.go | 1 - pkg/cmd/rollback_test.go | 5 ---- .../output/install-with-wait-for-jobs.txt | 7 ------ .../output/rollback-wait-for-jobs.txt | 1 - .../output/upgrade-with-wait-for-jobs.txt | 8 ------- pkg/cmd/upgrade.go | 2 -- pkg/cmd/upgrade_test.go | 6 ----- pkg/kube/client_test.go | 2 +- pkg/kube/fake/fake.go | 8 ------- pkg/kube/fake/printer.go | 5 ---- pkg/kube/interface.go | 3 --- pkg/kube/statuswait.go | 12 ---------- pkg/kube/statuswait_test.go | 10 +------- pkg/kube/wait.go | 5 ---- 21 files changed, 13 insertions(+), 155 deletions(-) delete mode 100644 pkg/cmd/testdata/output/install-with-wait-for-jobs.txt delete mode 100644 pkg/cmd/testdata/output/rollback-wait-for-jobs.txt delete mode 100644 pkg/cmd/testdata/output/upgrade-with-wait-for-jobs.txt diff --git a/pkg/action/install.go b/pkg/action/install.go index 8f76eee7b..826c0a0a2 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -85,7 +85,6 @@ type Install struct { DisableHooks bool Replace bool WaitStrategy kube.WaitStrategy - WaitForJobs bool Devel bool DependencyUpdate bool Timeout time.Duration @@ -494,11 +493,7 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource return rel, fmt.Errorf("failed to get waiter: %w", err) } - if i.WaitForJobs { - err = waiter.WaitWithJobs(resources, i.Timeout) - } else { - err = waiter.Wait(resources, i.Timeout) - } + err = waiter.Wait(resources, i.Timeout) if err != nil { return rel, err } diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index f567b3df4..fbf00bedc 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -573,22 +573,6 @@ func TestInstallRelease_Wait_Interrupted(t *testing.T) { time.Sleep(10 * time.Second) // wait for goroutine to finish is.Equal(goroutines, runtime.NumGoroutine()) } -func TestInstallRelease_WaitForJobs(t *testing.T) { - is := assert.New(t) - instAction := installAction(t) - instAction.ReleaseName = "come-fail-away" - failer := instAction.cfg.KubeClient.(*kubefake.FailingKubeClient) - failer.WaitError = fmt.Errorf("I timed out") - instAction.cfg.KubeClient = failer - instAction.WaitStrategy = kube.StatusWatcherStrategy - instAction.WaitForJobs = true - vals := map[string]interface{}{} - - res, err := instAction.Run(buildChart(), vals) - is.Error(err) - is.Contains(res.Info.Description, "I timed out") - is.Equal(res.Info.Status, release.StatusFailed) -} func TestInstallRelease_RollbackOnFailure(t *testing.T) { is := assert.New(t) diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index dd1f8c390..1a0699151 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -38,7 +38,6 @@ type Rollback struct { Version int Timeout time.Duration WaitStrategy kube.WaitStrategy - WaitForJobs bool DisableHooks bool DryRun bool // ForceReplace will, if set to `true`, ignore certain warnings and perform the rollback anyway. @@ -221,20 +220,12 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas if err != nil { return nil, fmt.Errorf("unable to set metadata visitor from target release: %w", err) } - if r.WaitForJobs { - if err := waiter.WaitWithJobs(target, r.Timeout); err != nil { - targetRelease.SetStatus(release.StatusFailed, fmt.Sprintf("Release %q failed: %s", targetRelease.Name, err.Error())) - r.cfg.recordRelease(currentRelease) - r.cfg.recordRelease(targetRelease) - return targetRelease, fmt.Errorf("release %s failed: %w", targetRelease.Name, err) - } - } else { - if err := waiter.Wait(target, r.Timeout); err != nil { - targetRelease.SetStatus(release.StatusFailed, fmt.Sprintf("Release %q failed: %s", targetRelease.Name, err.Error())) - r.cfg.recordRelease(currentRelease) - r.cfg.recordRelease(targetRelease) - return targetRelease, fmt.Errorf("release %s failed: %w", targetRelease.Name, err) - } + + if err := waiter.Wait(target, r.Timeout); err != nil { + targetRelease.SetStatus(release.StatusFailed, fmt.Sprintf("Release %q failed: %s", targetRelease.Name, err.Error())) + r.cfg.recordRelease(currentRelease) + r.cfg.recordRelease(targetRelease) + return targetRelease, fmt.Errorf("release %s failed: %w", targetRelease.Name, err) } // post-rollback hooks diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 065fa804e..8d5fda117 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -66,8 +66,6 @@ type Upgrade struct { Timeout time.Duration // WaitStrategy determines what type of waiting should be done WaitStrategy kube.WaitStrategy - // WaitForJobs determines whether the wait operation for the Jobs should be performed after the upgrade is requested. - WaitForJobs bool // DisableHooks disables hook processing if set to true. DisableHooks bool // DryRun controls whether the operation is prepared, but not executed. @@ -443,18 +441,10 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) return } - if u.WaitForJobs { - if err := waiter.WaitWithJobs(target, u.Timeout); err != nil { - u.cfg.recordRelease(originalRelease) - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) - return - } - } else { - if err := waiter.Wait(target, u.Timeout); err != nil { - u.cfg.recordRelease(originalRelease) - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) - return - } + if err := waiter.Wait(target, u.Timeout); err != nil { + u.cfg.recordRelease(originalRelease) + u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) + return } // post-upgrade hooks @@ -528,7 +518,6 @@ func (u *Upgrade) failRelease(rel *release.Release, created kube.ResourceList, e if u.WaitStrategy == kube.HookOnlyStrategy { rollin.WaitStrategy = kube.StatusWatcherStrategy } - rollin.WaitForJobs = u.WaitForJobs rollin.DisableHooks = u.DisableHooks rollin.ForceReplace = u.ForceReplace rollin.Timeout = u.Timeout diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index 8ec727671..c003b8ada 100644 --- a/pkg/action/upgrade_test.go +++ b/pkg/action/upgrade_test.go @@ -93,29 +93,6 @@ func TestUpgradeRelease_Wait(t *testing.T) { is.Equal(res.Info.Status, release.StatusFailed) } -func TestUpgradeRelease_WaitForJobs(t *testing.T) { - is := assert.New(t) - req := require.New(t) - - upAction := upgradeAction(t) - rel := releaseStub() - rel.Name = "come-fail-away" - rel.Info.Status = release.StatusDeployed - upAction.cfg.Releases.Create(rel) - - failer := upAction.cfg.KubeClient.(*kubefake.FailingKubeClient) - failer.WaitError = fmt.Errorf("I timed out") - upAction.cfg.KubeClient = failer - upAction.WaitStrategy = kube.StatusWatcherStrategy - upAction.WaitForJobs = true - vals := map[string]interface{}{} - - res, err := upAction.Run(rel.Name, buildChart(), vals) - req.Error(err) - is.Contains(res.Info.Description, "I timed out") - is.Equal(res.Info.Status, release.StatusFailed) -} - func TestUpgradeRelease_CleanupOnFail(t *testing.T) { is := assert.New(t) req := require.New(t) diff --git a/pkg/cmd/install.go b/pkg/cmd/install.go index 869163a2a..3388aacf9 100644 --- a/pkg/cmd/install.go +++ b/pkg/cmd/install.go @@ -199,7 +199,6 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during install") f.BoolVar(&client.Replace, "replace", false, "reuse the given name, only if that name is a deleted release which remains in the history. This is unsafe in production") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") - f.BoolVar(&client.WaitForJobs, "wait-for-jobs", false, "if set and --wait enabled, will wait until all Jobs have been completed before marking the release as successful. It will wait for as long as --timeout") f.BoolVarP(&client.GenerateName, "generate-name", "g", false, "generate the name (and omit the NAME parameter)") f.StringVar(&client.NameTemplate, "name-template", "", "specify template used to name the release") f.StringVar(&client.Description, "description", "", "add a custom description") diff --git a/pkg/cmd/install_test.go b/pkg/cmd/install_test.go index 9cd244e84..90eb3bf46 100644 --- a/pkg/cmd/install_test.go +++ b/pkg/cmd/install_test.go @@ -114,12 +114,6 @@ func TestInstall(t *testing.T) { cmd: "install apollo testdata/testcharts/empty --wait", golden: "output/install-with-wait.txt", }, - // Install, with wait-for-jobs - { - name: "install with wait-for-jobs", - cmd: "install apollo testdata/testcharts/empty --wait --wait-for-jobs", - golden: "output/install-with-wait-for-jobs.txt", - }, // Install, using the name-template { name: "install with name-template", diff --git a/pkg/cmd/rollback.go b/pkg/cmd/rollback.go index 4b7f3016d..46cfbcedd 100644 --- a/pkg/cmd/rollback.go +++ b/pkg/cmd/rollback.go @@ -82,7 +82,6 @@ func newRollbackCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.MarkDeprecated("force", "use --force-replace instead") f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during rollback") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") - f.BoolVar(&client.WaitForJobs, "wait-for-jobs", false, "if set and --wait enabled, will wait until all Jobs have been completed before marking the release as successful. It will wait for as long as --timeout") f.BoolVar(&client.CleanupOnFail, "cleanup-on-fail", false, "allow deletion of new resources created in this rollback when rollback fails") f.IntVar(&client.MaxHistory, "history-max", settings.MaxHistory, "limit the maximum number of revisions saved per release. Use 0 for no limit") AddWaitFlag(cmd, &client.WaitStrategy) diff --git a/pkg/cmd/rollback_test.go b/pkg/cmd/rollback_test.go index 53c63613e..0ee0e519a 100644 --- a/pkg/cmd/rollback_test.go +++ b/pkg/cmd/rollback_test.go @@ -56,11 +56,6 @@ func TestRollbackCmd(t *testing.T) { cmd: "rollback funny-honey 1 --wait", golden: "output/rollback-wait.txt", rels: rels, - }, { - name: "rollback a release with wait-for-jobs", - cmd: "rollback funny-honey 1 --wait --wait-for-jobs", - golden: "output/rollback-wait-for-jobs.txt", - rels: rels, }, { name: "rollback a release without revision", cmd: "rollback funny-honey", diff --git a/pkg/cmd/testdata/output/install-with-wait-for-jobs.txt b/pkg/cmd/testdata/output/install-with-wait-for-jobs.txt deleted file mode 100644 index c5676c610..000000000 --- a/pkg/cmd/testdata/output/install-with-wait-for-jobs.txt +++ /dev/null @@ -1,7 +0,0 @@ -NAME: apollo -LAST DEPLOYED: Fri Sep 2 22:04:05 1977 -NAMESPACE: default -STATUS: deployed -REVISION: 1 -DESCRIPTION: Install complete -TEST SUITE: None diff --git a/pkg/cmd/testdata/output/rollback-wait-for-jobs.txt b/pkg/cmd/testdata/output/rollback-wait-for-jobs.txt deleted file mode 100644 index ae3c6f1c4..000000000 --- a/pkg/cmd/testdata/output/rollback-wait-for-jobs.txt +++ /dev/null @@ -1 +0,0 @@ -Rollback was a success! Happy Helming! diff --git a/pkg/cmd/testdata/output/upgrade-with-wait-for-jobs.txt b/pkg/cmd/testdata/output/upgrade-with-wait-for-jobs.txt deleted file mode 100644 index 21784413c..000000000 --- a/pkg/cmd/testdata/output/upgrade-with-wait-for-jobs.txt +++ /dev/null @@ -1,8 +0,0 @@ -Release "crazy-bunny" has been upgraded. Happy Helming! -NAME: crazy-bunny -LAST DEPLOYED: Fri Sep 2 22:04:05 1977 -NAMESPACE: default -STATUS: deployed -REVISION: 3 -DESCRIPTION: Upgrade complete -TEST SUITE: None diff --git a/pkg/cmd/upgrade.go b/pkg/cmd/upgrade.go index 4ca889fc2..4af42411d 100644 --- a/pkg/cmd/upgrade.go +++ b/pkg/cmd/upgrade.go @@ -137,7 +137,6 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { instClient.SkipCRDs = client.SkipCRDs instClient.Timeout = client.Timeout instClient.WaitStrategy = client.WaitStrategy - instClient.WaitForJobs = client.WaitForJobs instClient.Devel = client.Devel instClient.Namespace = client.Namespace instClient.RollbackOnFailure = client.RollbackOnFailure @@ -281,7 +280,6 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.ResetValues, "reset-values", false, "when upgrading, reset the values to the ones built into the chart") f.BoolVar(&client.ReuseValues, "reuse-values", false, "when upgrading, reuse the last release's values and merge in any overrides from the command line via --set and -f. If '--reset-values' is specified, this is ignored") f.BoolVar(&client.ResetThenReuseValues, "reset-then-reuse-values", false, "when upgrading, reset the values to the ones built into the chart, apply the last release's values and merge in any overrides from the command line via --set and -f. If '--reset-values' or '--reuse-values' is specified, this is ignored") - f.BoolVar(&client.WaitForJobs, "wait-for-jobs", false, "if set and --wait enabled, will wait until all Jobs have been completed before marking the release as successful. It will wait for as long as --timeout") f.BoolVar(&client.RollbackOnFailure, "rollback-on-failure", false, "if set, Helm will rollback the upgrade to previous success release upon failure. The --wait flag will be defaulted to \"watcher\" if --rollback-on-failure is set") f.BoolVar(&client.RollbackOnFailure, "atomic", false, "deprecated") f.MarkDeprecated("atomic", "use --rollback-on-failure instead") diff --git a/pkg/cmd/upgrade_test.go b/pkg/cmd/upgrade_test.go index d7375dcad..07c848647 100644 --- a/pkg/cmd/upgrade_test.go +++ b/pkg/cmd/upgrade_test.go @@ -138,12 +138,6 @@ func TestUpgradeCmd(t *testing.T) { golden: "output/upgrade-with-wait.txt", rels: []*release.Release{relMock("crazy-bunny", 2, ch2)}, }, - { - name: "upgrade a release with wait-for-jobs", - cmd: fmt.Sprintf("upgrade crazy-bunny --wait --wait-for-jobs '%s'", chartPath), - golden: "output/upgrade-with-wait-for-jobs.txt", - rels: []*release.Release{relMock("crazy-bunny", 2, ch2)}, - }, { name: "upgrade a release with missing dependencies", cmd: fmt.Sprintf("upgrade bonkers-bunny %s", missingDepsPath), diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 5060a5fc2..c94525728 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -753,7 +753,7 @@ func TestWaitJob(t *testing.T) { t.Errorf("expected 1 resource created, got %d", len(result.Created)) } - if err := c.WaitWithJobs(resources, time.Second*30); err != nil { + if err := c.Wait(resources, time.Second*30); err != nil { t.Errorf("expected wait without error, got %s", err) } diff --git a/pkg/kube/fake/fake.go b/pkg/kube/fake/fake.go index 588bba83d..07360fd1e 100644 --- a/pkg/kube/fake/fake.go +++ b/pkg/kube/fake/fake.go @@ -84,14 +84,6 @@ func (f *FailingKubeWaiter) Wait(resources kube.ResourceList, d time.Duration) e return f.PrintingKubeWaiter.Wait(resources, d) } -// WaitWithJobs returns the configured error if set or prints -func (f *FailingKubeWaiter) WaitWithJobs(resources kube.ResourceList, d time.Duration) error { - if f.waitError != nil { - return f.waitError - } - return f.PrintingKubeWaiter.WaitWithJobs(resources, d) -} - // WaitForDelete returns the configured error if set or prints func (f *FailingKubeWaiter) WaitForDelete(resources kube.ResourceList, d time.Duration) error { if f.waitForDeleteError != nil { diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index 16c93615a..10d156e8a 100644 --- a/pkg/kube/fake/printer.go +++ b/pkg/kube/fake/printer.go @@ -70,11 +70,6 @@ func (p *PrintingKubeWaiter) Wait(resources kube.ResourceList, _ time.Duration) return err } -func (p *PrintingKubeWaiter) WaitWithJobs(resources kube.ResourceList, _ time.Duration) error { - _, err := io.Copy(p.Out, bufferize(resources)) - return err -} - func (p *PrintingKubeWaiter) WaitForDelete(resources kube.ResourceList, _ time.Duration) error { _, err := io.Copy(p.Out, bufferize(resources)) return err diff --git a/pkg/kube/interface.go b/pkg/kube/interface.go index 7339ae0ff..e46a376ec 100644 --- a/pkg/kube/interface.go +++ b/pkg/kube/interface.go @@ -58,9 +58,6 @@ type Waiter interface { // Wait waits up to the given timeout for the specified resources to be ready. Wait(resources ResourceList, timeout time.Duration) error - // WaitWithJobs wait up to the given timeout for the specified resources to be ready, including jobs. - WaitWithJobs(resources ResourceList, timeout time.Duration) error - // WaitForDelete wait up to the given timeout for the specified resources to be deleted. WaitForDelete(resources ResourceList, timeout time.Duration) error diff --git a/pkg/kube/statuswait.go b/pkg/kube/statuswait.go index 2d7cfe971..2441e9ee1 100644 --- a/pkg/kube/statuswait.go +++ b/pkg/kube/statuswait.go @@ -74,14 +74,6 @@ func (w *statusWaiter) WatchUntilReady(resourceList ResourceList, timeout time.D } func (w *statusWaiter) Wait(resourceList ResourceList, timeout time.Duration) error { - ctx, cancel := context.WithTimeout(context.TODO(), timeout) - defer cancel() - slog.Debug("waiting for resources", "count", len(resourceList), "timeout", timeout) - sw := watcher.NewDefaultStatusWatcher(w.client, w.restMapper) - return w.wait(ctx, resourceList, sw) -} - -func (w *statusWaiter) WaitWithJobs(resourceList ResourceList, timeout time.Duration) error { ctx, cancel := context.WithTimeout(context.TODO(), timeout) defer cancel() slog.Debug("waiting for resources", "count", len(resourceList), "timeout", timeout) @@ -226,10 +218,6 @@ func (w *hookOnlyWaiter) Wait(_ ResourceList, _ time.Duration) error { return nil } -func (w *hookOnlyWaiter) WaitWithJobs(_ ResourceList, _ time.Duration) error { - return nil -} - func (w *hookOnlyWaiter) WaitForDelete(_ ResourceList, _ time.Duration) error { return nil } diff --git a/pkg/kube/statuswait_test.go b/pkg/kube/statuswait_test.go index 4b06da896..bf8ebed01 100644 --- a/pkg/kube/statuswait_test.go +++ b/pkg/kube/statuswait_test.go @@ -274,19 +274,11 @@ func TestStatusWait(t *testing.T) { name string objManifests []string expectErrs []error - waitForJobs bool }{ { name: "Job is not complete", objManifests: []string{jobNoStatusManifest}, expectErrs: []error{errors.New("resource not ready, name: test, kind: Job, status: InProgress"), errors.New("context deadline exceeded")}, - waitForJobs: true, - }, - { - name: "Job is ready but not complete", - objManifests: []string{jobReadyManifest}, - expectErrs: nil, - waitForJobs: false, }, { name: "Pod is ready", @@ -380,7 +372,7 @@ func TestWaitForJobComplete(t *testing.T) { assert.NoError(t, err) } resourceList := getResourceListFromRuntimeObjs(t, c, objs) - err := statusWaiter.WaitWithJobs(resourceList, time.Second*3) + err := statusWaiter.Wait(resourceList, time.Second*3) if tt.expectErrs != nil { assert.EqualError(t, err, errors.Join(tt.expectErrs...).Error()) return diff --git a/pkg/kube/wait.go b/pkg/kube/wait.go index 9bfa1ef6d..aba5cbe38 100644 --- a/pkg/kube/wait.go +++ b/pkg/kube/wait.go @@ -52,11 +52,6 @@ type legacyWaiter struct { } func (hw *legacyWaiter) Wait(resources ResourceList, timeout time.Duration) error { - hw.c = NewReadyChecker(hw.kubeClient, PausedAsReady(true)) - return hw.waitForResources(resources, timeout) -} - -func (hw *legacyWaiter) WaitWithJobs(resources ResourceList, timeout time.Duration) error { hw.c = NewReadyChecker(hw.kubeClient, PausedAsReady(true), CheckJobs(true)) return hw.waitForResources(resources, timeout) }