From b2dc411f9d77f3bca969fb4ad955d4a091aa0454 Mon Sep 17 00:00:00 2001 From: George Jenkins Date: Tue, 12 Aug 2025 10:49:10 -0700 Subject: [PATCH] code review (error checks, collapse forceConflicts, UpdateApplyFunc) Signed-off-by: George Jenkins --- pkg/action/hooks.go | 2 +- pkg/action/install.go | 8 +++--- pkg/action/rollback.go | 2 +- pkg/action/upgrade.go | 2 +- pkg/kube/client.go | 64 +++++++++++++++++++++-------------------- pkg/kube/client_test.go | 16 +++++------ 6 files changed, 47 insertions(+), 47 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 95260e0e4..275a1bf52 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -75,7 +75,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, // Create hook resources if _, err := cfg.KubeClient.Create( resources, - kube.ClientCreateOptionServerSideApply(false)); err != nil { + kube.ClientCreateOptionServerSideApply(false, false)); err != nil { h.LastRun.CompletedAt = helmtime.Now() h.LastRun.Phase = release.HookPhaseFailed return fmt.Errorf("warning: Hook %s %s failed: %w", hook, h.Path, err) diff --git a/pkg/action/install.go b/pkg/action/install.go index 9a9101f5d..b46b4446b 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -175,7 +175,7 @@ func (i *Install) installCRDs(crds []chart.CRD) error { // Send them to Kube if _, err := i.cfg.KubeClient.Create( res, - kube.ClientCreateOptionServerSideApply(false)); err != nil { + kube.ClientCreateOptionServerSideApply(false, false)); err != nil { // If the error is CRD already exists, continue. if apierrors.IsAlreadyExists(err) { crdName := res[0].Name @@ -403,7 +403,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma } if _, err := i.cfg.KubeClient.Create( resourceList, - kube.ClientCreateOptionServerSideApply(false)); err != nil && !apierrors.IsAlreadyExists(err) { + kube.ClientCreateOptionServerSideApply(false, false)); err != nil && !apierrors.IsAlreadyExists(err) { return nil, err } } @@ -474,13 +474,13 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource if len(toBeAdopted) == 0 && len(resources) > 0 { _, err = i.cfg.KubeClient.Create( resources, - kube.ClientCreateOptionServerSideApply(false)) + kube.ClientCreateOptionServerSideApply(false, false)) } else if len(resources) > 0 { updateThreeWayMergeForUnstructured := i.TakeOwnership _, err = i.cfg.KubeClient.Update( toBeAdopted, resources, - kube.ClientUpdateOptionServerSideApply(false), + kube.ClientUpdateOptionServerSideApply(false, false), kube.ClientUpdateOptionThreeWayMergeForUnstructured(updateThreeWayMergeForUnstructured), kube.ClientUpdateOptionForceReplace(i.ForceReplace)) } diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index f60d4f4bc..dd1f8c390 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -193,7 +193,7 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas results, err := r.cfg.KubeClient.Update( current, target, - kube.ClientUpdateOptionServerSideApply(false), + kube.ClientUpdateOptionServerSideApply(false, false), kube.ClientUpdateOptionForceReplace(r.ForceReplace)) if err != nil { diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index a32d6e78e..abf4342d3 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -429,7 +429,7 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele results, err := u.cfg.KubeClient.Update( current, target, - kube.ClientUpdateOptionServerSideApply(false), + kube.ClientUpdateOptionServerSideApply(false, false), kube.ClientUpdateOptionForceReplace(u.ForceReplace)) if err != nil { u.cfg.recordRelease(originalRelease) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index b436f518f..016055392 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -214,26 +214,23 @@ type ClientCreateOption func(*clientCreateOptions) error // ClientUpdateOptionServerSideApply enables performing object apply server-side // see: https://kubernetes.io/docs/reference/using-api/server-side-apply/ -func ClientCreateOptionServerSideApply(serverSideApply bool) ClientCreateOption { - return func(o *clientCreateOptions) error { - o.serverSideApply = serverSideApply - - return nil - } -} - -// ClientCreateOptionForceConflicts forces field conflicts to be resolved +// +// `forceConflicts` forces conflicts to be resolved (may be when serverSideApply enabled only) // see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts -// Only valid when ClientUpdateOptionServerSideApply enabled -func ClientCreateOptionForceConflicts(forceConflicts bool) ClientCreateOption { +func ClientCreateOptionServerSideApply(serverSideApply, forceConflicts bool) ClientCreateOption { return func(o *clientCreateOptions) error { + if !serverSideApply && forceConflicts { + return fmt.Errorf("forceConflicts enabled when serverSideApply disabled") + } + + o.serverSideApply = serverSideApply o.forceConflicts = forceConflicts return nil } } -// ClientCreateOptionDryRun performs non-mutating operations only +// ClientCreateOptionDryRun requests the server to perform non-mutating operations only func ClientCreateOptionDryRun(dryRun bool) ClientCreateOption { return func(o *clientCreateOptions) error { o.dryRun = dryRun @@ -264,8 +261,12 @@ func (c *Client) Create(resources ResourceList, options ...ClientCreateOption) ( fieldValidationDirective: FieldValidationDirectiveStrict, } + errs := make([]error, 0, len(options)) for _, o := range options { - o(&createOptions) + errs = append(errs, o(&createOptions)) + } + if err := errors.Join(errs...); err != nil { + return nil, fmt.Errorf("invalid client create option(s): %w", err) } if createOptions.forceConflicts && !createOptions.serverSideApply { @@ -499,7 +500,7 @@ func (c *Client) BuildTable(reader io.Reader, validate bool) (ResourceList, erro transformRequests) } -func (c *Client) update(originals, targets ResourceList, updateApplyFunc func(original, target *resource.Info) error) (*Result, error) { +func (c *Client) update(originals, targets ResourceList, updateApplyFunc UpdateApplyFunc) (*Result, error) { updateErrors := []error{} res := &Result{} @@ -599,9 +600,17 @@ func ClientUpdateOptionThreeWayMergeForUnstructured(threeWayMergeForUnstructured // ClientUpdateOptionServerSideApply enables performing object apply server-side (default) // see: https://kubernetes.io/docs/reference/using-api/server-side-apply/ // Must not be enabled when ClientUpdateOptionThreeWayMerge is enabled -func ClientUpdateOptionServerSideApply(serverSideApply bool) ClientUpdateOption { +// +// `forceConflicts` forces conflicts to be resolved (may be enabled when serverSideApply enabled only) +// see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts +func ClientUpdateOptionServerSideApply(serverSideApply, forceConflicts bool) ClientUpdateOption { return func(o *clientUpdateOptions) error { + if !serverSideApply && forceConflicts { + return fmt.Errorf("forceConflicts enabled when serverSideApply disabled") + } + o.serverSideApply = serverSideApply + o.forceConflicts = forceConflicts return nil } @@ -617,20 +626,7 @@ func ClientUpdateOptionForceReplace(forceReplace bool) ClientUpdateOption { } } -// ClientUpdateOptionForceConflicts forces field conflicts to be resolved -// see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts -// Must not be enabled when ClientUpdateOptionForceReplace is enabled -func ClientUpdateOptionForceConflicts(forceConflicts bool) ClientUpdateOption { - return func(o *clientUpdateOptions) error { - o.forceConflicts = forceConflicts - - return nil - } -} - -// ClientUpdateOptionForceConflicts forces field conflicts to be resolved -// see: https://kubernetes.io/docs/reference/using-api/server-side-apply/#conflicts -// Must not be enabled when ClientUpdateOptionForceReplace is enabled +// ClientUpdateOptionDryRun requests the server to perform non-mutating operations only func ClientUpdateOptionDryRun(dryRun bool) ClientUpdateOption { return func(o *clientUpdateOptions) error { o.dryRun = dryRun @@ -652,6 +648,8 @@ func ClientUpdateOptionFieldValidationDirective(fieldValidationDirective FieldVa } } +type UpdateApplyFunc func(original, target *resource.Info) error + // Update takes the current list of objects and target list of objects and // creates resources that don't already exist, updates resources that have been // modified in the target configuration, and deletes resources from the current @@ -667,8 +665,12 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate fieldValidationDirective: FieldValidationDirectiveStrict, } + errs := make([]error, 0, len(options)) for _, o := range options { - o(&updateOptions) + errs = append(errs, o(&updateOptions)) + } + if err := errors.Join(errs...); err != nil { + return nil, fmt.Errorf("invalid client update option(s): %w", err) } if updateOptions.threeWayMergeForUnstructured && updateOptions.serverSideApply { @@ -683,7 +685,7 @@ func (c *Client) Update(originals, targets ResourceList, options ...ClientUpdate return nil, fmt.Errorf("invalid operation: cannot use server-side apply and force replace together") } - makeUpdateApplyFunc := func() func(original, target *resource.Info) error { + makeUpdateApplyFunc := func() UpdateApplyFunc { if updateOptions.forceReplace { slog.Debug( "using resource replace update strategy", diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index bdc5a9d7f..5060a5fc2 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -292,7 +292,7 @@ func TestCreate(t *testing.T) { result, err := c.Create( list, - ClientCreateOptionServerSideApply(tc.ServerSideApply)) + ClientCreateOptionServerSideApply(tc.ServerSideApply, false)) if tc.ExpectedErrorContains != "" { require.ErrorContains(t, err, tc.ExpectedErrorContains) } else { @@ -467,7 +467,7 @@ func TestUpdate(t *testing.T) { second, ClientUpdateOptionThreeWayMergeForUnstructured(tc.ThreeWayMergeForUnstructured), ClientUpdateOptionForceReplace(false), - ClientUpdateOptionServerSideApply(tc.ServerSideApply)) + ClientUpdateOptionServerSideApply(tc.ServerSideApply, false)) require.NoError(t, err) assert.Len(t, result.Created, 1, "expected 1 resource created, got %d", len(result.Created)) @@ -684,7 +684,7 @@ func TestWait(t *testing.T) { result, err := c.Create( resources, - ClientCreateOptionServerSideApply(false)) + ClientCreateOptionServerSideApply(false, false)) if err != nil { t.Fatal(err) @@ -744,7 +744,7 @@ func TestWaitJob(t *testing.T) { } result, err := c.Create( resources, - ClientCreateOptionServerSideApply(false)) + ClientCreateOptionServerSideApply(false, false)) if err != nil { t.Fatal(err) @@ -806,7 +806,7 @@ func TestWaitDelete(t *testing.T) { } result, err := c.Create( resources, - ClientCreateOptionServerSideApply(false)) + ClientCreateOptionServerSideApply(false, false)) if err != nil { t.Fatal(err) } @@ -1225,7 +1225,6 @@ func TestCreatePatchCustomResourceSpec(t *testing.T) { t.Run(testCase.name, testCase.run) } -<<<<<<< HEAD type errorFactory struct { *cmdtesting.TestFactory err error @@ -1326,8 +1325,8 @@ func TestIsReachable(t *testing.T) { } }) } -||||||| parent of 36a476ff4 (Kube client support server-side apply) -======= +} + func TestIsIncompatibleServerError(t *testing.T) { testCases := map[string]struct { Err error @@ -1749,5 +1748,4 @@ func TestDetermineFieldValidationDirective(t *testing.T) { assert.Equal(t, FieldValidationDirectiveIgnore, determineFieldValidationDirective(false)) assert.Equal(t, FieldValidationDirectiveStrict, determineFieldValidationDirective(true)) ->>>>>>> 36a476ff4 (Kube client support server-side apply) }