Skip to content

Reduce code duplication across rpkg CLI commands #541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thc1006 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reduce code duplication across rpkg CLI commands #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 14 additions & 66 deletions pkg/cli/commands/rpkg/approve/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
}
Expand All @@ -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)
Comment thread
thc1006 marked this conversation as resolved.
}
40 changes: 7 additions & 33 deletions pkg/cli/commands/rpkg/approve/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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",
},
},
Expand Down Expand Up @@ -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})
Expand All @@ -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")
Expand Down
30 changes: 9 additions & 21 deletions pkg/cli/commands/rpkg/del/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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",
Expand All @@ -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,
}
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
24 changes: 16 additions & 8 deletions pkg/cli/commands/rpkg/del/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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})
Expand Down Expand Up @@ -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")
}
}
Loading
Loading