From 7a61c9a5bdfca99328da38f7d72117ee76757f88 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 14 May 2026 22:41:47 +0800 Subject: [PATCH] Reduce code duplication across rpkg CLI commands Extract shared helpers to pkg/cli/commands/rpkg/util/common.go and apply them across the seven rpkg lifecycle commands (approve, propose, proposedelete, reject, del, pull, push): - InitClient: wraps cliutils.CreateClientWithFlags with namespace flag validation. Previously only the get command checked for an empty --namespace flag; this now applies to approve, propose, proposedelete, reject and del as well. Also rejects a nil cfg up front so callers cannot trigger a panic inside ToRESTConfig. - CreateScheme: consolidates the identical local createScheme() function that was duplicated in both pull and push, registering porchapi, porchconfig, corev1 and metav1 so callers see the same kinds as cliutils.CreateClientWithFlags. - MakePreRunE: returns a cobra PreRunE closure that validates namespace and creates the client. Eliminates duplicate preRunE methods from approve, propose, proposedelete and del. - RunForEachPackage: extracts the retry-with-error-collection loop shared by approve, propose, proposedelete and reject. Each command now provides only its lifecycle-specific logic as a callback. The lastErr workaround for k8s retry library behaviour is included in the shared helper. Per-iteration body is further split into fetchAndAct and reportResult helpers for clarity. Behavioural options are grouped into RunForEachOpts; CmdName is required and rejected up front if empty. - Runner struct + NewTestRunner: shared fields (Ctx, Cfg, Client, Command) embedded into each command's runner. NewTestRunner builds a Runner pre-wired for table-driven CLI tests. Ctx is intentionally stored on the struct, matching the existing per-command runner convention (see SonarQube godre:S8242 -- documented rather than refactored). The seven lifecycle command files (approve, propose, proposedelete, reject, del, pull, push) all embed rpkgutil.Runner. pull and push retain their printer field on top of the embed. Behavioural improvements that surfaced during the refactor: - approve and propose-delete now reject empty argument invocations with "PACKAGE is a required positional argument", matching reject's existing behaviour. The reject error message itself is aligned to say PACKAGE rather than PACKAGE_REVISION, matching the Use string. - reject's DeletionProposed -> Published path now uses UpdatePackageRevisionApproval like the Proposed -> Draft path, resolving the inconsistency kispaljr's NOTE comment had pointed at. - pull, push and reject preRunE gain explicit unit tests that exercise the cfg.ToRESTConfig + CreateScheme + client.New wiring through a kubeconfig fixture, raising coverage on those files from 0% to 76.9% (pull/push) and 87.5% (reject). Fixes #870 Fixes #900 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- pkg/cli/commands/rpkg/approve/command.go | 80 +--- pkg/cli/commands/rpkg/approve/command_test.go | 40 +- pkg/cli/commands/rpkg/del/command.go | 30 +- pkg/cli/commands/rpkg/del/command_test.go | 24 +- pkg/cli/commands/rpkg/propose/command.go | 100 +---- pkg/cli/commands/rpkg/propose/command_test.go | 36 +- .../commands/rpkg/proposedelete/command.go | 90 +--- .../rpkg/proposedelete/command_test.go | 36 +- pkg/cli/commands/rpkg/pull/command.go | 41 +- pkg/cli/commands/rpkg/pull/command_test.go | 60 ++- pkg/cli/commands/rpkg/push/command.go | 43 +- pkg/cli/commands/rpkg/push/command_test.go | 59 ++- pkg/cli/commands/rpkg/reject/command.go | 98 ++--- pkg/cli/commands/rpkg/reject/command_test.go | 91 ++-- pkg/cli/commands/rpkg/util/common.go | 226 ++++++++++ pkg/cli/commands/rpkg/util/common_test.go | 394 ++++++++++++++++++ 16 files changed, 942 insertions(+), 506 deletions(-) create mode 100644 pkg/cli/commands/rpkg/util/common_test.go diff --git a/pkg/cli/commands/rpkg/approve/command.go b/pkg/cli/commands/rpkg/approve/command.go index 049a088e8..5509b214c 100644 --- a/pkg/cli/commands/rpkg/approve/command.go +++ b/pkg/cli/commands/rpkg/approve/command.go @@ -17,15 +17,13 @@ package approve import ( "context" "fmt" - "strings" - "github.com/kptdev/kpt/pkg/lib/errors" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -33,6 +31,9 @@ const ( command = "cmdrpkgapprove" ) +// NewCommand returns the cobra command for `rpkg approve`, which +// transitions a package revision into the Published lifecycle state +// after readiness gates have been satisfied. func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra.Command { v1 := newRunner(ctx, rcg) v2 := newV1Alpha2Runner(ctx, rcg) @@ -42,8 +43,7 @@ func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra. func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ @@ -51,7 +51,7 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner Short: docs.ApproveShort, Long: docs.ApproveShort + "\n" + docs.ApproveLong, Example: docs.ApproveExamples, - PreRunE: r.preRunE, + PreRunE: rpkgutil.MakePreRunE(command+".preRunE", rcg, &r.Client), RunE: r.runE, Hidden: cliutils.HidePorchCommands, } @@ -61,70 +61,18 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command - - // Flags + rpkgutil.Runner } -func (r *runner) preRunE(_ *cobra.Command, _ []string) error { - const op errors.Op = command + ".preRunE" - - client, err := cliutils.CreateClientWithFlags(r.cfg) - if err != nil { - return errors.E(op, err) +func approveAction(ctx context.Context, client client.Client, pr *porchapi.PackageRevision) (string, error) { + if err := cliutils.UpdatePackageRevisionApproval(ctx, client, pr, porchapi.PackageRevisionLifecyclePublished); err != nil { + return "", err } - r.client = client - return nil + return fmt.Sprintf("%s approved", pr.Name), nil } func (r *runner) runE(_ *cobra.Command, args []string) error { - const op errors.Op = command + ".runE" - var messages []string - - namespace := *r.cfg.Namespace - - for _, name := range args { - key := client.ObjectKey{ - Namespace: namespace, - Name: name, - } - var lastErr error - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - var pr porchapi.PackageRevision - err := r.client.Get(r.ctx, key, &pr) - if err != nil { - lastErr = err - return err - } - if !porchapi.PackageRevisionIsReady(pr.Spec.ReadinessGates, pr.Status.Conditions) { - lastErr = fmt.Errorf("readiness conditions not met") - return lastErr - } - err = cliutils.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, porchapi.PackageRevisionLifecyclePublished) - if err != nil { - lastErr = err - } else { - lastErr = nil - } - return err - }) - // Workaround for k8s retry library bug: OnError/RetryOnConflict sometimes returns nil even when errors occur - if err == nil && lastErr != nil { - err = lastErr - } - if err != nil { - messages = append(messages, err.Error()) - fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err) - } else { - fmt.Fprintf(r.Command.OutOrStdout(), "%s approved\n", name) - } - } - if len(messages) > 0 { - return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) - } - - return nil + return rpkgutil.RunForEachPackage(r.Ctx, r.Client, r.Command, *r.Cfg.Namespace, args, + rpkgutil.RunForEachOpts{CmdName: command, WithRetry: true, CheckReadiness: true}, + approveAction) } diff --git a/pkg/cli/commands/rpkg/approve/command_test.go b/pkg/cli/commands/rpkg/approve/command_test.go index 68115f9b3..b1c43159c 100644 --- a/pkg/cli/commands/rpkg/approve/command_test.go +++ b/pkg/cli/commands/rpkg/approve/command_test.go @@ -22,9 +22,9 @@ import ( "github.com/google/go-cmp/cmp" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -32,24 +32,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) -func createScheme() (*runtime.Scheme, error) { - scheme := runtime.NewScheme() - for _, api := range (runtime.SchemeBuilder{ - porchapi.AddToScheme, - }) { - if err := api(scheme); err != nil { - return nil, err - } - } - scheme.AddKnownTypes(porchapi.SchemeGroupVersion, &porchapi.PackageRevision{}) - return scheme, nil -} - func TestCmd(t *testing.T) { pkgRevName := "test-pr" repoName := "test-repo" ns := "ns" - var scheme, err = createScheme() + var scheme, err = rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -104,14 +91,14 @@ func TestCmd(t *testing.T) { RepositoryName: repoName, ReadinessGates: []porchapi.ReadinessGate{ { - ConditionType: "kpt.dev.Specializer.specialize", + ConditionType: "nephio.org.Specializer.specialize", }, }, }, Status: porchapi.PackageRevisionStatus{ Conditions: []porchapi.Condition{ { - Type: "kpt.dev.Specializer.specialize", + Type: "nephio.org.Specializer.specialize", Status: "False", }, }, @@ -195,14 +182,7 @@ func TestCmd(t *testing.T) { os.Stdout = write os.Stderr = write - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{ - Namespace: &ns, - }, - client: tc.fakeclient, - Command: cmd, - } + r := &runner{Runner: rpkgutil.NewTestRunner(ns, tc.fakeclient, cmd)} go func() { defer write.Close() err := r.runE(cmd, []string{pkgRevName}) @@ -227,7 +207,7 @@ func TestCmd(t *testing.T) { // issues happen. The easiest way to trigger this in tests is to use an // unreachable cluster. func TestLastErrWorkaround(t *testing.T) { - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -235,13 +215,7 @@ func TestLastErrWorkaround(t *testing.T) { if err != nil { t.Fatalf("error creating client: %v", err) } - ns := "ns" - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, - client: c, - Command: &cobra.Command{}, - } + r := &runner{Runner: rpkgutil.NewTestRunner("ns", c, &cobra.Command{})} err = r.runE(r.Command, []string{"test-pkg"}) if err == nil { t.Fatal("expected error but got nil") diff --git a/pkg/cli/commands/rpkg/del/command.go b/pkg/cli/commands/rpkg/del/command.go index bee343b4d..4ced6e349 100644 --- a/pkg/cli/commands/rpkg/del/command.go +++ b/pkg/cli/commands/rpkg/del/command.go @@ -23,11 +23,11 @@ import ( porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -36,8 +36,7 @@ const ( func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ Use: "del PACKAGE", @@ -46,7 +45,7 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner Short: docs.DelShort, Long: docs.DelShort + "\n" + docs.DelLong, Example: docs.DelExamples, - PreRunE: r.preRunE, + PreRunE: rpkgutil.MakePreRunE(command+".preRunE", rcg, &r.Client), RunE: r.runE, Hidden: cliutils.HidePorchCommands, } @@ -65,25 +64,14 @@ func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra. } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command -} - -func (r *runner) preRunE(_ *cobra.Command, _ []string) error { - const op errors.Op = command + ".preRunE" - - client, err := cliutils.CreateClientWithFlags(r.cfg) - if err != nil { - return errors.E(op, err) - } - r.client = client - return nil + rpkgutil.Runner } func (r *runner) runE(_ *cobra.Command, args []string) error { const op errors.Op = command + ".runE" + if len(args) == 0 { + return errors.E(op, fmt.Errorf("PACKAGE is a required positional argument")) + } var messages []string for _, pkg := range args { @@ -93,12 +81,12 @@ func (r *runner) runE(_ *cobra.Command, args []string) error { APIVersion: porchapi.SchemeGroupVersion.Identifier(), }, ObjectMeta: metav1.ObjectMeta{ - Namespace: *r.cfg.Namespace, + Namespace: *r.Cfg.Namespace, Name: pkg, }, } - if err := r.client.Delete(r.ctx, pr); err != nil { + if err := r.Client.Delete(r.Ctx, pr); err != nil { messages = append(messages, err.Error()) fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", pkg, err) } else { diff --git a/pkg/cli/commands/rpkg/del/command_test.go b/pkg/cli/commands/rpkg/del/command_test.go index 1550632b0..a473ecfb2 100644 --- a/pkg/cli/commands/rpkg/del/command_test.go +++ b/pkg/cli/commands/rpkg/del/command_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/go-cmp/cmp" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -86,14 +87,7 @@ func TestCmd(t *testing.T) { os.Stdout = write os.Stderr = write - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{ - Namespace: &tc.ns, - }, - client: c, - Command: cmd, - } + r := &runner{Runner: rpkgutil.NewTestRunner(tc.ns, c, cmd)} go func() { defer write.Close() err := r.runE(cmd, []string{pkgRevName}) @@ -121,3 +115,17 @@ func TestNewCommand(t *testing.T) { t.Fatal("NewCommand returned nil") } } + +func TestRunE_RequiresPackageArg(t *testing.T) { + ns := "ns" + scheme, err := rpkgutil.CreateScheme() + if err != nil { + t.Fatalf("error creating scheme: %v", err) + } + c := fake.NewClientBuilder().WithScheme(scheme).Build() + r := &runner{Runner: rpkgutil.NewTestRunner(ns, c, &cobra.Command{})} + + if err := r.runE(r.Command, nil); err == nil { + t.Fatal("runE with no args must error") + } +} diff --git a/pkg/cli/commands/rpkg/propose/command.go b/pkg/cli/commands/rpkg/propose/command.go index 9f197f194..db117990a 100644 --- a/pkg/cli/commands/rpkg/propose/command.go +++ b/pkg/cli/commands/rpkg/propose/command.go @@ -17,15 +17,13 @@ package propose import ( "context" "fmt" - "strings" - "github.com/kptdev/kpt/pkg/lib/errors" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -33,6 +31,9 @@ const ( command = "cmdrpkgpropose" ) +// NewCommand returns the cobra command for `rpkg propose`, which moves +// a draft package revision into the Proposed lifecycle so that an +// approver can later promote it to Published. func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra.Command { v1 := newRunner(ctx, rcg) v2 := newV1Alpha2Runner(ctx, rcg) @@ -42,9 +43,7 @@ func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra. func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, - client: nil, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ @@ -52,7 +51,7 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner Short: docs.ProposeShort, Long: docs.ProposeShort + "\n" + docs.ProposeLong, Example: docs.ProposeExamples, - PreRunE: r.preRunE, + PreRunE: rpkgutil.MakePreRunE(command+".preRunE", rcg, &r.Client), RunE: r.runE, Hidden: cliutils.HidePorchCommands, } @@ -62,80 +61,27 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command - - // Flags + rpkgutil.Runner } -func (r *runner) preRunE(_ *cobra.Command, _ []string) error { - const op errors.Op = command + ".preRunE" - - client, err := cliutils.CreateClientWithFlags(r.cfg) - if err != nil { - return errors.E(op, err) +func (r *runner) proposeAction(ctx context.Context, client client.Client, pr *porchapi.PackageRevision) (string, error) { + switch pr.Spec.Lifecycle { + case porchapi.PackageRevisionLifecycleDraft: + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + if err := client.Update(ctx, pr); err != nil { + return "", err + } + return fmt.Sprintf("%s proposed", pr.Name), nil + case porchapi.PackageRevisionLifecycleProposed: + fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed\n", pr.Name) + return "", nil + default: + return "", fmt.Errorf("cannot propose %s package", pr.Spec.Lifecycle) } - r.client = client - return nil } func (r *runner) runE(_ *cobra.Command, args []string) error { - const op errors.Op = command + ".runE" - var messages []string - namespace := *r.cfg.Namespace - - for _, name := range args { - key := client.ObjectKey{ - Namespace: namespace, - Name: name, - } - var lastErr error - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - var pr porchapi.PackageRevision - err := r.client.Get(r.ctx, key, &pr) - if err != nil { - lastErr = err - return err - } - if !porchapi.PackageRevisionIsReady(pr.Spec.ReadinessGates, pr.Status.Conditions) { - lastErr = fmt.Errorf("readiness conditions not met") - return lastErr - } - switch pr.Spec.Lifecycle { - case porchapi.PackageRevisionLifecycleDraft: - pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed - err := r.client.Update(r.ctx, &pr) - if err == nil { - lastErr = nil - fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed\n", name) - } else { - lastErr = err - } - return err - case porchapi.PackageRevisionLifecycleProposed: - lastErr = nil - fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed\n", name) - return nil - default: - lastErr = fmt.Errorf("cannot propose %s package", pr.Spec.Lifecycle) - return lastErr - } - }) - // Workaround for k8s retry library bug: OnError/RetryOnConflict sometimes returns nil even when errors occur - if err == nil && lastErr != nil { - err = lastErr - } - if err != nil { - messages = append(messages, err.Error()) - fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err) - } - } - - if len(messages) > 0 { - return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) - } - - return nil + return rpkgutil.RunForEachPackage(r.Ctx, r.Client, r.Command, *r.Cfg.Namespace, args, + rpkgutil.RunForEachOpts{CmdName: command, WithRetry: true, CheckReadiness: true}, + r.proposeAction) } diff --git a/pkg/cli/commands/rpkg/propose/command_test.go b/pkg/cli/commands/rpkg/propose/command_test.go index bba4fda25..ed8182e24 100644 --- a/pkg/cli/commands/rpkg/propose/command_test.go +++ b/pkg/cli/commands/rpkg/propose/command_test.go @@ -22,32 +22,19 @@ import ( "github.com/google/go-cmp/cmp" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -func createScheme() (*runtime.Scheme, error) { - scheme := runtime.NewScheme() - - for _, api := range (runtime.SchemeBuilder{ - porchapi.AddToScheme, - }) { - if err := api(scheme); err != nil { - return nil, err - } - } - return scheme, nil -} - func TestCmd(t *testing.T) { pkgRevName := "test-fjdos9u2nfe2f32" ns := "ns" - var scheme, err = createScheme() + var scheme, err = rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -97,14 +84,7 @@ func TestCmd(t *testing.T) { os.Stdout = write os.Stderr = write - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{ - Namespace: &ns, - }, - client: c, - Command: cmd, - } + r := &runner{Runner: rpkgutil.NewTestRunner(ns, c, cmd)} go func() { defer write.Close() err := r.runE(cmd, []string{pkgRevName}) @@ -129,7 +109,7 @@ func TestCmd(t *testing.T) { // issues happen. The easiest way to trigger this in tests is to use an // unreachable cluster. func TestLastErrWorkaround(t *testing.T) { - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -137,13 +117,7 @@ func TestLastErrWorkaround(t *testing.T) { if err != nil { t.Fatalf("error creating client: %v", err) } - ns := "ns" - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, - client: c, - Command: &cobra.Command{}, - } + r := &runner{Runner: rpkgutil.NewTestRunner("ns", c, &cobra.Command{})} err = r.runE(r.Command, []string{"test-pkg"}) if err == nil { t.Fatal("expected error but got nil") diff --git a/pkg/cli/commands/rpkg/proposedelete/command.go b/pkg/cli/commands/rpkg/proposedelete/command.go index 29b263c05..554eff6ab 100644 --- a/pkg/cli/commands/rpkg/proposedelete/command.go +++ b/pkg/cli/commands/rpkg/proposedelete/command.go @@ -17,15 +17,13 @@ package proposedelete import ( "context" "fmt" - "strings" - "github.com/kptdev/kpt/pkg/lib/errors" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -35,8 +33,7 @@ const ( func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ Use: "propose-delete PACKAGE", @@ -45,7 +42,7 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner Long: docs.ProposeDeleteShort + "\n" + docs.ProposeDeleteLong, Example: docs.ProposeDeleteExamples, SuggestFor: []string{}, - PreRunE: r.preRunE, + PreRunE: rpkgutil.MakePreRunE(command+".preRunE", rcg, &r.Client), RunE: r.runE, Hidden: cliutils.HidePorchCommands, } @@ -64,74 +61,27 @@ func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra. } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command + rpkgutil.Runner } -func (r *runner) preRunE(_ *cobra.Command, _ []string) error { - const op errors.Op = command + ".preRunE" - - client, err := cliutils.CreateClientWithFlags(r.cfg) - if err != nil { - return errors.E(op, err) +func (r *runner) proposeDeleteAction(ctx context.Context, client client.Client, pr *porchapi.PackageRevision) (string, error) { + switch pr.Spec.Lifecycle { + case porchapi.PackageRevisionLifecyclePublished: + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed + if err := client.Update(ctx, pr); err != nil { + return "", err + } + return fmt.Sprintf("%s proposed for deletion", pr.Name), nil + case porchapi.PackageRevisionLifecycleDeletionProposed: + fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed for deletion\n", pr.Name) + return "", nil + default: + return "", fmt.Errorf("can only propose published packages for deletion; package %s is not published", pr.Name) } - r.client = client - return nil } func (r *runner) runE(_ *cobra.Command, args []string) error { - const op errors.Op = command + ".runE" - var messages []string - namespace := *r.cfg.Namespace - - for _, name := range args { - key := client.ObjectKey{ - Namespace: namespace, - Name: name, - } - var lastErr error - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - var pr porchapi.PackageRevision - if err := r.client.Get(r.ctx, key, &pr); err != nil { - lastErr = err - return err - } - - switch pr.Spec.Lifecycle { - case porchapi.PackageRevisionLifecyclePublished: - pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed - err := r.client.Update(r.ctx, &pr) - if err == nil { - lastErr = nil - fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed for deletion\n", name) - } else { - lastErr = err - } - return err - case porchapi.PackageRevisionLifecycleDeletionProposed: - lastErr = nil - fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed for deletion\n", name) - return nil - default: - lastErr = fmt.Errorf("can only propose published packages for deletion; package %s is not published", name) - return lastErr - } - - }) - // Workaround for k8s retry library bug: OnError/RetryOnConflict sometimes returns nil even when errors occur - if err == nil && lastErr != nil { - err = lastErr - } - if err != nil { - messages = append(messages, err.Error()) - fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err) - } - } - - if len(messages) > 0 { - return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) - } - return nil + return rpkgutil.RunForEachPackage(r.Ctx, r.Client, r.Command, *r.Cfg.Namespace, args, + rpkgutil.RunForEachOpts{CmdName: command, WithRetry: true, CheckReadiness: false}, + r.proposeDeleteAction) } diff --git a/pkg/cli/commands/rpkg/proposedelete/command_test.go b/pkg/cli/commands/rpkg/proposedelete/command_test.go index 3b546404d..843f82941 100644 --- a/pkg/cli/commands/rpkg/proposedelete/command_test.go +++ b/pkg/cli/commands/rpkg/proposedelete/command_test.go @@ -22,31 +22,18 @@ import ( "github.com/google/go-cmp/cmp" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -func createScheme() (*runtime.Scheme, error) { - scheme := runtime.NewScheme() - - for _, api := range (runtime.SchemeBuilder{ - porchapi.AddToScheme, - }) { - if err := api(scheme); err != nil { - return nil, err - } - } - return scheme, nil -} - func TestCmd(t *testing.T) { pkgRevName := "test-fjdos9u2nfe2f32" - var scheme, err = createScheme() + var scheme, err = rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -103,14 +90,7 @@ func TestCmd(t *testing.T) { os.Stdout = write os.Stderr = write - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{ - Namespace: &tc.ns, - }, - client: c, - Command: cmd, - } + r := &runner{Runner: rpkgutil.NewTestRunner(tc.ns, c, cmd)} go func() { defer write.Close() err := r.runE(cmd, []string{pkgRevName}) @@ -135,7 +115,7 @@ func TestCmd(t *testing.T) { // issues happen. The easiest way to trigger this in tests is to use an // unreachable cluster. func TestLastErrWorkaround(t *testing.T) { - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -143,13 +123,7 @@ func TestLastErrWorkaround(t *testing.T) { if err != nil { t.Fatalf("error creating client: %v", err) } - ns := "ns" - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, - client: c, - Command: &cobra.Command{}, - } + r := &runner{Runner: rpkgutil.NewTestRunner("ns", c, &cobra.Command{})} err = r.runE(r.Command, []string{"test-pkg"}) if err == nil { t.Fatal("expected error but got nil") diff --git a/pkg/cli/commands/rpkg/pull/command.go b/pkg/cli/commands/rpkg/pull/command.go index 1f797a35e..db8a8b658 100644 --- a/pkg/cli/commands/rpkg/pull/command.go +++ b/pkg/cli/commands/rpkg/pull/command.go @@ -29,9 +29,8 @@ import ( porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" - "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/genericclioptions" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kustomize/kyaml/kio" @@ -44,8 +43,7 @@ const ( func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ Use: "pull PACKAGE [DIR]", @@ -62,26 +60,26 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner return r } +// NewCommand returns the cobra command for `rpkg pull`, which fetches +// the resources of a package revision and writes them to a local +// directory or stdout for inspection or editing. func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra.Command { return newRunner(ctx, rcg).Command } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command + rpkgutil.Runner printer printer.Printer } func (r *runner) preRunE(_ *cobra.Command, _ []string) error { const op errors.Op = command + ".preRunE" - config, err := r.cfg.ToRESTConfig() + config, err := r.Cfg.ToRESTConfig() if err != nil { return errors.E(op, err) } - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { return errors.E(op, err) } @@ -91,8 +89,8 @@ func (r *runner) preRunE(_ *cobra.Command, _ []string) error { return errors.E(op, err) } - r.client = c - r.printer = printer.FromContextOrDie(r.ctx) + r.Client = c + r.printer = printer.FromContextOrDie(r.Ctx) return nil } @@ -106,14 +104,14 @@ func (r *runner) runE(_ *cobra.Command, args []string) error { packageName := args[0] var resources porchapi.PackageRevisionResources - if err := r.client.Get(r.ctx, client.ObjectKey{ - Namespace: *r.cfg.Namespace, + if err := r.Client.Get(r.Ctx, client.ObjectKey{ + Namespace: *r.Cfg.Namespace, Name: packageName, }, &resources); err != nil { return errors.E(op, err) } - if err := util.AddRevisionMetadata(&resources); err != nil { + if err := rpkgutil.AddRevisionMetadata(&resources); err != nil { return errors.E(op, err) } @@ -187,19 +185,6 @@ func writeToWriter(resources map[string]string, out io.Writer) error { }.Execute() } -func createScheme() (*runtime.Scheme, error) { - scheme := runtime.NewScheme() - - for _, api := range (runtime.SchemeBuilder{ - porchapi.AddToScheme, - }) { - if err := api(scheme); err != nil { - return nil, err - } - } - return scheme, nil -} - var matchResourceContents = append(kio.MatchAll, kptfilev1.KptFileName, kptfilev1.RevisionMetaDataFileName) func includeFile(path string) bool { diff --git a/pkg/cli/commands/rpkg/pull/command_test.go b/pkg/cli/commands/rpkg/pull/command_test.go index 306c3bac8..b81d4eed7 100644 --- a/pkg/cli/commands/rpkg/pull/command_test.go +++ b/pkg/cli/commands/rpkg/pull/command_test.go @@ -16,6 +16,7 @@ package pull import ( "bytes" + "os" "strings" "testing" @@ -23,17 +24,42 @@ import ( "github.com/kptdev/kpt/pkg/printer" fakeprint "github.com/kptdev/kpt/pkg/printer/fake" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/cli-runtime/pkg/genericclioptions" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +// writeTempKubeconfig writes a minimal kubeconfig pointing at an unreachable +// host into t.TempDir() and returns the path. Sufficient for cfg.ToRESTConfig +// and client.New(), neither of which open a connection. +func writeTempKubeconfig(t *testing.T) string { + t.Helper() + path := t.TempDir() + "/kubeconfig" + const kubeconfigYAML = `apiVersion: v1 +kind: Config +clusters: +- cluster: {server: https://127.0.0.1:1} + name: t +contexts: +- context: {cluster: t, user: t} + name: t +current-context: t +users: +- name: t +` + if err := os.WriteFile(path, []byte(kubeconfigYAML), 0o600); err != nil { + t.Fatalf("write kubeconfig: %v", err) + } + return path +} + func TestCmd(t *testing.T) { pkgRevName := "repo-fjdos9u2nfe2f32" ns := "ns" - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -189,11 +215,11 @@ items: output := &bytes.Buffer{} ctx := fakeprint.CtxWithPrinter(output, output) r := &runner{ - ctx: ctx, - cfg: &genericclioptions.ConfigFlags{ - Namespace: &ns, + Runner: rpkgutil.Runner{ + Ctx: ctx, + Cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, + Client: c, }, - client: c, printer: printer.FromContextOrDie(ctx), } cmd := &cobra.Command{} @@ -207,3 +233,27 @@ items: }) } } + +// TestPreRunE_PopulatesClientAndPrinter exercises preRunE so the helper +// wiring (cfg.ToRESTConfig, rpkgutil.CreateScheme, client.New and the +// printer lookup) is covered. A valid kubeconfig pointed at an unreachable +// host is enough -- the client is built lazily. +func TestPreRunE_PopulatesClientAndPrinter(t *testing.T) { + kubeconfig := writeTempKubeconfig(t) + cfg := genericclioptions.NewConfigFlags(false) + cfg.KubeConfig = &kubeconfig + + var buf bytes.Buffer + ctx := fakeprint.CtxWithPrinter(&buf, &buf) + + r := &runner{Runner: rpkgutil.Runner{Ctx: ctx, Cfg: cfg}} + if err := r.preRunE(&cobra.Command{}, nil); err != nil { + t.Fatalf("preRunE returned error: %v", err) + } + if r.Client == nil { + t.Error("preRunE must populate r.Client") + } + if r.printer == nil { + t.Error("preRunE must populate r.printer") + } +} diff --git a/pkg/cli/commands/rpkg/push/command.go b/pkg/cli/commands/rpkg/push/command.go index 651941273..582abcf9a 100644 --- a/pkg/cli/commands/rpkg/push/command.go +++ b/pkg/cli/commands/rpkg/push/command.go @@ -32,10 +32,9 @@ import ( porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" - "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/genericclioptions" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kustomize/kyaml/kio" @@ -49,8 +48,7 @@ const ( func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ Use: "push PACKAGE [DIR]", @@ -67,26 +65,26 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner return r } +// NewCommand returns the cobra command for `rpkg push`, which uploads +// a local directory of KRM resources back into a draft package +// revision, replacing its existing contents. func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra.Command { return newRunner(ctx, rcg).Command } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command + rpkgutil.Runner printer printer.Printer } func (r *runner) preRunE(_ *cobra.Command, _ []string) error { const op errors.Op = command + ".preRunE" - config, err := r.cfg.ToRESTConfig() + config, err := r.Cfg.ToRESTConfig() if err != nil { return errors.E(op, err) } - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { return errors.E(op, err) } @@ -96,8 +94,8 @@ func (r *runner) preRunE(_ *cobra.Command, _ []string) error { return errors.E(op, err) } - r.client = c - r.printer = printer.FromContextOrDie(r.ctx) + r.Client = c + r.printer = printer.FromContextOrDie(r.Ctx) return nil } @@ -131,24 +129,24 @@ func (r *runner) runE(cmd *cobra.Command, args []string) error { }, ObjectMeta: metav1.ObjectMeta{ Name: packageName, - Namespace: *r.cfg.Namespace, + Namespace: *r.Cfg.Namespace, }, Spec: porchapi.PackageRevisionResourcesSpec{ Resources: resources, }, } - rv, err := util.GetResourceVersion(&pkgResources) + rv, err := rpkgutil.GetResourceVersion(&pkgResources) if err != nil { errString := err.Error() + "\nuse \"porchctl rpkg pull\" to download the remote package metadata before pushing" return errors.E(op, err, errString) } pkgResources.ResourceVersion = rv - if err = util.RemoveRevisionMetadata(&pkgResources); err != nil { + if err = rpkgutil.RemoveRevisionMetadata(&pkgResources); err != nil { return errors.E(op, err) } - if err := r.client.Update(r.ctx, &pkgResources); err != nil { + if err := r.Client.Update(r.Ctx, &pkgResources); err != nil { return errors.E(op, err) } rs := pkgResources.Status.RenderStatus @@ -280,19 +278,6 @@ func readFromReader(in io.Reader) (map[string]string, error) { return rw.resources, nil } -func createScheme() (*runtime.Scheme, error) { - scheme := runtime.NewScheme() - - for _, api := range (runtime.SchemeBuilder{ - porchapi.AddToScheme, - }) { - if err := api(scheme); err != nil { - return nil, err - } - } - return scheme, nil -} - type resourceWriter struct { resources map[string]string } diff --git a/pkg/cli/commands/rpkg/push/command_test.go b/pkg/cli/commands/rpkg/push/command_test.go index 38409f883..11dfced5e 100644 --- a/pkg/cli/commands/rpkg/push/command_test.go +++ b/pkg/cli/commands/rpkg/push/command_test.go @@ -25,18 +25,67 @@ import ( "github.com/kptdev/kpt/pkg/printer" fakeprint "github.com/kptdev/kpt/pkg/printer/fake" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/cli-runtime/pkg/genericclioptions" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +// writeTempKubeconfig writes a minimal kubeconfig pointing at an unreachable +// host into t.TempDir() and returns the path. Sufficient for cfg.ToRESTConfig +// and client.New(), neither of which open a connection. +func writeTempKubeconfig(t *testing.T) string { + t.Helper() + path := t.TempDir() + "/kubeconfig" + const kubeconfigYAML = `apiVersion: v1 +kind: Config +clusters: +- cluster: {server: https://127.0.0.1:1} + name: t +contexts: +- context: {cluster: t, user: t} + name: t +current-context: t +users: +- name: t +` + if err := os.WriteFile(path, []byte(kubeconfigYAML), 0o600); err != nil { + t.Fatalf("write kubeconfig: %v", err) + } + return path +} + +// TestPreRunE_PopulatesClientAndPrinter covers the helper wiring inside +// preRunE so the cfg.ToRESTConfig / rpkgutil.CreateScheme / client.New / +// printer.FromContextOrDie path is exercised. The kubeconfig points at an +// unreachable host, which is fine since client.New is lazy. +func TestPreRunE_PopulatesClientAndPrinter(t *testing.T) { + kubeconfig := writeTempKubeconfig(t) + cfg := genericclioptions.NewConfigFlags(false) + cfg.KubeConfig = &kubeconfig + + var buf bytes.Buffer + ctx := fakeprint.CtxWithPrinter(&buf, &buf) + + r := &runner{Runner: rpkgutil.Runner{Ctx: ctx, Cfg: cfg}} + if err := r.preRunE(&cobra.Command{}, nil); err != nil { + t.Fatalf("preRunE returned error: %v", err) + } + if r.Client == nil { + t.Error("preRunE must populate r.Client") + } + if r.printer == nil { + t.Error("preRunE must populate r.printer") + } +} + func TestCmd(t *testing.T) { pkgRevName := "test-fjdos9u2nfe2f32" ns := "ns" pkgDir := "testdata/test-fjdos9u2nfe2f32" - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -83,11 +132,11 @@ func TestCmd(t *testing.T) { os.Stderr = write ctx := fakeprint.CtxWithPrinter(output, output) r := &runner{ - ctx: ctx, - cfg: &genericclioptions.ConfigFlags{ - Namespace: &ns, + Runner: rpkgutil.Runner{ + Ctx: ctx, + Cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, + Client: c, }, - client: c, printer: printer.FromContextOrDie(ctx), } cmd := &cobra.Command{} diff --git a/pkg/cli/commands/rpkg/reject/command.go b/pkg/cli/commands/rpkg/reject/command.go index 21d7b7c57..cb7968c44 100644 --- a/pkg/cli/commands/rpkg/reject/command.go +++ b/pkg/cli/commands/rpkg/reject/command.go @@ -17,15 +17,14 @@ package reject import ( "context" "fmt" - "strings" "github.com/kptdev/kpt/pkg/lib/errors" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cliutils "github.com/kptdev/porch/internal/cliutils" "github.com/kptdev/porch/pkg/cli/commands/rpkg/docs" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -33,6 +32,10 @@ const ( command = "cmdrpkgreject" ) +// NewCommand returns the cobra command for `rpkg reject`, which rolls +// a Proposed package revision back to Draft, or moves a deletion- +// proposed revision back to Published when an approver declines the +// pending deletion request. func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra.Command { v1 := newRunner(ctx, rcg) v2 := newV1Alpha2Runner(ctx, rcg) @@ -42,8 +45,7 @@ func NewCommand(ctx context.Context, rcg *genericclioptions.ConfigFlags) *cobra. func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner { r := &runner{ - ctx: ctx, - cfg: rcg, + Runner: rpkgutil.Runner{Ctx: ctx, Cfg: rcg}, } c := &cobra.Command{ @@ -61,87 +63,43 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner } type runner struct { - ctx context.Context - cfg *genericclioptions.ConfigFlags - client client.Client - Command *cobra.Command - - // Flags + rpkgutil.Runner } -func (r *runner) preRunE(_ *cobra.Command, args []string) error { +func (r *runner) preRunE(cmd *cobra.Command, args []string) error { const op errors.Op = command + ".preRunE" if len(args) < 1 { - return errors.E(op, "PACKAGE_REVISION is a required positional argument") + return errors.E(op, "PACKAGE is a required positional argument") } - client, err := cliutils.CreateClientWithFlags(r.cfg) + client, err := rpkgutil.InitClient(cmd, r.Cfg) if err != nil { return errors.E(op, err) } - r.client = client + r.Client = client return nil } -func (r *runner) runE(_ *cobra.Command, args []string) error { - const op errors.Op = command + ".runE" - var messages []string - - namespace := *r.cfg.Namespace - var proposedFor string - for _, name := range args { - key := client.ObjectKey{ - Namespace: namespace, - Name: name, - } - var lastErr error - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - var pr porchapi.PackageRevision - if err := r.client.Get(r.ctx, key, &pr); err != nil { - lastErr = err - return err - } - switch pr.Spec.Lifecycle { - case porchapi.PackageRevisionLifecycleProposed: - proposedFor = "approval" - err := cliutils.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, porchapi.PackageRevisionLifecycleDraft) - if err == nil { - lastErr = nil - } else { - lastErr = err - } - return err - case porchapi.PackageRevisionLifecycleDeletionProposed: - proposedFor = "deletion" - // NOTE(kispaljr): should we use UpdatePackageRevisionApproval() here? - pr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished - err := r.client.Update(r.ctx, &pr) - if err == nil { - lastErr = nil - } else { - lastErr = err - } - return err - default: - lastErr = fmt.Errorf("cannot reject %s with lifecycle '%s'", name, pr.Spec.Lifecycle) - return lastErr - } - }) - // Workaround for k8s retry library bug: OnError/RetryOnConflict sometimes returns nil even when errors occur - if err == nil && lastErr != nil { - err = lastErr +func rejectAction(ctx context.Context, client client.Client, pr *porchapi.PackageRevision) (string, error) { + switch pr.Spec.Lifecycle { + case porchapi.PackageRevisionLifecycleProposed: + if err := cliutils.UpdatePackageRevisionApproval(ctx, client, pr, porchapi.PackageRevisionLifecycleDraft); err != nil { + return "", err } - if err != nil { - messages = append(messages, err.Error()) - fmt.Fprintf(r.Command.ErrOrStderr(), "%s failed (%s)\n", name, err) - } else { - fmt.Fprintf(r.Command.OutOrStdout(), "%s no longer proposed for %s\n", name, proposedFor) + return fmt.Sprintf("%s no longer proposed for approval", pr.Name), nil + case porchapi.PackageRevisionLifecycleDeletionProposed: + if err := cliutils.UpdatePackageRevisionApproval(ctx, client, pr, porchapi.PackageRevisionLifecyclePublished); err != nil { + return "", err } + return fmt.Sprintf("%s no longer proposed for deletion", pr.Name), nil + default: + return "", fmt.Errorf("cannot reject %s with lifecycle '%s'", pr.Name, pr.Spec.Lifecycle) } - if len(messages) > 0 { - return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) - } +} - return nil +func (r *runner) runE(_ *cobra.Command, args []string) error { + return rpkgutil.RunForEachPackage(r.Ctx, r.Client, r.Command, *r.Cfg.Namespace, args, + rpkgutil.RunForEachOpts{CmdName: command, WithRetry: true, CheckReadiness: false}, + rejectAction) } diff --git a/pkg/cli/commands/rpkg/reject/command_test.go b/pkg/cli/commands/rpkg/reject/command_test.go index 7b0082291..9761d1435 100644 --- a/pkg/cli/commands/rpkg/reject/command_test.go +++ b/pkg/cli/commands/rpkg/reject/command_test.go @@ -18,13 +18,14 @@ import ( "context" "io" "os" + "strings" "testing" "github.com/google/go-cmp/cmp" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + rpkgutil "github.com/kptdev/porch/pkg/cli/commands/rpkg/util" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -32,24 +33,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) -func createScheme() (*runtime.Scheme, error) { - scheme := runtime.NewScheme() - for _, api := range (runtime.SchemeBuilder{ - porchapi.AddToScheme, - }) { - if err := api(scheme); err != nil { - return nil, err - } - } - scheme.AddKnownTypes(porchapi.SchemeGroupVersion, &porchapi.PackageRevision{}) - return scheme, nil -} - func TestCmd(t *testing.T) { pkgRevName := "test-pr" repoName := "test-repo" ns := "ns" - var scheme, err = createScheme() + var scheme, err = rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -78,7 +66,11 @@ func TestCmd(t *testing.T) { }, "Reject deletion-proposed package": { output: pkgRevName + " no longer proposed for deletion\n", - fakeclient: fake.NewClientBuilder().WithScheme(scheme). + fakeclient: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + return nil + }, + }).WithScheme(scheme). WithObjects(&porchapi.PackageRevision{ TypeMeta: metav1.TypeMeta{ Kind: "PackageRevision", @@ -164,14 +156,7 @@ func TestCmd(t *testing.T) { os.Stdout = write os.Stderr = write - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{ - Namespace: &ns, - }, - client: tc.fakeclient, - Command: cmd, - } + r := &runner{Runner: rpkgutil.NewTestRunner(ns, tc.fakeclient, cmd)} go func() { defer write.Close() err := r.runE(cmd, []string{pkgRevName}) @@ -196,7 +181,7 @@ func TestCmd(t *testing.T) { // issues happen. The easiest way to trigger this in tests is to use an // unreachable cluster. func TestLastErrWorkaround(t *testing.T) { - scheme, err := createScheme() + scheme, err := rpkgutil.CreateScheme() if err != nil { t.Fatalf("error creating scheme: %v", err) } @@ -204,13 +189,7 @@ func TestLastErrWorkaround(t *testing.T) { if err != nil { t.Fatalf("error creating client: %v", err) } - ns := "ns" - r := &runner{ - ctx: context.Background(), - cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, - client: c, - Command: &cobra.Command{}, - } + r := &runner{Runner: rpkgutil.NewTestRunner("ns", c, &cobra.Command{})} err = r.runE(r.Command, []string{"test-pkg"}) if err == nil { t.Fatal("expected error but got nil") @@ -226,3 +205,51 @@ func TestNewCommand(t *testing.T) { t.Fatal("NewCommand returned nil") } } + +// writeTempKubeconfig writes a minimal kubeconfig pointing at an unreachable +// host into t.TempDir() and returns the path. +func writeTempKubeconfig(t *testing.T) string { + t.Helper() + path := t.TempDir() + "/kubeconfig" + const kubeconfigYAML = `apiVersion: v1 +kind: Config +clusters: +- cluster: {server: https://127.0.0.1:1} + name: t +contexts: +- context: {cluster: t, user: t} + name: t +current-context: t +users: +- name: t +` + if err := os.WriteFile(path, []byte(kubeconfigYAML), 0o600); err != nil { + t.Fatalf("write kubeconfig: %v", err) + } + return path +} + +func TestPreRunE_RequiresPositionalArg(t *testing.T) { + r := &runner{Runner: rpkgutil.Runner{Ctx: context.Background()}} + err := r.preRunE(&cobra.Command{}, nil) + if err == nil { + t.Fatal("preRunE with no args must error") + } + if !strings.Contains(err.Error(), "PACKAGE") { + t.Errorf("error should mention PACKAGE, got: %v", err) + } +} + +func TestPreRunE_WithArgsAndValidCfgPopulatesClient(t *testing.T) { + kubeconfig := writeTempKubeconfig(t) + cfg := genericclioptions.NewConfigFlags(false) + cfg.KubeConfig = &kubeconfig + + r := &runner{Runner: rpkgutil.Runner{Ctx: context.Background(), Cfg: cfg}} + if err := r.preRunE(&cobra.Command{}, []string{"pkg-a"}); err != nil { + t.Fatalf("preRunE returned error: %v", err) + } + if r.Client == nil { + t.Error("preRunE must populate r.Client when args + cfg are valid") + } +} diff --git a/pkg/cli/commands/rpkg/util/common.go b/pkg/cli/commands/rpkg/util/common.go index 1ae88e137..5fe308a4f 100644 --- a/pkg/cli/commands/rpkg/util/common.go +++ b/pkg/cli/commands/rpkg/util/common.go @@ -15,17 +15,243 @@ package util import ( + "context" "fmt" + "strings" kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1" + "github.com/kptdev/kpt/pkg/lib/errors" fnsdk "github.com/kptdev/krm-functions-sdk/go/fn" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + cliutils "github.com/kptdev/porch/internal/cliutils" + "github.com/spf13/cobra" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( ResourceVersionAnnotation = "internal.kpt.dev/resource-version" ) +// InitClient creates a controller-runtime client and validates the namespace flag. +// If --namespace is specified without a value, it returns an error immediately. +// A nil cfg is rejected up front so callers (and tests) cannot trigger a panic +// inside cliutils.CreateClientWithFlags -> ConfigFlags.ToRESTConfig. +func InitClient(cmd *cobra.Command, cfg *genericclioptions.ConfigFlags) (client.Client, error) { + // pflag stores the -n shorthand against the same flag entry as the + // --namespace long form, so a single lookup of "namespace" covers both. + nsFlag := cmd.Flag("namespace") + if nsFlag != nil && nsFlag.Changed && nsFlag.Value.String() == "" { + return nil, fmt.Errorf("namespace flag specified without a value; please provide a value for --namespace/-n or omit the flag") + } + + if cfg == nil { + return nil, fmt.Errorf("cfg must not be nil") + } + + c, err := cliutils.CreateClientWithFlags(cfg) + if err != nil { + return nil, err + } + return c, nil +} + +// CreateScheme returns a runtime.Scheme registered with the type sets that +// rpkg subcommands operate on: porchapi (PackageRevision and friends), the +// porchconfig API (Repository), corev1 (ConfigMap and other core kinds), and +// metav1 (Status, WatchEvent and other meta types required for list/watch +// decoding). The set mirrors the scheme used by cliutils.CreateClientWithFlags +// so that commands which build their own client see the same kinds. +func CreateScheme() (*runtime.Scheme, error) { + return buildScheme([]func(*runtime.Scheme) error{ + porchapi.AddToScheme, + configapi.AddToScheme, + corev1.AddToScheme, + metav1.AddMetaToScheme, + }) +} + +// buildScheme registers the given AddToScheme functions on a fresh scheme, +// returning the first error encountered. Splitting this out lets tests inject +// a deliberately failing adder to cover the error branch. +func buildScheme(adders []func(*runtime.Scheme) error) (*runtime.Scheme, error) { + scheme := runtime.NewScheme() + for _, add := range adders { + if err := add(scheme); err != nil { + return nil, err + } + } + return scheme, nil +} + +// MakePreRunE returns a cobra PreRunE function that validates the namespace flag +// and creates a controller-runtime client, storing it in *clientPtr. +// Pass the command's op constant for error context (e.g. command + ".preRunE"). +func MakePreRunE(op errors.Op, cfg *genericclioptions.ConfigFlags, clientPtr *client.Client) func(*cobra.Command, []string) error { + return func(cmd *cobra.Command, _ []string) error { + c, err := InitClient(cmd, cfg) + if err != nil { + return errors.E(op, err) + } + *clientPtr = c + return nil + } +} + +// Runner holds the common fields used by rpkg lifecycle commands. +// +// Ctx is intentionally stored on the struct rather than passed per-method. +// Each rpkg subcommand is a short-lived CLI runner whose lifetime equals one +// command invocation; the context is captured at construction time from the +// outer cobra command and used by the embedded methods (preRunE/runE) that +// cobra invokes through a fixed signature. This mirrors the per-command +// `runner` struct convention that already exists across rpkg subpackages. +// See SonarQube rule godre:S8242 for the general guidance against contexts +// in structs; this exception is documented rather than refactored to avoid +// fanning a behavioural change out across every rpkg subcommand. +type Runner struct { + Ctx context.Context + Cfg *genericclioptions.ConfigFlags + Client client.Client + Command *cobra.Command +} + +// NewTestRunner returns a Runner pre-wired for table-driven CLI tests in the +// rpkg sub-packages. It accepts a namespace string (heap-escaped via &ns), +// a fake client, and the cobra command under test. +func NewTestRunner(ns string, c client.Client, cmd *cobra.Command) Runner { + return Runner{ + Ctx: context.Background(), + Cfg: &genericclioptions.ConfigFlags{Namespace: &ns}, + Client: c, + Command: cmd, + } +} + +// retryOnConflict retries fn on conflict errors with exponential backoff. +// Unlike retry.RetryOnConflict from client-go, this wrapper tracks the last +// error from fn and returns it reliably even when the underlying library +// silently drops it due to a known edge case in ExponentialBackoff. +func retryOnConflict(fn func() error) error { + var lastErr error + wrapped := func() error { + lastErr = fn() + return lastErr + } + err := retry.RetryOnConflict(retry.DefaultRetry, wrapped) + if err == nil && lastErr != nil { + return lastErr + } + return err +} + +// PackageAction processes a single PackageRevision and returns a success message. +// Return ("", nil) to skip the standard success print. Callbacks that have +// already written their own informational output to the command's output stream +// take this path, for example propose and propose-delete printing +// "already proposed" before returning. +type PackageAction func(ctx context.Context, client client.Client, pr *porchapi.PackageRevision) (string, error) + +// RunForEachOpts groups the behavioural flags accepted by RunForEachPackage so +// that call sites read as `WithRetry: true, CheckReadiness: true` instead of +// two anonymous booleans. +// +// CmdName is the per-subcommand identifier (e.g. "cmdrpkgapprove") used to +// build the errors.Op tag suffixed with ".runE". It is required; passing an +// empty string is rejected up front by RunForEachPackage rather than producing +// a malformed ".runE" op tag. +type RunForEachOpts struct { + CmdName string + WithRetry bool + CheckReadiness bool +} + +// fetchAndAct fetches the named PackageRevision, optionally verifies readiness, +// then dispatches to action. Splitting it out keeps RunForEachPackage's +// per-iteration body shallow. +func fetchAndAct( + ctx context.Context, + c client.Client, + key client.ObjectKey, + checkReadiness bool, + action PackageAction, +) (string, error) { + var pr porchapi.PackageRevision + if err := c.Get(ctx, key, &pr); err != nil { + return "", err + } + if checkReadiness && !porchapi.PackageRevisionIsReady(pr.Spec.ReadinessGates, pr.Status.Conditions) { + return "", fmt.Errorf("readiness conditions not met") + } + return action(ctx, c, &pr) +} + +// reportResult prints the per-package outcome and appends the error (if any) +// to messages. Centralising the print/append logic keeps the caller free of +// the if/else-if branching that previously inflated cognitive complexity. +func reportResult(cmd *cobra.Command, name, successMsg string, err error, messages *[]string) { + if err != nil { + *messages = append(*messages, err.Error()) + fmt.Fprintf(cmd.ErrOrStderr(), "%s failed (%s)\n", name, err) + return + } + if successMsg != "" { + fmt.Fprintf(cmd.OutOrStdout(), "%s\n", successMsg) + } +} + +// RunForEachPackage loops over package names, fetches each PackageRevision, +// optionally runs a readiness pre-check, then runs the action. Errors are +// collected and reported. When opts.WithRetry is true, each iteration is +// retried on conflict. When opts.CheckReadiness is true, readiness gates +// are verified before the action runs. +func RunForEachPackage( + ctx context.Context, + c client.Client, + cmd *cobra.Command, + namespace string, + args []string, + opts RunForEachOpts, + action PackageAction, +) error { + if opts.CmdName == "" { + return fmt.Errorf("RunForEachOpts.CmdName must not be empty") + } + op := errors.Op(opts.CmdName + ".runE") + if len(args) == 0 { + return errors.E(op, fmt.Errorf("PACKAGE is a required positional argument")) + } + var messages []string + + for _, name := range args { + key := client.ObjectKey{Namespace: namespace, Name: name} + var successMsg string + run := func() error { + msg, err := fetchAndAct(ctx, c, key, opts.CheckReadiness, action) + successMsg = msg + return err + } + + var err error + if opts.WithRetry { + err = retryOnConflict(run) + } else { + err = run() + } + reportResult(cmd, name, successMsg, err, &messages) + } + + if len(messages) > 0 { + return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) + } + return nil +} + func GetResourceFileKubeObject(prr *porchapi.PackageRevisionResources, file, kind, name string) (*fnsdk.KubeObject, error) { if prr.Spec.Resources == nil { return nil, fmt.Errorf("nil resources found for PackageRevisionResources '%s/%s'", prr.Namespace, prr.Name) diff --git a/pkg/cli/commands/rpkg/util/common_test.go b/pkg/cli/commands/rpkg/util/common_test.go new file mode 100644 index 000000000..c683c9e86 --- /dev/null +++ b/pkg/cli/commands/rpkg/util/common_test.go @@ -0,0 +1,394 @@ +// 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 util + +import ( + "bytes" + "context" + "fmt" + "os" + "testing" + + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/cli-runtime/pkg/genericclioptions" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// -- InitClient tests -- + +func TestInitClient_EmptyNamespaceReturnsError(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().String("namespace", "", "") + _ = cmd.Flags().Set("namespace", "") + + _, err := InitClient(cmd, nil) + assert.ErrorContains(t, err, "namespace flag specified without a value") +} + +func TestInitClient_NamespaceNotSetPassesValidation(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().String("namespace", "default", "") // flag defined but never .Set, so .Changed is false + + kubeconfig := createTempKubeconfig(t) + cfg := genericclioptions.NewConfigFlags(false) + cfg.KubeConfig = &kubeconfig + + _, err := InitClient(cmd, cfg) + assert.NoError(t, err, "an unchanged namespace flag must pass the namespace-empty check") +} + +func TestInitClient_ValidConfigReturnsClient(t *testing.T) { + cmd := &cobra.Command{} + kubeconfig := createTempKubeconfig(t) + cfg := genericclioptions.NewConfigFlags(false) + cfg.KubeConfig = &kubeconfig + + c, err := InitClient(cmd, cfg) + assert.NoError(t, err) + assert.NotNil(t, c) +} + +func TestInitClient_ShorthandFlagSharesEntryWithLongName(t *testing.T) { + // Declaring with StringP creates a single flag entry indexed by the long + // name "namespace"; the shorthand -n does not get its own entry. This + // test guards against accidentally re-introducing a separate cmd.Flag("n") + // lookup, which would always return nil. + cmd := &cobra.Command{} + cmd.Flags().StringP("namespace", "n", "", "") + _ = cmd.Flags().Set("namespace", "") + + _, err := InitClient(cmd, nil) + assert.ErrorContains(t, err, "namespace flag specified without a value") + assert.Nil(t, cmd.Flag("n"), + "pflag does not register the shorthand as a separate flag entry") +} + +func TestInitClient_NilCfgReturnsError(t *testing.T) { + cmd := &cobra.Command{} + // No namespace flag is changed, so namespace validation passes and the + // nil-cfg branch is reached. + _, err := InitClient(cmd, nil) + assert.ErrorContains(t, err, "cfg must not be nil") +} + +// -- MakePreRunE tests -- + +func TestMakePreRunE_SetsClient(t *testing.T) { + kubeconfig := createTempKubeconfig(t) + cfg := genericclioptions.NewConfigFlags(false) + cfg.KubeConfig = &kubeconfig + + var c client.Client + preRunE := MakePreRunE("test.preRunE", cfg, &c) + + err := preRunE(&cobra.Command{}, nil) + assert.NoError(t, err) + assert.NotNil(t, c) +} + +func TestMakePreRunE_EmptyNamespaceReturnsError(t *testing.T) { + var c client.Client + preRunE := MakePreRunE("test.preRunE", &genericclioptions.ConfigFlags{}, &c) + + cmd := &cobra.Command{} + cmd.Flags().String("namespace", "", "") + _ = cmd.Flags().Set("namespace", "") + + err := preRunE(cmd, nil) + assert.ErrorContains(t, err, "namespace flag specified without a value") +} + +// -- CreateScheme tests -- + +func TestCreateScheme_RegistersPorchTypes(t *testing.T) { + scheme, err := CreateScheme() + require.NoError(t, err) + assert.NotNil(t, scheme) + + gvk := porchapi.SchemeGroupVersion.WithKind("PackageRevision") + assert.True(t, scheme.Recognizes(gvk), "scheme should recognize PackageRevision") +} + +func TestCreateScheme_RegistersConfigAndCoreTypes(t *testing.T) { + scheme, err := CreateScheme() + require.NoError(t, err) + + configGVK := configapi.GroupVersion.WithKind("Repository") + assert.True(t, scheme.Recognizes(configGVK), "scheme should recognize porchconfig Repository") + + coreGVK := corev1.SchemeGroupVersion.WithKind("ConfigMap") + assert.True(t, scheme.Recognizes(coreGVK), "scheme should recognize core/v1 ConfigMap") +} + +func TestBuildScheme_ReturnsErrorWhenAdderFails(t *testing.T) { + wantErr := fmt.Errorf("boom") + scheme, err := buildScheme([]func(*runtime.Scheme) error{ + func(*runtime.Scheme) error { return wantErr }, + }) + assert.Nil(t, scheme) + assert.ErrorIs(t, err, wantErr) +} + +func TestBuildScheme_StopsAtFirstError(t *testing.T) { + called := 0 + first := func(*runtime.Scheme) error { called++; return fmt.Errorf("first") } + second := func(*runtime.Scheme) error { called++; return nil } + _, err := buildScheme([]func(*runtime.Scheme) error{first, second}) + assert.ErrorContains(t, err, "first") + assert.Equal(t, 1, called, "should not call subsequent adders after error") +} + +// -- NewTestRunner tests -- + +func TestNewTestRunner_PopulatesAllFields(t *testing.T) { + cmd := &cobra.Command{} + r := NewTestRunner("kube-system", nil, cmd) + + require.NotNil(t, r.Ctx, "Ctx should be set to a non-nil context") + require.NotNil(t, r.Cfg, "Cfg should be initialised") + require.NotNil(t, r.Cfg.Namespace, "Cfg.Namespace should be a pointer to a heap-escaped string") + assert.Equal(t, "kube-system", *r.Cfg.Namespace) + assert.Same(t, cmd, r.Command, "Command should reference the caller's *cobra.Command") +} + +// -- RunForEachPackage tests -- + +func TestRunForEachPackage_AllSucceed(t *testing.T) { + cmd, stdout, stderr := setupCmdBuffers() + + fc := setupFakeClient(t, + newPR("pkg-a", "ns", porchapi.PackageRevisionLifecycleDraft), + newPR("pkg-b", "ns", porchapi.PackageRevisionLifecycleDraft), + ) + opts := RunForEachOpts{CmdName: "cmdrpkgtest", WithRetry: true} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"pkg-a", "pkg-b"}, opts, + func(_ context.Context, _ client.Client, pr *porchapi.PackageRevision) (string, error) { + return fmt.Sprintf("%s done", pr.Name), nil + }) + assert.NoError(t, err) + assert.Contains(t, stdout.String(), "pkg-a done") + assert.Contains(t, stdout.String(), "pkg-b done") + assert.Empty(t, stderr.String()) +} + +func TestRunForEachPackage_PartialFailure(t *testing.T) { + cmd, stdout, stderr := setupCmdBuffers() + + fc := setupFakeClient(t, + newPR("pkg-a", "ns", porchapi.PackageRevisionLifecycleDraft), + newPR("pkg-b", "ns", porchapi.PackageRevisionLifecycleDraft), + ) + opts := RunForEachOpts{CmdName: "cmdrpkgtest", WithRetry: true} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"pkg-a", "pkg-b"}, opts, + func(_ context.Context, _ client.Client, pr *porchapi.PackageRevision) (string, error) { + if pr.Name == "pkg-b" { + return "", fmt.Errorf("something went wrong") + } + return fmt.Sprintf("%s done", pr.Name), nil + }) + assert.Error(t, err) + assert.Contains(t, stdout.String(), "pkg-a done") + assert.Contains(t, stderr.String(), "pkg-b failed") +} + +func TestRunForEachPackage_NotFoundReportsError(t *testing.T) { + cmd, _, stderr := setupCmdBuffers() + + fc := setupFakeClient(t) + opts := RunForEachOpts{CmdName: "cmdrpkgtest", WithRetry: true} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"missing-pkg"}, opts, + func(_ context.Context, _ client.Client, pr *porchapi.PackageRevision) (string, error) { + return fmt.Sprintf("%s done", pr.Name), nil + }) + assert.Error(t, err) + assert.Contains(t, stderr.String(), "missing-pkg failed") +} + +func TestRunForEachPackage_WithoutRetry(t *testing.T) { + cmd, stdout, _ := setupCmdBuffers() + + fc := setupFakeClient(t, + newPR("pkg-a", "ns", porchapi.PackageRevisionLifecycleDraft), + ) + opts := RunForEachOpts{CmdName: "cmdrpkgtest"} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"pkg-a"}, opts, + func(_ context.Context, _ client.Client, pr *porchapi.PackageRevision) (string, error) { + return fmt.Sprintf("%s deleted", pr.Name), nil + }) + assert.NoError(t, err) + assert.Contains(t, stdout.String(), "pkg-a deleted") +} + +func TestRunForEachPackage_EmptySuccessMessageSkipsPrint(t *testing.T) { + cmd, stdout, stderr := setupCmdBuffers() + + fc := setupFakeClient(t, + newPR("pkg-a", "ns", porchapi.PackageRevisionLifecycleDraft), + ) + opts := RunForEachOpts{CmdName: "cmdrpkgtest", WithRetry: true} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"pkg-a"}, opts, + func(_ context.Context, _ client.Client, _ *porchapi.PackageRevision) (string, error) { + return "", nil + }) + assert.NoError(t, err) + assert.Empty(t, stdout.String(), "empty success message should not print anything") + assert.Empty(t, stderr.String()) +} + +func TestRunForEachPackage_WithoutRetryGetFails(t *testing.T) { + cmd, _, stderr := setupCmdBuffers() + + fc := setupFakeClient(t) + opts := RunForEachOpts{CmdName: "cmdrpkgtest"} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"missing"}, opts, + func(_ context.Context, _ client.Client, pr *porchapi.PackageRevision) (string, error) { + return fmt.Sprintf("%s done", pr.Name), nil + }) + assert.Error(t, err) + assert.Contains(t, stderr.String(), "missing failed") +} + +func TestRunForEachPackage_ReadinessCheckFails(t *testing.T) { + cmd, _, stderr := setupCmdBuffers() + + fc := setupFakeClient(t, + newPR("pkg-a", "ns", porchapi.PackageRevisionLifecycleDraft), + ) + // Set a readiness gate that is not met + pr := &porchapi.PackageRevision{} + require.NoError(t, fc.Get(context.Background(), client.ObjectKey{Namespace: "ns", Name: "pkg-a"}, pr)) + pr.Spec.ReadinessGates = []porchapi.ReadinessGate{{ConditionType: "test.Ready"}} + require.NoError(t, fc.Update(context.Background(), pr)) + + opts := RunForEachOpts{CmdName: "cmdrpkgtest", WithRetry: true, CheckReadiness: true} + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"pkg-a"}, opts, + func(_ context.Context, _ client.Client, pr *porchapi.PackageRevision) (string, error) { + return fmt.Sprintf("%s done", pr.Name), nil + }) + assert.Error(t, err) + assert.Contains(t, stderr.String(), "readiness conditions not met") +} + +func TestRunForEachPackage_RequiresAtLeastOneArg(t *testing.T) { + cmd, _, _ := setupCmdBuffers() + fc := setupFakeClient(t) + + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + nil, // no args + RunForEachOpts{CmdName: "cmdrpkgtest"}, + func(_ context.Context, _ client.Client, _ *porchapi.PackageRevision) (string, error) { + return "", nil + }) + assert.ErrorContains(t, err, "PACKAGE", + "empty args must fail with a clear error rather than silently returning success") +} + +func TestRunForEachPackage_RequiresCmdName(t *testing.T) { + cmd, _, _ := setupCmdBuffers() + fc := setupFakeClient(t) + + err := RunForEachPackage(context.Background(), fc, cmd, "ns", + []string{"pkg-a"}, + RunForEachOpts{}, // CmdName intentionally empty + func(_ context.Context, _ client.Client, _ *porchapi.PackageRevision) (string, error) { + return "", nil + }) + assert.ErrorContains(t, err, "RunForEachOpts.CmdName must not be empty", + "empty CmdName must fail fast so error.Op tags are well-formed") +} + +func TestRetryOnConflict_ReturnsErrorWhenFnFails(t *testing.T) { + called := 0 + err := retryOnConflict(func() error { + called++ + return fmt.Errorf("connection refused") + }) + assert.ErrorContains(t, err, "connection refused") + assert.Equal(t, 1, called, "non-conflict error should not retry") +} + +func TestRetryOnConflict_ReturnsNilOnSuccess(t *testing.T) { + err := retryOnConflict(func() error { + return nil + }) + assert.NoError(t, err) +} + +// -- test helpers -- + +func setupCmdBuffers() (cmd *cobra.Command, stdout *bytes.Buffer, stderr *bytes.Buffer) { + stdout = &bytes.Buffer{} + stderr = &bytes.Buffer{} + cmd = &cobra.Command{} + cmd.SetOut(stdout) + cmd.SetErr(stderr) + return +} + +func newPR(name, ns string, lifecycle porchapi.PackageRevisionLifecycle) *porchapi.PackageRevision { + return &porchapi.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: porchapi.PackageRevisionSpec{Lifecycle: lifecycle}, + } +} + +func setupFakeClient(t *testing.T, objs ...client.Object) client.Client { + t.Helper() + scheme, err := CreateScheme() + require.NoError(t, err) + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() +} + +func createTempKubeconfig(t *testing.T) string { + t.Helper() + content := `apiVersion: v1 +kind: Config +clusters: +- cluster: + server: https://127.0.0.1:6443 + name: test +contexts: +- context: + cluster: test + user: test + name: test +current-context: test +users: +- name: test + user: + token: fake-token +` + f, err := os.CreateTemp(t.TempDir(), "kubeconfig-*") + require.NoError(t, err) + _, err = f.WriteString(content) + require.NoError(t, err) + require.NoError(t, f.Close()) + return f.Name() +}