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() +}