From 47c2fd02d90e621867787b372faa37719e52b726 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Fri, 24 Apr 2026 21:09:44 +0530 Subject: [PATCH 1/5] fix(live): propagate caller ctx into ResourceGroup inventory ops pkg/live/inventoryrg.go passed context.TODO() to all 7 of its live cluster API calls, so Ctrl-C and command-level timeouts could not cancel an in-flight apply/destroy/plan. The upstream inventory.Storage interface doesn't accept a ctx parameter, so carry ctx on the InventoryResourceGroup struct and inject it at wrap time via a new WrapInventoryObjWithContext factory. Legacy callers without a ctx fall back to context.Background(). ResourceGroupCRDMatched takes ctx directly. All 6 callers updated. Signed-off-by: Jaisheesh-2006 --- commands/alpha/live/plan/command.go | 2 +- commands/live/apply/cmdapply.go | 4 +- commands/live/destroy/cmddestroy.go | 2 +- commands/live/livecmd.go | 2 +- commands/live/migrate/migratecmd.go | 14 ++--- commands/live/migrate/migratecmd_test.go | 5 +- pkg/live/inventory-client-factory.go | 30 +++++++++- pkg/live/inventoryrg.go | 61 ++++++++++++++++--- pkg/live/inventoryrg_test.go | 74 +++++++++++++++++++++++- pkg/live/planner/cluster.go | 8 ++- 10 files changed, 173 insertions(+), 29 deletions(-) diff --git a/commands/alpha/live/plan/command.go b/commands/alpha/live/plan/command.go index 886face464..2f73715683 100644 --- a/commands/alpha/live/plan/command.go +++ b/commands/alpha/live/plan/command.go @@ -133,7 +133,7 @@ func (r *Runner) RunE(c *cobra.Command, args []string) error { } // Create and execute the planner. - planner, err := kptplanner.NewClusterPlanner(r.factory) + planner, err := kptplanner.NewClusterPlanner(r.ctx, r.factory) if err != nil { return err } diff --git a/commands/live/apply/cmdapply.go b/commands/live/apply/cmdapply.go index 2c5ea3e0c1..37a58059c9 100644 --- a/commands/live/apply/cmdapply.go +++ b/commands/live/apply/cmdapply.go @@ -228,7 +228,7 @@ func runApply(r *Runner, invInfo inventory.Info, objs []*unstructured.Unstructur if err = cmdutil.InstallResourceGroupCRD(r.ctx, f); err != nil { return err } - } else if !live.ResourceGroupCRDMatched(f) { + } else if !live.ResourceGroupCRDMatched(r.ctx, f) { if err = cmdutil.InstallResourceGroupCRD(r.ctx, f); err != nil { return &cmdutil.ResourceGroupCRDNotLatestError{ Err: err, @@ -239,7 +239,7 @@ func runApply(r *Runner, invInfo inventory.Info, objs []*unstructured.Unstructur // Run the applier. It will return a channel where we can receive updates // to keep track of progress and any issues. - invClient, err := inventory.NewClient(r.factory, live.WrapInventoryObj, live.InvToUnstructuredFunc, r.statusPolicy, live.ResourceGroupGVK) + invClient, err := inventory.NewClient(r.factory, live.WrapInventoryObjWithContext(r.ctx), live.InvToUnstructuredFunc, r.statusPolicy, live.ResourceGroupGVK) if err != nil { return err } diff --git a/commands/live/destroy/cmddestroy.go b/commands/live/destroy/cmddestroy.go index bc274f2578..9bee3f94b4 100644 --- a/commands/live/destroy/cmddestroy.go +++ b/commands/live/destroy/cmddestroy.go @@ -168,7 +168,7 @@ func (r *Runner) runE(c *cobra.Command, args []string) error { func runDestroy(r *Runner, inv inventory.Info, dryRunStrategy common.DryRunStrategy) error { // Run the destroyer. It will return a channel where we can receive updates // to keep track of progress and any issues. - invClient, err := inventory.NewClient(r.factory, live.WrapInventoryObj, live.InvToUnstructuredFunc, r.statusPolicy, live.ResourceGroupGVK) + invClient, err := inventory.NewClient(r.factory, live.WrapInventoryObjWithContext(r.ctx), live.InvToUnstructuredFunc, r.statusPolicy, live.ResourceGroupGVK) if err != nil { return err } diff --git a/commands/live/livecmd.go b/commands/live/livecmd.go index fea734d7cb..e177668bad 100644 --- a/commands/live/livecmd.go +++ b/commands/live/livecmd.go @@ -47,7 +47,7 @@ func GetCommand(ctx context.Context, _, version string) *cobra.Command { } f := util.NewFactory(liveCmd, version) - invFactory := live.NewClusterClientFactory() + invFactory := live.NewClusterClientFactoryWithContext(ctx) loader := status.NewRGInventoryLoader(ctx, f) // Init command which updates a Kptfile for the ResourceGroup inventory object. diff --git a/commands/live/migrate/migratecmd.go b/commands/live/migrate/migratecmd.go index 616e228898..874d2c66fe 100644 --- a/commands/live/migrate/migratecmd.go +++ b/commands/live/migrate/migratecmd.go @@ -58,8 +58,8 @@ type Runner struct { name string rgFile string force bool - rgInvClientFunc func(util.Factory) (inventory.Client, error) - cmInvClientFunc func(util.Factory) (inventory.Client, error) + rgInvClientFunc func(context.Context, util.Factory) (inventory.Client, error) + cmInvClientFunc func(context.Context, util.Factory) (inventory.Client, error) cmLoader manifestreader.ManifestLoader cmNotMigrated bool // flag to determine if migration from ConfigMap has occurred } @@ -348,11 +348,11 @@ func validateParams(reader io.Reader, args []string) error { return nil } -func rgInvClient(factory util.Factory) (inventory.Client, error) { - return inventory.NewClient(factory, live.WrapInventoryObj, live.InvToUnstructuredFunc, inventory.StatusPolicyAll, live.ResourceGroupGVK) +func rgInvClient(ctx context.Context, factory util.Factory) (inventory.Client, error) { + return inventory.NewClient(factory, live.WrapInventoryObjWithContext(ctx), live.InvToUnstructuredFunc, inventory.StatusPolicyAll, live.ResourceGroupGVK) } -func cmInvClient(factory util.Factory) (inventory.Client, error) { +func cmInvClient(_ context.Context, factory util.Factory) (inventory.Client, error) { return inventory.NewClient(factory, inventory.WrapInventoryObj, inventory.InvInfoToConfigMap, inventory.StatusPolicyAll, live.ResourceGroupGVK) } @@ -412,11 +412,11 @@ func (mr *Runner) migrateKptfileToRG(args []string) error { func (mr *Runner) migrateCMToRG(stdinBytes []byte, args []string) error { // Create the inventory clients for reading inventories based on RG and // ConfigMap. - rgInvClient, err := mr.rgInvClientFunc(mr.factory) + rgInvClient, err := mr.rgInvClientFunc(mr.ctx, mr.factory) if err != nil { return err } - cmInvClient, err := mr.cmInvClientFunc(mr.factory) + cmInvClient, err := mr.cmInvClientFunc(mr.ctx, mr.factory) if err != nil { return err } diff --git a/commands/live/migrate/migratecmd_test.go b/commands/live/migrate/migratecmd_test.go index b6dbadda80..51273b01aa 100644 --- a/commands/live/migrate/migratecmd_test.go +++ b/commands/live/migrate/migratecmd_test.go @@ -15,6 +15,7 @@ package migrate import ( + "context" "os" "path/filepath" "strings" @@ -169,7 +170,7 @@ func TestKptMigrate_migrateKptfileToRG(t *testing.T) { migrateRunner := NewRunner(ctx, tf, cmLoader, ioStreams) migrateRunner.dryRun = tc.dryRun migrateRunner.rgFile = tc.rgFilename - migrateRunner.cmInvClientFunc = func(_ util.Factory) (inventory.Client, error) { + migrateRunner.cmInvClientFunc = func(_ context.Context, _ util.Factory) (inventory.Client, error) { return inventory.NewFakeClient([]object.ObjMetadata{}), nil } err = migrateRunner.migrateKptfileToRG([]string{dir}) @@ -247,7 +248,7 @@ func TestKptMigrate_retrieveConfigMapInv(t *testing.T) { // Create MigrateRunner and call "retrieveConfigMapInv" cmLoader := manifestreader.NewManifestLoader(tf) migrateRunner := NewRunner(ctx, tf, cmLoader, ioStreams) - migrateRunner.cmInvClientFunc = func(_ util.Factory) (inventory.Client, error) { + migrateRunner.cmInvClientFunc = func(_ context.Context, _ util.Factory) (inventory.Client, error) { return inventory.NewFakeClient([]object.ObjMetadata{}), nil } actual, err := migrateRunner.retrieveConfigMapInv(strings.NewReader(tc.configMap), []string{"-"}) diff --git a/pkg/live/inventory-client-factory.go b/pkg/live/inventory-client-factory.go index 74ded93db7..4cc7829659 100644 --- a/pkg/live/inventory-client-factory.go +++ b/pkg/live/inventory-client-factory.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt Authors +// Copyright 2022,2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,18 +15,42 @@ package live import ( + "context" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "sigs.k8s.io/cli-utils/pkg/inventory" ) -// ClusterClientFactory is a factory that creates instances of ClusterClient inventory client. +// ClusterClientFactory is a factory that creates instances of ClusterClient +// inventory client. +// +// Ctx, if set, is plumbed into the InventoryResourceGroup wrapper so +// Apply / ApplyWithPrune honor caller cancellation (Ctrl-C, timeouts). +// The upstream inventory.ClientFactory interface's NewClient signature +// does not accept a context, so we carry one on the factory instead; +// construct via NewClusterClientFactoryWithContext when you have one. type ClusterClientFactory struct { StatusPolicy inventory.StatusPolicy + Ctx context.Context } +// NewClusterClientFactory returns a ClusterClientFactory that will build +// inventory clients with no context propagation (cluster API calls use +// context.Background()). Prefer NewClusterClientFactoryWithContext. func NewClusterClientFactory() *ClusterClientFactory { return &ClusterClientFactory{StatusPolicy: inventory.StatusPolicyNone} } + +// NewClusterClientFactoryWithContext returns a ClusterClientFactory that +// threads ctx into every inventory client it produces. +func NewClusterClientFactoryWithContext(ctx context.Context) *ClusterClientFactory { + return &ClusterClientFactory{StatusPolicy: inventory.StatusPolicyNone, Ctx: ctx} +} + func (ccf *ClusterClientFactory) NewClient(factory cmdutil.Factory) (inventory.Client, error) { - return inventory.NewClient(factory, WrapInventoryObj, InvToUnstructuredFunc, ccf.StatusPolicy, ResourceGroupGVK) + wrap := WrapInventoryObj + if ccf.Ctx != nil { + wrap = WrapInventoryObjWithContext(ccf.Ctx) + } + return inventory.NewClient(factory, wrap, InvToUnstructuredFunc, ccf.StatusPolicy, ResourceGroupGVK) } diff --git a/pkg/live/inventoryrg.go b/pkg/live/inventoryrg.go index 5434d9abad..c8c0f94162 100644 --- a/pkg/live/inventoryrg.go +++ b/pkg/live/inventoryrg.go @@ -1,4 +1,4 @@ -// Copyright 2020 The kpt Authors +// Copyright 2020,2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -59,12 +59,30 @@ var ResourceGroupGVK = schema.GroupVersionKind{ // InventoryResourceGroup wraps a ResourceGroup resource and implements // the Inventory and InventoryInfo interface. This wrapper loads and stores the // object metadata (inventory) to and from the wrapped ResourceGroup. +// +// ctx, if non-nil, is the caller's context and is used for the live +// Kubernetes API calls performed by Apply / ApplyWithPrune. It exists on +// the struct (rather than as a parameter) because the upstream +// inventory.Storage interface signatures do not include a context; see +// WrapInventoryObjWithContext. type InventoryResourceGroup struct { + ctx context.Context inv *unstructured.Unstructured objMetas []object.ObjMetadata objStatus []actuation.ObjectStatus } +// contextOrBackground returns the caller-supplied context if set, or +// context.Background() otherwise. Callers going through +// WrapInventoryObjWithContext get real cancellation/timeout propagation; +// legacy callers using WrapInventoryObj continue to work unchanged. +func (icm *InventoryResourceGroup) contextOrBackground() context.Context { + if icm.ctx != nil { + return icm.ctx + } + return context.Background() +} + func (icm *InventoryResourceGroup) Strategy() inventory.Strategy { return inventory.NameStrategy } @@ -75,6 +93,11 @@ var _ inventory.Info = &InventoryResourceGroup{} // WrapInventoryObj takes a passed ResourceGroup (as a resource.Info), // wraps it with the InventoryResourceGroup and upcasts the wrapper as // an the Inventory interface. +// +// The wrapped inventory will use context.Background() for cluster API +// calls. Prefer WrapInventoryObjWithContext when you have a caller +// context (e.g. cmd.Context()) so that Ctrl-C and caller-side timeouts +// actually cancel the in-flight request. func WrapInventoryObj(obj *unstructured.Unstructured) inventory.Storage { if obj != nil { klog.V(4).Infof("wrapping Inventory obj: %s/%s\n", obj.GetNamespace(), obj.GetName()) @@ -82,6 +105,21 @@ func WrapInventoryObj(obj *unstructured.Unstructured) inventory.Storage { return &InventoryResourceGroup{inv: obj} } +// WrapInventoryObjWithContext returns a wrapper function compatible with +// inventory.NewClient's WrapObjFunc parameter. The returned function +// produces an InventoryResourceGroup that carries ctx, so subsequent +// Apply / ApplyWithPrune calls honor the caller's cancellation and +// timeout. ctx MUST NOT be nil; pass context.Background() explicitly if +// you truly want no cancellation. +func WrapInventoryObjWithContext(ctx context.Context) func(*unstructured.Unstructured) inventory.Storage { + return func(obj *unstructured.Unstructured) inventory.Storage { + if obj != nil { + klog.V(4).Infof("wrapping Inventory obj with ctx: %s/%s\n", obj.GetNamespace(), obj.GetName()) + } + return &InventoryResourceGroup{ctx: ctx, inv: obj} + } +} + func WrapInventoryInfoObj(obj *unstructured.Unstructured) inventory.Info { if obj != nil { klog.V(4).Infof("wrapping InventoryInfo obj: %s/%s\n", obj.GetNamespace(), obj.GetName()) @@ -256,9 +294,10 @@ func (icm *InventoryResourceGroup) Apply(dc dynamic.Interface, mapper meta.RESTM if err != nil { return err } + ctx := icm.contextOrBackground() // Get cluster object, if exsists. - clusterObj, err := namespacedClient.Get(context.TODO(), invInfo.GetName(), metav1.GetOptions{}) + clusterObj, err := namespacedClient.Get(ctx, invInfo.GetName(), metav1.GetOptions{}) if err != nil && !apierrors.IsNotFound(err) { return err } @@ -267,10 +306,10 @@ func (icm *InventoryResourceGroup) Apply(dc dynamic.Interface, mapper meta.RESTM if clusterObj == nil { // Create cluster inventory object, if it does not exist on cluster. - appliedObj, err = namespacedClient.Create(context.TODO(), invInfo, metav1.CreateOptions{}) + appliedObj, err = namespacedClient.Create(ctx, invInfo, metav1.CreateOptions{}) } else { // Update the cluster inventory object instead. - appliedObj, err = namespacedClient.Update(context.TODO(), invInfo, metav1.UpdateOptions{}) + appliedObj, err = namespacedClient.Update(ctx, invInfo, metav1.UpdateOptions{}) } if err != nil { return err @@ -279,7 +318,7 @@ func (icm *InventoryResourceGroup) Apply(dc dynamic.Interface, mapper meta.RESTM // Update status. if statusPolicy == inventory.StatusPolicyAll { invInfo.SetResourceVersion(appliedObj.GetResourceVersion()) - _, err = namespacedClient.UpdateStatus(context.TODO(), invInfo, metav1.UpdateOptions{}) + _, err = namespacedClient.UpdateStatus(ctx, invInfo, metav1.UpdateOptions{}) } return err @@ -290,11 +329,12 @@ func (icm *InventoryResourceGroup) ApplyWithPrune(dc dynamic.Interface, mapper m if err != nil { return err } + ctx := icm.contextOrBackground() // Update the cluster inventory object. // Since the ResourceGroup CRD specifies the status as a sub-resource, this // will not update the status. - appliedObj, err := namespacedClient.Update(context.TODO(), invInfo, metav1.UpdateOptions{}) + appliedObj, err := namespacedClient.Update(ctx, invInfo, metav1.UpdateOptions{}) if err != nil { return err } @@ -314,7 +354,7 @@ func (icm *InventoryResourceGroup) ApplyWithPrune(dc dynamic.Interface, mapper m if err != nil { return err } - _, err = namespacedClient.UpdateStatus(context.TODO(), appliedObj, metav1.UpdateOptions{}) + _, err = namespacedClient.UpdateStatus(ctx, appliedObj, metav1.UpdateOptions{}) if err != nil { return err } @@ -386,7 +426,10 @@ func ResourceGroupCRDApplied(factory cmdutil.Factory) bool { // ResourceGroupCRDMatched checks if the ResourceGroup CRD // in the cluster matches the CRD in the kpt binary. -func ResourceGroupCRDMatched(factory cmdutil.Factory) bool { +// +// ctx is used for the live cluster Get call; cancelling it (e.g. via +// Ctrl-C or a command-level timeout) aborts the check. +func ResourceGroupCRDMatched(ctx context.Context, factory cmdutil.Factory) bool { mapper, err := factory.ToRESTMapper() if err != nil { klog.V(4).Infof("error retrieving RESTMapper when checking ResourceGroup CRD: %s\n", err) @@ -410,7 +453,7 @@ func ResourceGroupCRDMatched(factory cmdutil.Factory) bool { return false } - liveCRD, err := dc.Resource(mapping.Resource).Get(context.TODO(), "resourcegroups.kpt.dev", metav1.GetOptions{ + liveCRD, err := dc.Resource(mapping.Resource).Get(ctx, "resourcegroups.kpt.dev", metav1.GetOptions{ TypeMeta: metav1.TypeMeta{ APIVersion: crd.GetAPIVersion(), Kind: "CustomResourceDefinition", diff --git a/pkg/live/inventoryrg_test.go b/pkg/live/inventoryrg_test.go index 9e72668732..9bc42467a3 100644 --- a/pkg/live/inventoryrg_test.go +++ b/pkg/live/inventoryrg_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 The kpt Authors +// Copyright 2020,2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,6 +15,7 @@ package live import ( + "context" "testing" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -240,3 +241,74 @@ func TestIsResourceGroupInventory(t *testing.T) { }) } } + +// TestWrapInventoryObjWithContext_StoresContext proves the new factory +// threads the caller's context into the InventoryResourceGroup struct. +// This is the mechanism the Apply / ApplyWithPrune methods use to honor +// Ctrl-C / caller timeouts instead of the old context.TODO() behavior. +func TestWrapInventoryObjWithContext_StoresContext(t *testing.T) { + type ctxKey struct{} + ctx := context.WithValue(context.Background(), ctxKey{}, "propagated") + + storage := WrapInventoryObjWithContext(ctx)(inventoryObj) + icm, ok := storage.(*InventoryResourceGroup) + if !ok { + t.Fatalf("WrapInventoryObjWithContext produced unexpected type %T", storage) + } + if icm.ctx == nil { + t.Fatal("expected ctx on InventoryResourceGroup; got nil") + } + if got := icm.ctx.Value(ctxKey{}); got != "propagated" { + t.Fatalf("expected stored ctx to carry propagated value; got %v", got) + } +} + +// TestWrapInventoryObj_LeavesContextNil confirms the legacy wrapper keeps +// ctx nil so contextOrBackground falls back to context.Background() — +// preserving the pre-refactor behavior for callers that haven't migrated. +func TestWrapInventoryObj_LeavesContextNil(t *testing.T) { + storage := WrapInventoryObj(inventoryObj) + icm, ok := storage.(*InventoryResourceGroup) + if !ok { + t.Fatalf("WrapInventoryObj produced unexpected type %T", storage) + } + if icm.ctx != nil { + t.Fatalf("expected legacy wrapper to leave ctx nil; got %v", icm.ctx) + } +} + +// TestContextOrBackground covers both the override path (caller-supplied +// ctx is returned verbatim, including cancellation state) and the +// fallback path (nil ctx becomes context.Background()). +func TestContextOrBackground(t *testing.T) { + t.Run("returns stored ctx when set", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + icm := &InventoryResourceGroup{ctx: ctx} + + got := icm.contextOrBackground() + if got != ctx { + t.Fatalf("expected contextOrBackground to return the stored ctx") + } + // Cancellation on the original ctx must be visible through the + // returned ctx — proof the value isn't copied or unwrapped. + cancel() + select { + case <-got.Done(): + // expected + default: + t.Fatalf("returned ctx did not observe cancellation of the stored ctx") + } + }) + + t.Run("falls back to Background when nil", func(t *testing.T) { + icm := &InventoryResourceGroup{} + got := icm.contextOrBackground() + if got == nil { + t.Fatal("contextOrBackground returned nil; expected context.Background()") + } + // Background() never cancels; Done() returns a nil channel. + if got.Done() != nil { + t.Fatalf("expected Background-equivalent ctx; Done channel was not nil") + } + }) +} diff --git a/pkg/live/planner/cluster.go b/pkg/live/planner/cluster.go index 8a1760be2a..5c40e00a79 100644 --- a/pkg/live/planner/cluster.go +++ b/pkg/live/planner/cluster.go @@ -47,13 +47,17 @@ type ClusterPlanner struct { resourceFetcher ResourceFetcher } -func NewClusterPlanner(f util.Factory) (*ClusterPlanner, error) { +// NewClusterPlanner builds a ClusterPlanner. ctx is the caller's context +// and is plumbed into the inventory wrapper so Apply / ApplyWithPrune on +// the underlying ResourceGroup honor caller cancellation (Ctrl-C, +// deadlines). +func NewClusterPlanner(ctx context.Context, f util.Factory) (*ClusterPlanner, error) { fetcher, err := NewResourceFetcher(f) if err != nil { return nil, err } - invClient, err := inventory.NewClient(f, live.WrapInventoryObj, live.InvToUnstructuredFunc, inventory.StatusPolicyNone, live.ResourceGroupGVK) + invClient, err := inventory.NewClient(f, live.WrapInventoryObjWithContext(ctx), live.InvToUnstructuredFunc, inventory.StatusPolicyNone, live.ResourceGroupGVK) if err != nil { return nil, err } From a1199f9ef801c4ac2afa3624b40ebc139f426096 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Fri, 24 Apr 2026 23:58:23 +0530 Subject: [PATCH 2/5] fix(live): propagate caller ctx into ResourceGroup inventory ops Apply/ApplyWithPrune/ResourceGroupCRDMatched passed context.TODO(), so Ctrl-C and command-level timeouts could not cancel in-flight apply/destroy/plan calls. Carry ctx on the InventoryResourceGroup struct and inject it via new WrapInventoryObjWithContext, NewClusterClientFactoryWithContext, ResourceGroupCRDMatchedWithContext, and NewClusterPlannerWithContext. Legacy entry points kept as thin back-compat wrappers; nil ctx normalized to Background(). Signed-off-by: Jaisheesh-2006 --- commands/alpha/live/plan/command.go | 2 +- commands/live/apply/cmdapply.go | 2 +- pkg/live/inventoryrg.go | 29 +++++++++++++++---- pkg/live/inventoryrg_test.go | 45 +++++++++++++++++++++++++++++ pkg/live/planner/cluster.go | 22 ++++++++++---- pkg/live/planner/cluster_test.go | 25 ++++++++++++++++ 6 files changed, 113 insertions(+), 12 deletions(-) diff --git a/commands/alpha/live/plan/command.go b/commands/alpha/live/plan/command.go index 2f73715683..843958831c 100644 --- a/commands/alpha/live/plan/command.go +++ b/commands/alpha/live/plan/command.go @@ -133,7 +133,7 @@ func (r *Runner) RunE(c *cobra.Command, args []string) error { } // Create and execute the planner. - planner, err := kptplanner.NewClusterPlanner(r.ctx, r.factory) + planner, err := kptplanner.NewClusterPlannerWithContext(r.ctx, r.factory) if err != nil { return err } diff --git a/commands/live/apply/cmdapply.go b/commands/live/apply/cmdapply.go index 37a58059c9..70ed253c77 100644 --- a/commands/live/apply/cmdapply.go +++ b/commands/live/apply/cmdapply.go @@ -228,7 +228,7 @@ func runApply(r *Runner, invInfo inventory.Info, objs []*unstructured.Unstructur if err = cmdutil.InstallResourceGroupCRD(r.ctx, f); err != nil { return err } - } else if !live.ResourceGroupCRDMatched(r.ctx, f) { + } else if !live.ResourceGroupCRDMatchedWithContext(r.ctx, f) { if err = cmdutil.InstallResourceGroupCRD(r.ctx, f); err != nil { return &cmdutil.ResourceGroupCRDNotLatestError{ Err: err, diff --git a/pkg/live/inventoryrg.go b/pkg/live/inventoryrg.go index c8c0f94162..2e798c82fe 100644 --- a/pkg/live/inventoryrg.go +++ b/pkg/live/inventoryrg.go @@ -109,9 +109,15 @@ func WrapInventoryObj(obj *unstructured.Unstructured) inventory.Storage { // inventory.NewClient's WrapObjFunc parameter. The returned function // produces an InventoryResourceGroup that carries ctx, so subsequent // Apply / ApplyWithPrune calls honor the caller's cancellation and -// timeout. ctx MUST NOT be nil; pass context.Background() explicitly if -// you truly want no cancellation. +// timeout. +// +// If ctx is nil it is normalized to context.Background() so callers +// cannot accidentally trigger a nil-deref inside client-go. Prefer +// passing a real ctx (e.g. cmd.Context()) to actually gain cancellation. func WrapInventoryObjWithContext(ctx context.Context) func(*unstructured.Unstructured) inventory.Storage { + if ctx == nil { + ctx = context.Background() + } return func(obj *unstructured.Unstructured) inventory.Storage { if obj != nil { klog.V(4).Infof("wrapping Inventory obj with ctx: %s/%s\n", obj.GetNamespace(), obj.GetName()) @@ -427,9 +433,22 @@ func ResourceGroupCRDApplied(factory cmdutil.Factory) bool { // ResourceGroupCRDMatched checks if the ResourceGroup CRD // in the cluster matches the CRD in the kpt binary. // -// ctx is used for the live cluster Get call; cancelling it (e.g. via -// Ctrl-C or a command-level timeout) aborts the check. -func ResourceGroupCRDMatched(ctx context.Context, factory cmdutil.Factory) bool { +// This signature is preserved for backward compatibility with external +// callers; it delegates to ResourceGroupCRDMatchedWithContext with +// context.Background(). Prefer ResourceGroupCRDMatchedWithContext when +// you have a caller context so Ctrl-C and timeouts can abort the check. +func ResourceGroupCRDMatched(factory cmdutil.Factory) bool { + return ResourceGroupCRDMatchedWithContext(context.Background(), factory) +} + +// ResourceGroupCRDMatchedWithContext is the context-aware variant of +// ResourceGroupCRDMatched. ctx is used for the live cluster Get call; +// cancelling it (e.g. via Ctrl-C or a command-level timeout) aborts the +// check. A nil ctx is normalized to context.Background(). +func ResourceGroupCRDMatchedWithContext(ctx context.Context, factory cmdutil.Factory) bool { + if ctx == nil { + ctx = context.Background() + } mapper, err := factory.ToRESTMapper() if err != nil { klog.V(4).Infof("error retrieving RESTMapper when checking ResourceGroup CRD: %s\n", err) diff --git a/pkg/live/inventoryrg_test.go b/pkg/live/inventoryrg_test.go index 9bc42467a3..b7b62a88c2 100644 --- a/pkg/live/inventoryrg_test.go +++ b/pkg/live/inventoryrg_test.go @@ -20,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "sigs.k8s.io/cli-utils/pkg/apis/actuation" "sigs.k8s.io/cli-utils/pkg/common" "sigs.k8s.io/cli-utils/pkg/inventory" @@ -277,6 +278,50 @@ func TestWrapInventoryObj_LeavesContextNil(t *testing.T) { } } +// TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground proves the +// factory is nil-safe: passing a nil ctx cannot produce a wrapper that +// would nil-deref inside client-go. The stored ctx is normalized to +// context.Background() at factory construction time. +func TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground(t *testing.T) { + //nolint:staticcheck // SA1012: deliberately passing a nil context to exercise the nil-safety guard. + storage := WrapInventoryObjWithContext(nil)(inventoryObj) + icm, ok := storage.(*InventoryResourceGroup) + if !ok { + t.Fatalf("WrapInventoryObjWithContext(nil) produced unexpected type %T", storage) + } + if icm.ctx == nil { + t.Fatal("expected nil ctx to be normalized to Background(); got nil") + } + // Background() never cancels; Done() returns a nil channel. + if icm.ctx.Done() != nil { + t.Fatalf("expected Background()-equivalent ctx; Done() returned non-nil") + } +} + +// TestResourceGroupCRDMatched_BackCompatSignaturePreserved is a +// compile-time guard that the legacy ResourceGroupCRDMatched(factory) +// signature is still exported, alongside the new context-aware +// ResourceGroupCRDMatchedWithContext(ctx, factory). If either function +// is renamed, removed, or has its signature changed, this test stops +// compiling and the API-compat break is visible immediately. +// +// Uses typed anonymous-function parameters so the compiler verifies +// signature assignability. This pattern is deliberate — staticcheck's +// QF1011 would otherwise suggest removing a `var _ T = fn` type +// annotation, which would silently destroy the guarantee. +// +// We don't invoke the functions because both require a live +// cmdutil.Factory; their runtime behavior is exercised by the +// apply/destroy e2e tests. +func TestResourceGroupCRDMatched_BackCompatSignaturePreserved(t *testing.T) { + pinSignatures := func( + _ func(cmdutil.Factory) bool, + _ func(context.Context, cmdutil.Factory) bool, + ) { + } + pinSignatures(ResourceGroupCRDMatched, ResourceGroupCRDMatchedWithContext) +} + // TestContextOrBackground covers both the override path (caller-supplied // ctx is returned verbatim, including cancellation state) and the // fallback path (nil ctx becomes context.Background()). diff --git a/pkg/live/planner/cluster.go b/pkg/live/planner/cluster.go index 5c40e00a79..e537c2b6aa 100644 --- a/pkg/live/planner/cluster.go +++ b/pkg/live/planner/cluster.go @@ -47,11 +47,23 @@ type ClusterPlanner struct { resourceFetcher ResourceFetcher } -// NewClusterPlanner builds a ClusterPlanner. ctx is the caller's context -// and is plumbed into the inventory wrapper so Apply / ApplyWithPrune on -// the underlying ResourceGroup honor caller cancellation (Ctrl-C, -// deadlines). -func NewClusterPlanner(ctx context.Context, f util.Factory) (*ClusterPlanner, error) { +// NewClusterPlanner builds a ClusterPlanner using context.Background() +// for the underlying inventory-client cluster calls. +// +// This signature is preserved for backward compatibility with external +// callers; it delegates to NewClusterPlannerWithContext. Prefer the +// context-aware constructor when you have a caller context so Ctrl-C +// and command-level timeouts can cancel inventory I/O. +func NewClusterPlanner(f util.Factory) (*ClusterPlanner, error) { + return NewClusterPlannerWithContext(context.Background(), f) +} + +// NewClusterPlannerWithContext is the context-aware variant of +// NewClusterPlanner. ctx is plumbed into the inventory wrapper so +// Apply / ApplyWithPrune on the underlying ResourceGroup honor caller +// cancellation (Ctrl-C, deadlines). A nil ctx is normalized to +// context.Background() by WrapInventoryObjWithContext. +func NewClusterPlannerWithContext(ctx context.Context, f util.Factory) (*ClusterPlanner, error) { fetcher, err := NewResourceFetcher(f) if err != nil { return nil, err diff --git a/pkg/live/planner/cluster_test.go b/pkg/live/planner/cluster_test.go index 71699c38b5..bdd5f2d280 100644 --- a/pkg/live/planner/cluster_test.go +++ b/pkg/live/planner/cluster_test.go @@ -21,6 +21,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/kubectl/pkg/cmd/util" "sigs.k8s.io/cli-utils/pkg/apply" "sigs.k8s.io/cli-utils/pkg/apply/event" "sigs.k8s.io/cli-utils/pkg/inventory" @@ -166,3 +167,27 @@ func (fii *FakeInventoryInfo) ID() string { func (fii *FakeInventoryInfo) Strategy() inventory.Strategy { return inventory.NameStrategy } + +// TestNewClusterPlanner_BackCompatSignaturePreserved is a compile-time +// guard that both the legacy NewClusterPlanner(f) entry point and the +// new context-aware NewClusterPlannerWithContext(ctx, f) remain +// exported with their current signatures. If either is renamed, +// removed, or has its parameter list changed, this test stops +// compiling and the API-compat break is visible immediately. +// +// Uses typed anonymous-function parameters so the compiler verifies +// signature assignability. This pattern is deliberate — staticcheck's +// QF1011 would otherwise suggest removing a `var _ T = fn` type +// annotation, which would silently destroy the guarantee. +// +// Runtime behavior of both constructors is exercised through the +// command-level tests that instantiate the planner via the real +// factory in commands/alpha/live/plan. +func TestNewClusterPlanner_BackCompatSignaturePreserved(t *testing.T) { + pinSignatures := func( + _ func(util.Factory) (*ClusterPlanner, error), + _ func(context.Context, util.Factory) (*ClusterPlanner, error), + ) { + } + pinSignatures(NewClusterPlanner, NewClusterPlannerWithContext) +} From e333d2f95b8c60c2ad3a994295d882b958dd2ff7 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Sat, 25 Apr 2026 00:45:28 +0530 Subject: [PATCH 3/5] fix(live): normalize nil ctx in ClusterClientFactory Signed-off-by: Jaisheesh-2006 --- pkg/live/inventory-client-factory.go | 19 +++++++++++++++---- pkg/live/inventoryrg.go | 4 ++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/pkg/live/inventory-client-factory.go b/pkg/live/inventory-client-factory.go index 4cc7829659..eaf01f9cc0 100644 --- a/pkg/live/inventory-client-factory.go +++ b/pkg/live/inventory-client-factory.go @@ -43,14 +43,25 @@ func NewClusterClientFactory() *ClusterClientFactory { // NewClusterClientFactoryWithContext returns a ClusterClientFactory that // threads ctx into every inventory client it produces. +// +// A nil ctx is normalized to context.Background() so the docstring's +// promise ("threads ctx into every inventory client") holds for every +// input: there is no hidden code path that silently drops propagation. func NewClusterClientFactoryWithContext(ctx context.Context) *ClusterClientFactory { + if ctx == nil { + ctx = context.Background() + } return &ClusterClientFactory{StatusPolicy: inventory.StatusPolicyNone, Ctx: ctx} } func (ccf *ClusterClientFactory) NewClient(factory cmdutil.Factory) (inventory.Client, error) { - wrap := WrapInventoryObj - if ccf.Ctx != nil { - wrap = WrapInventoryObjWithContext(ccf.Ctx) + // Defense in depth: normalize a nil Ctx here too. This covers the + // case where a caller constructed ClusterClientFactory as a struct + // literal (e.g. &ClusterClientFactory{StatusPolicy: ...}) and left + // Ctx unset — see NewClusterClientFactory, which does exactly that. + ctx := ccf.Ctx + if ctx == nil { + ctx = context.Background() } - return inventory.NewClient(factory, wrap, InvToUnstructuredFunc, ccf.StatusPolicy, ResourceGroupGVK) + return inventory.NewClient(factory, WrapInventoryObjWithContext(ctx), InvToUnstructuredFunc, ccf.StatusPolicy, ResourceGroupGVK) } diff --git a/pkg/live/inventoryrg.go b/pkg/live/inventoryrg.go index 2e798c82fe..db417213ac 100644 --- a/pkg/live/inventoryrg.go +++ b/pkg/live/inventoryrg.go @@ -92,7 +92,7 @@ var _ inventory.Info = &InventoryResourceGroup{} // WrapInventoryObj takes a passed ResourceGroup (as a resource.Info), // wraps it with the InventoryResourceGroup and upcasts the wrapper as -// an the Inventory interface. +// the Inventory interface. // // The wrapped inventory will use context.Background() for cluster API // calls. Prefer WrapInventoryObjWithContext when you have a caller @@ -302,7 +302,7 @@ func (icm *InventoryResourceGroup) Apply(dc dynamic.Interface, mapper meta.RESTM } ctx := icm.contextOrBackground() - // Get cluster object, if exsists. + // Get cluster object, if exists. clusterObj, err := namespacedClient.Get(ctx, invInfo.GetName(), metav1.GetOptions{}) if err != nil && !apierrors.IsNotFound(err) { return err From 557b0a94011927b072234171ddb53623a370335f Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 7 May 2026 22:56:45 +0530 Subject: [PATCH 4/5] Docs : Made the doc comments concise Signed-off-by: Jaisheesh-2006 --- pkg/live/inventory-client-factory_test.go | 60 +++++++++++ pkg/live/inventoryrg.go | 50 ++------- pkg/live/inventoryrg_test.go | 125 +++++----------------- pkg/live/planner/cluster_test.go | 22 +--- 4 files changed, 102 insertions(+), 155 deletions(-) create mode 100644 pkg/live/inventory-client-factory_test.go diff --git a/pkg/live/inventory-client-factory_test.go b/pkg/live/inventory-client-factory_test.go new file mode 100644 index 0000000000..c4c4346cfc --- /dev/null +++ b/pkg/live/inventory-client-factory_test.go @@ -0,0 +1,60 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package live + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestNewClusterClientFactoryWithContext_NilIsNormalized verifies nil ctx is +// normalized to Background(). +func TestNewClusterClientFactoryWithContext_NilIsNormalized(t *testing.T) { + //nolint:staticcheck // SA1012: deliberately passing nil to exercise the nil-safety guard. + ccf := NewClusterClientFactoryWithContext(nil) + require.NotNil(t, ccf, "NewClusterClientFactoryWithContext returned nil") + require.NotNil(t, ccf.Ctx, "expected nil ctx to be normalized to Background(); got nil") + // Background() never cancels; Done() returns a nil channel. + require.Nil(t, ccf.Ctx.Done(), "expected Background()-equivalent ctx; Done() returned non-nil") +} + +// TestNewClusterClientFactoryWithContext_PreservesRealCtx stores the caller +// ctx and preserves cancellation. +func TestNewClusterClientFactoryWithContext_PreservesRealCtx(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + ccf := NewClusterClientFactoryWithContext(ctx) + require.Same(t, ctx, ccf.Ctx, "factory stored a different ctx than the one passed in") + assert.NotNil(t, ccf.Ctx.Done(), "ctx should expose Done channel") + cancel() + select { + case <-ccf.Ctx.Done(): + default: + require.FailNow(t, "factory ctx did not observe cancellation of the source ctx") + } +} + +// TestNewClusterClientFactory_StructLiteralPathTolerated keeps legacy nil ctx. +func TestNewClusterClientFactory_StructLiteralPathTolerated(t *testing.T) { + // Exercises the legacy constructor which leaves Ctx unset. + ccf := NewClusterClientFactory() + require.Nil(t, ccf.Ctx, "legacy constructor must not synthesize a ctx; callers rely on nil to signal opt-out") + // We don't call ccf.NewClient here because it needs a real + // cmdutil.Factory; the observable contract is that inside NewClient + // the nil Ctx is normalized to Background(). That path is exercised + // in the existing apply/destroy tests via the CLI integration tests. +} diff --git a/pkg/live/inventoryrg.go b/pkg/live/inventoryrg.go index db417213ac..a2ac526ed9 100644 --- a/pkg/live/inventoryrg.go +++ b/pkg/live/inventoryrg.go @@ -56,15 +56,8 @@ var ResourceGroupGVK = schema.GroupVersionKind{ Kind: "ResourceGroup", } -// InventoryResourceGroup wraps a ResourceGroup resource and implements -// the Inventory and InventoryInfo interface. This wrapper loads and stores the -// object metadata (inventory) to and from the wrapped ResourceGroup. -// -// ctx, if non-nil, is the caller's context and is used for the live -// Kubernetes API calls performed by Apply / ApplyWithPrune. It exists on -// the struct (rather than as a parameter) because the upstream -// inventory.Storage interface signatures do not include a context; see -// WrapInventoryObjWithContext. +// InventoryResourceGroup wraps a ResourceGroup inventory and carries a caller +// context for API calls. type InventoryResourceGroup struct { ctx context.Context inv *unstructured.Unstructured @@ -72,10 +65,7 @@ type InventoryResourceGroup struct { objStatus []actuation.ObjectStatus } -// contextOrBackground returns the caller-supplied context if set, or -// context.Background() otherwise. Callers going through -// WrapInventoryObjWithContext get real cancellation/timeout propagation; -// legacy callers using WrapInventoryObj continue to work unchanged. +// contextOrBackground returns ctx when set, otherwise Background(). func (icm *InventoryResourceGroup) contextOrBackground() context.Context { if icm.ctx != nil { return icm.ctx @@ -90,14 +80,7 @@ func (icm *InventoryResourceGroup) Strategy() inventory.Strategy { var _ inventory.Storage = &InventoryResourceGroup{} var _ inventory.Info = &InventoryResourceGroup{} -// WrapInventoryObj takes a passed ResourceGroup (as a resource.Info), -// wraps it with the InventoryResourceGroup and upcasts the wrapper as -// the Inventory interface. -// -// The wrapped inventory will use context.Background() for cluster API -// calls. Prefer WrapInventoryObjWithContext when you have a caller -// context (e.g. cmd.Context()) so that Ctrl-C and caller-side timeouts -// actually cancel the in-flight request. +// WrapInventoryObj wraps inventory and uses Background() for API calls. func WrapInventoryObj(obj *unstructured.Unstructured) inventory.Storage { if obj != nil { klog.V(4).Infof("wrapping Inventory obj: %s/%s\n", obj.GetNamespace(), obj.GetName()) @@ -105,15 +88,8 @@ func WrapInventoryObj(obj *unstructured.Unstructured) inventory.Storage { return &InventoryResourceGroup{inv: obj} } -// WrapInventoryObjWithContext returns a wrapper function compatible with -// inventory.NewClient's WrapObjFunc parameter. The returned function -// produces an InventoryResourceGroup that carries ctx, so subsequent -// Apply / ApplyWithPrune calls honor the caller's cancellation and -// timeout. -// -// If ctx is nil it is normalized to context.Background() so callers -// cannot accidentally trigger a nil-deref inside client-go. Prefer -// passing a real ctx (e.g. cmd.Context()) to actually gain cancellation. +// WrapInventoryObjWithContext returns a WrapObjFunc that stores ctx. +// Nil ctx is normalized to Background(). func WrapInventoryObjWithContext(ctx context.Context) func(*unstructured.Unstructured) inventory.Storage { if ctx == nil { ctx = context.Background() @@ -430,21 +406,13 @@ func ResourceGroupCRDApplied(factory cmdutil.Factory) bool { return true } -// ResourceGroupCRDMatched checks if the ResourceGroup CRD -// in the cluster matches the CRD in the kpt binary. -// -// This signature is preserved for backward compatibility with external -// callers; it delegates to ResourceGroupCRDMatchedWithContext with -// context.Background(). Prefer ResourceGroupCRDMatchedWithContext when -// you have a caller context so Ctrl-C and timeouts can abort the check. +// ResourceGroupCRDMatched checks the cluster CRD using Background(). func ResourceGroupCRDMatched(factory cmdutil.Factory) bool { return ResourceGroupCRDMatchedWithContext(context.Background(), factory) } -// ResourceGroupCRDMatchedWithContext is the context-aware variant of -// ResourceGroupCRDMatched. ctx is used for the live cluster Get call; -// cancelling it (e.g. via Ctrl-C or a command-level timeout) aborts the -// check. A nil ctx is normalized to context.Background(). +// ResourceGroupCRDMatchedWithContext checks the cluster CRD using ctx. +// Nil ctx is normalized to Background(). func ResourceGroupCRDMatchedWithContext(ctx context.Context, factory cmdutil.Factory) bool { if ctx == nil { ctx = context.Background() diff --git a/pkg/live/inventoryrg_test.go b/pkg/live/inventoryrg_test.go index b7b62a88c2..36439df142 100644 --- a/pkg/live/inventoryrg_test.go +++ b/pkg/live/inventoryrg_test.go @@ -18,6 +18,8 @@ import ( "context" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" cmdutil "k8s.io/kubectl/pkg/cmd/util" @@ -157,31 +159,17 @@ func TestLoadStore(t *testing.T) { _ = wrapped.Store(tc.objs, tc.objStatus) invStored, err := wrapped.GetObject() if tc.isError { - if err == nil { - t.Fatalf("expected error but received none") - } - return - } - if !tc.isError && err != nil { - t.Fatalf("unexpected error %v received", err) + require.Error(t, err) return } + require.NoError(t, err) wrapped = WrapInventoryObj(invStored) objs, err := wrapped.Load() - if !tc.isError && err != nil { - t.Fatalf("unexpected error %v received", err) - return - } - if !objs.Equal(tc.objs) { - t.Fatalf("expected inventory objs (%v), got (%v)", tc.objs, objs) - } + require.NoError(t, err) + require.True(t, objs.Equal(tc.objs), "expected inventory objs (%v), got (%v)", tc.objs, objs) resourceStatus, _, err := unstructured.NestedSlice(invStored.Object, "status", "resourceStatuses") - if err != nil { - t.Fatalf("unexpected error %v received", err) - } - if len(resourceStatus) != len(tc.objStatus) { - t.Fatalf("expected %d resource status but got %d", len(tc.objStatus), len(resourceStatus)) - } + require.NoError(t, err) + require.Len(t, resourceStatus, len(tc.objStatus), "expected %d resource status but got %d", len(tc.objStatus), len(resourceStatus)) }) } } @@ -227,92 +215,47 @@ func TestIsResourceGroupInventory(t *testing.T) { t.Run(name, func(t *testing.T) { actual, err := IsResourceGroupInventory(tc.invObj) if tc.isError { - if err == nil { - t.Fatalf("expected error but received none") - } - return - } - if !tc.isError && err != nil { - t.Fatalf("unexpected error %v received", err) + require.Error(t, err) return } - if tc.expected != actual { - t.Errorf("expected inventory as (%t), got (%t)", tc.expected, actual) - } + require.NoError(t, err) + assert.Equal(t, tc.expected, actual, "expected inventory as (%t), got (%t)", tc.expected, actual) }) } } -// TestWrapInventoryObjWithContext_StoresContext proves the new factory -// threads the caller's context into the InventoryResourceGroup struct. -// This is the mechanism the Apply / ApplyWithPrune methods use to honor -// Ctrl-C / caller timeouts instead of the old context.TODO() behavior. +// TestWrapInventoryObjWithContext_StoresContext stores ctx on the wrapper. func TestWrapInventoryObjWithContext_StoresContext(t *testing.T) { type ctxKey struct{} ctx := context.WithValue(context.Background(), ctxKey{}, "propagated") storage := WrapInventoryObjWithContext(ctx)(inventoryObj) icm, ok := storage.(*InventoryResourceGroup) - if !ok { - t.Fatalf("WrapInventoryObjWithContext produced unexpected type %T", storage) - } - if icm.ctx == nil { - t.Fatal("expected ctx on InventoryResourceGroup; got nil") - } - if got := icm.ctx.Value(ctxKey{}); got != "propagated" { - t.Fatalf("expected stored ctx to carry propagated value; got %v", got) - } + require.True(t, ok, "WrapInventoryObjWithContext produced unexpected type %T", storage) + require.NotNil(t, icm.ctx, "expected ctx on InventoryResourceGroup; got nil") + require.Equal(t, "propagated", icm.ctx.Value(ctxKey{}), "expected stored ctx to carry propagated value") } -// TestWrapInventoryObj_LeavesContextNil confirms the legacy wrapper keeps -// ctx nil so contextOrBackground falls back to context.Background() — -// preserving the pre-refactor behavior for callers that haven't migrated. +// TestWrapInventoryObj_LeavesContextNil keeps legacy nil ctx. func TestWrapInventoryObj_LeavesContextNil(t *testing.T) { storage := WrapInventoryObj(inventoryObj) icm, ok := storage.(*InventoryResourceGroup) - if !ok { - t.Fatalf("WrapInventoryObj produced unexpected type %T", storage) - } - if icm.ctx != nil { - t.Fatalf("expected legacy wrapper to leave ctx nil; got %v", icm.ctx) - } + require.True(t, ok, "WrapInventoryObj produced unexpected type %T", storage) + require.Nil(t, icm.ctx, "expected legacy wrapper to leave ctx nil; got %v", icm.ctx) } -// TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground proves the -// factory is nil-safe: passing a nil ctx cannot produce a wrapper that -// would nil-deref inside client-go. The stored ctx is normalized to -// context.Background() at factory construction time. +// TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground normalizes nil ctx. func TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground(t *testing.T) { //nolint:staticcheck // SA1012: deliberately passing a nil context to exercise the nil-safety guard. storage := WrapInventoryObjWithContext(nil)(inventoryObj) icm, ok := storage.(*InventoryResourceGroup) - if !ok { - t.Fatalf("WrapInventoryObjWithContext(nil) produced unexpected type %T", storage) - } - if icm.ctx == nil { - t.Fatal("expected nil ctx to be normalized to Background(); got nil") - } + require.True(t, ok, "WrapInventoryObjWithContext(nil) produced unexpected type %T", storage) + require.NotNil(t, icm.ctx, "expected nil ctx to be normalized to Background(); got nil") // Background() never cancels; Done() returns a nil channel. - if icm.ctx.Done() != nil { - t.Fatalf("expected Background()-equivalent ctx; Done() returned non-nil") - } + require.Nil(t, icm.ctx.Done(), "expected Background()-equivalent ctx; Done() returned non-nil") } -// TestResourceGroupCRDMatched_BackCompatSignaturePreserved is a -// compile-time guard that the legacy ResourceGroupCRDMatched(factory) -// signature is still exported, alongside the new context-aware -// ResourceGroupCRDMatchedWithContext(ctx, factory). If either function -// is renamed, removed, or has its signature changed, this test stops -// compiling and the API-compat break is visible immediately. -// -// Uses typed anonymous-function parameters so the compiler verifies -// signature assignability. This pattern is deliberate — staticcheck's -// QF1011 would otherwise suggest removing a `var _ T = fn` type -// annotation, which would silently destroy the guarantee. -// -// We don't invoke the functions because both require a live -// cmdutil.Factory; their runtime behavior is exercised by the -// apply/destroy e2e tests. +// TestResourceGroupCRDMatched_BackCompatSignaturePreserved pins exported signatures. func TestResourceGroupCRDMatched_BackCompatSignaturePreserved(t *testing.T) { pinSignatures := func( _ func(cmdutil.Factory) bool, @@ -322,38 +265,28 @@ func TestResourceGroupCRDMatched_BackCompatSignaturePreserved(t *testing.T) { pinSignatures(ResourceGroupCRDMatched, ResourceGroupCRDMatchedWithContext) } -// TestContextOrBackground covers both the override path (caller-supplied -// ctx is returned verbatim, including cancellation state) and the -// fallback path (nil ctx becomes context.Background()). +// TestContextOrBackground covers stored ctx and fallback behavior. func TestContextOrBackground(t *testing.T) { t.Run("returns stored ctx when set", func(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) icm := &InventoryResourceGroup{ctx: ctx} got := icm.contextOrBackground() - if got != ctx { - t.Fatalf("expected contextOrBackground to return the stored ctx") - } - // Cancellation on the original ctx must be visible through the - // returned ctx — proof the value isn't copied or unwrapped. + require.Same(t, ctx, got, "expected contextOrBackground to return the stored ctx") + // Cancellation on the original ctx must be visible through the returned ctx. cancel() select { case <-got.Done(): - // expected default: - t.Fatalf("returned ctx did not observe cancellation of the stored ctx") + require.FailNow(t, "returned ctx did not observe cancellation of the stored ctx") } }) t.Run("falls back to Background when nil", func(t *testing.T) { icm := &InventoryResourceGroup{} got := icm.contextOrBackground() - if got == nil { - t.Fatal("contextOrBackground returned nil; expected context.Background()") - } + require.NotNil(t, got, "contextOrBackground returned nil; expected context.Background()") // Background() never cancels; Done() returns a nil channel. - if got.Done() != nil { - t.Fatalf("expected Background-equivalent ctx; Done channel was not nil") - } + require.Nil(t, got.Done(), "expected Background-equivalent ctx; Done channel was not nil") }) } diff --git a/pkg/live/planner/cluster_test.go b/pkg/live/planner/cluster_test.go index bdd5f2d280..f72a43f035 100644 --- a/pkg/live/planner/cluster_test.go +++ b/pkg/live/planner/cluster_test.go @@ -19,6 +19,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/kubectl/pkg/cmd/util" @@ -113,9 +114,8 @@ func TestClusterPlanner(t *testing.T) { }).BuildPlan(ctx, &FakeInventoryInfo{}, []*unstructured.Unstructured{}, Options{}) require.NoError(t, err) - if diff := cmp.Diff(tc.expectedPlan, plan); diff != "" { - t.Errorf("plan mismatch (-want +got):\n%s", diff) - } + diff := cmp.Diff(tc.expectedPlan, plan) + assert.Empty(t, diff, "plan mismatch (-want +got):\n%s", diff) }) } } @@ -168,21 +168,7 @@ func (fii *FakeInventoryInfo) Strategy() inventory.Strategy { return inventory.NameStrategy } -// TestNewClusterPlanner_BackCompatSignaturePreserved is a compile-time -// guard that both the legacy NewClusterPlanner(f) entry point and the -// new context-aware NewClusterPlannerWithContext(ctx, f) remain -// exported with their current signatures. If either is renamed, -// removed, or has its parameter list changed, this test stops -// compiling and the API-compat break is visible immediately. -// -// Uses typed anonymous-function parameters so the compiler verifies -// signature assignability. This pattern is deliberate — staticcheck's -// QF1011 would otherwise suggest removing a `var _ T = fn` type -// annotation, which would silently destroy the guarantee. -// -// Runtime behavior of both constructors is exercised through the -// command-level tests that instantiate the planner via the real -// factory in commands/alpha/live/plan. +// TestNewClusterPlanner_BackCompatSignaturePreserved pins exported signatures. func TestNewClusterPlanner_BackCompatSignaturePreserved(t *testing.T) { pinSignatures := func( _ func(util.Factory) (*ClusterPlanner, error), From d63fb91bc3e41292c27b0b801a9dccff450679f7 Mon Sep 17 00:00:00 2001 From: Jaisheesh-2006 Date: Thu, 7 May 2026 23:11:22 +0530 Subject: [PATCH 5/5] Fix : Passed r.ctx into destroyer.Run Signed-off-by: Jaisheesh-2006 --- commands/live/destroy/cmddestroy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commands/live/destroy/cmddestroy.go b/commands/live/destroy/cmddestroy.go index 9bee3f94b4..3d37503f5b 100644 --- a/commands/live/destroy/cmddestroy.go +++ b/commands/live/destroy/cmddestroy.go @@ -192,7 +192,7 @@ func runDestroy(r *Runner, inv inventory.Info, dryRunStrategy common.DryRunStrat DryRunStrategy: dryRunStrategy, EmitStatusEvents: true, } - ch := destroyer.Run(context.Background(), inv, options) + ch := destroyer.Run(r.ctx, inv, options) // Print the preview strategy unless the output format is json. if dryRunStrategy.ClientOrServerDryRun() && r.output != printers.JSONPrinter {