Reduce code duplication across rpkg CLI commands (#541)

* 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>

* Address @mozesl-nokia post-approval review feedback

Two cosmetic items raised after @liamfallon's approval are addressed
in this commit.

1. approve/command_test.go used "nephio.org.Specializer.specialize"
   as the ReadinessGate fixture string, which lingered from the
   branch state before #981 swept the rest of the repository. The
   string is pure test data: the test only verifies that the
   ConditionType on a ReadinessGate matches a Condition.Type with
   status False. Aligned to "kpt.dev.Specializer.specialize" so it
   matches what #981 settled on for the rest of the codebase.

2. writeTempKubeconfig was duplicated across pull, push and reject
   test files. Extracted as the exported rpkgutil.WriteTempKubeconfig
   in a new pkg/cli/commands/rpkg/util/testhelpers.go, matching the
   existing testhelpers_v1alpha2.go pattern. The three test files
   now call the shared helper, and the now-unused "os" import is
   dropped from pull/command_test.go.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

---------

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Co-authored-by: Liam Fallon <35595825+liamfallon@users.noreply.github.com>

Author: thc1006

pkg/cli/commands/rpkg/approve/command.go

@@ -17,22 +17,23 @@ 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"
 )
 
 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,16 +43,15 @@ 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{
 		Use:     "approve PACKAGE",
 		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)
 }

pkg/cli/commands/rpkg/approve/command_test.go

@@ -22,34 +22,21 @@ 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"
 	"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)
 	}
@@ -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,21 +207,15 @@ 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)
 	}
 	c, err := client.New(&rest.Config{Host: "https://127.0.0.1:1", Timeout: 1, TLSClientConfig: rest.TLSClientConfig{Insecure: true}}, client.Options{Scheme: scheme})
 	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")

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 {

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