diff --git a/internal/kpt/util/porch/approval.go b/internal/kpt/util/porch/approval.go index c0bdd680..97a3d5e4 100644 --- a/internal/kpt/util/porch/approval.go +++ b/internal/kpt/util/porch/approval.go @@ -21,24 +21,25 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func UpdatePackageRevisionApproval(ctx context.Context, client client.Client, key client.ObjectKey, new v1alpha1.PackageRevisionLifecycle) error { - - var pr v1alpha1.PackageRevision - if err := client.Get(ctx, key, &pr); err != nil { - return err - } +func UpdatePackageRevisionApproval(ctx context.Context, client client.Client, pr *v1alpha1.PackageRevision, new v1alpha1.PackageRevisionLifecycle) error { switch lifecycle := pr.Spec.Lifecycle; lifecycle { - case v1alpha1.PackageRevisionLifecycleProposed, v1alpha1.PackageRevisionLifecycleDeletionProposed: - // ok + case v1alpha1.PackageRevisionLifecycleProposed: + // Approve - change the package revision kind to 'final'. + if new != v1alpha1.PackageRevisionLifecyclePublished && new != v1alpha1.PackageRevisionLifecycleDraft { + return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new) + } + case v1alpha1.PackageRevisionLifecycleDeletionProposed: + if new != v1alpha1.PackageRevisionLifecyclePublished { + return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new) + } case new: // already correct value return nil default: return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new) } - // Approve - change the package revision kind to "final". - pr.Spec.Lifecycle = new - return client.SubResource("approval").Update(ctx, &pr) + pr.Spec.Lifecycle = new + return client.SubResource("approval").Update(ctx, pr) } diff --git a/pkg/cli/commands/rpkg/approve/command.go b/pkg/cli/commands/rpkg/approve/command.go index 2e6c6cbe..5fb4bfb4 100644 --- a/pkg/cli/commands/rpkg/approve/command.go +++ b/pkg/cli/commands/rpkg/approve/command.go @@ -25,6 +25,7 @@ import ( "github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -83,17 +84,24 @@ func (r *runner) runE(_ *cobra.Command, args []string) error { namespace := *r.cfg.Namespace for _, name := range args { - if err := porch.UpdatePackageRevisionApproval(r.ctx, r.client, client.ObjectKey{ + key := client.ObjectKey{ Namespace: namespace, Name: name, - }, v1alpha1.PackageRevisionLifecyclePublished); err != nil { + } + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var pr v1alpha1.PackageRevision + if err := r.client.Get(r.ctx, key, &pr); err != nil { + return err + } + return porch.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, v1alpha1.PackageRevisionLifecyclePublished) + }) + 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 "))) } diff --git a/pkg/cli/commands/rpkg/propose/command.go b/pkg/cli/commands/rpkg/propose/command.go index a979cde3..3ae744d0 100644 --- a/pkg/cli/commands/rpkg/propose/command.go +++ b/pkg/cli/commands/rpkg/propose/command.go @@ -25,6 +25,7 @@ import ( "github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -83,33 +84,34 @@ func (r *runner) runE(_ *cobra.Command, args []string) error { namespace := *r.cfg.Namespace for _, name := range args { - pr := &v1alpha1.PackageRevision{} - if err := r.client.Get(r.ctx, client.ObjectKey{ + key := client.ObjectKey{ Namespace: namespace, Name: name, - }, pr); err != nil { - return errors.E(op, err) } - - switch pr.Spec.Lifecycle { - case v1alpha1.PackageRevisionLifecycleDraft: - // ok - case v1alpha1.PackageRevisionLifecycleProposed: - fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed\n", name) - continue - default: - msg := fmt.Sprintf("cannot propose %s package", pr.Spec.Lifecycle) - messages = append(messages, msg) - fmt.Fprintln(r.Command.ErrOrStderr(), msg) - continue - } - - pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleProposed - if err := r.client.Update(r.ctx, pr); err != nil { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var pr v1alpha1.PackageRevision + if err := r.client.Get(r.ctx, key, &pr); err != nil { + return err + } + + switch pr.Spec.Lifecycle { + case v1alpha1.PackageRevisionLifecycleDraft: + pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleProposed + err := r.client.Update(r.ctx, &pr) + if err == nil { + fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed\n", name) + } + return err + case v1alpha1.PackageRevisionLifecycleProposed: + fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed\n", name) + return nil + default: + return fmt.Errorf("cannot propose %s package", pr.Spec.Lifecycle) + } + }) + 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 proposed\n", name) } } diff --git a/pkg/cli/commands/rpkg/propose/command_test.go b/pkg/cli/commands/rpkg/propose/command_test.go index fce021f2..cce16abd 100644 --- a/pkg/cli/commands/rpkg/propose/command_test.go +++ b/pkg/cli/commands/rpkg/propose/command_test.go @@ -63,7 +63,7 @@ func TestCmd(t *testing.T) { lc: porchapi.PackageRevisionLifecycleDraft, }, "Cannot propose package": { - output: "cannot propose Published package\n", + output: pkgRevName + " failed (cannot propose Published package)\n", lc: porchapi.PackageRevisionLifecyclePublished, wantErr: true, }, diff --git a/pkg/cli/commands/rpkg/proposedelete/command.go b/pkg/cli/commands/rpkg/proposedelete/command.go index a417b470..19936a59 100644 --- a/pkg/cli/commands/rpkg/proposedelete/command.go +++ b/pkg/cli/commands/rpkg/proposedelete/command.go @@ -25,6 +25,7 @@ import ( "github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -83,39 +84,40 @@ func (r *runner) runE(_ *cobra.Command, args []string) error { namespace := *r.cfg.Namespace for _, name := range args { - pr := &v1alpha1.PackageRevision{} - if err := r.client.Get(r.ctx, client.ObjectKey{ + key := client.ObjectKey{ Namespace: namespace, Name: name, - }, pr); err != nil { - return errors.E(op, err) } - - switch pr.Spec.Lifecycle { - case v1alpha1.PackageRevisionLifecyclePublished: - // ok - case v1alpha1.PackageRevisionLifecycleDeletionProposed: - fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed for deletion\n", name) - continue - default: - msg := fmt.Sprintf("can only propose published packages for deletion; package %s is not published", name) - messages = append(messages, msg) - fmt.Fprintln(r.Command.ErrOrStderr(), msg) - continue - } - - pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleDeletionProposed - if err := r.client.Update(r.ctx, pr); err != nil { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var pr v1alpha1.PackageRevision + if err := r.client.Get(r.ctx, key, &pr); err != nil { + return err + } + + switch pr.Spec.Lifecycle { + case v1alpha1.PackageRevisionLifecyclePublished: + pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleDeletionProposed + err := r.client.Update(r.ctx, &pr) + if err == nil { + fmt.Fprintf(r.Command.OutOrStdout(), "%s proposed for deletion\n", name) + } + return err + case v1alpha1.PackageRevisionLifecycleDeletionProposed: + fmt.Fprintf(r.Command.OutOrStderr(), "%s is already proposed for deletion\n", name) + return nil + default: + return fmt.Errorf("can only propose published packages for deletion; package %s is not published", name) + } + + }) + 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 proposed for deletion\n", name) } } if len(messages) > 0 { return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) } - return nil } diff --git a/pkg/cli/commands/rpkg/proposedelete/command_test.go b/pkg/cli/commands/rpkg/proposedelete/command_test.go index 7ad83ba2..2495170c 100644 --- a/pkg/cli/commands/rpkg/proposedelete/command_test.go +++ b/pkg/cli/commands/rpkg/proposedelete/command_test.go @@ -55,11 +55,12 @@ func TestCmd(t *testing.T) { ns string }{ "Package not found in ns": { + output: pkgRevName + " failed (packagerevisions.porch.kpt.dev \"" + pkgRevName + "\" not found)\n", ns: "doesnotexist", wantErr: true, }, "Package not published": { - output: "can only propose published packages for deletion; package " + pkgRevName + " is not published\n", + output: pkgRevName + " failed (can only propose published packages for deletion; package " + pkgRevName + " is not published)\n", ns: "ns", wantErr: true, }, diff --git a/pkg/cli/commands/rpkg/reject/command.go b/pkg/cli/commands/rpkg/reject/command.go index 52f23eb0..30d90ec2 100644 --- a/pkg/cli/commands/rpkg/reject/command.go +++ b/pkg/cli/commands/rpkg/reject/command.go @@ -25,6 +25,7 @@ import ( "github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -85,41 +86,37 @@ func (r *runner) runE(_ *cobra.Command, args []string) error { var messages []string namespace := *r.cfg.Namespace - + var proposedFor string for _, name := range args { - pr := &v1alpha1.PackageRevision{} - if err := r.client.Get(r.ctx, client.ObjectKey{ + key := client.ObjectKey{ Namespace: namespace, Name: name, - }, pr); err != nil { - return errors.E(op, err) } - switch pr.Spec.Lifecycle { - case v1alpha1.PackageRevisionLifecycleProposed: - if err := porch.UpdatePackageRevisionApproval(r.ctx, r.client, client.ObjectKey{ - Namespace: namespace, - Name: name, - }, v1alpha1.PackageRevisionLifecycleDraft); 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 rejected\n", name) + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var pr v1alpha1.PackageRevision + if err := r.client.Get(r.ctx, key, &pr); err != nil { + return err } - case v1alpha1.PackageRevisionLifecycleDeletionProposed: - pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecyclePublished - if err := r.client.Update(r.ctx, pr); 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 deletion\n", name) + switch pr.Spec.Lifecycle { + case v1alpha1.PackageRevisionLifecycleProposed: + proposedFor = "approval" + return porch.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, v1alpha1.PackageRevisionLifecycleDraft) + case v1alpha1.PackageRevisionLifecycleDeletionProposed: + proposedFor = "deletion" + // NOTE(kispaljr): should we use UpdatePackageRevisionApproval() here? + pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecyclePublished + return r.client.Update(r.ctx, &pr) + default: + return fmt.Errorf("cannot reject %s with lifecycle '%s'", name, pr.Spec.Lifecycle) } - default: - msg := fmt.Sprintf("cannot reject %s with lifecycle '%s'", name, pr.Spec.Lifecycle) - messages = append(messages, msg) - fmt.Fprintln(r.Command.ErrOrStderr(), msg) + }) + 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) } } - if len(messages) > 0 { return errors.E(op, fmt.Errorf("errors:\n %s", strings.Join(messages, "\n "))) } diff --git a/pkg/cli/commands/rpkg/reject/command_test.go b/pkg/cli/commands/rpkg/reject/command_test.go index 94ca4730..e92601bb 100644 --- a/pkg/cli/commands/rpkg/reject/command_test.go +++ b/pkg/cli/commands/rpkg/reject/command_test.go @@ -58,6 +58,7 @@ func TestCmd(t *testing.T) { fakeclient client.WithWatch }{ "Package not found in ns": { + output: pkgRevName + " failed (packagerevisions.porch.kpt.dev \"" + pkgRevName + "\" not found)\n", wantErr: true, fakeclient: fake.NewClientBuilder().WithScheme(scheme). WithObjects(&porchapi.PackageRevision{ @@ -93,7 +94,7 @@ func TestCmd(t *testing.T) { }, "Reject draft package": { wantErr: true, - output: "cannot reject " + pkgRevName + " with lifecycle 'Draft'\n", + output: pkgRevName + " failed (cannot reject " + pkgRevName + " with lifecycle 'Draft')\n", fakeclient: fake.NewClientBuilder().WithScheme(scheme). WithObjects(&porchapi.PackageRevision{ TypeMeta: metav1.TypeMeta{ @@ -111,7 +112,7 @@ func TestCmd(t *testing.T) { }, "Reject published package": { wantErr: true, - output: "cannot reject " + pkgRevName + " with lifecycle 'Published'\n", + output: pkgRevName + " failed (cannot reject " + pkgRevName + " with lifecycle 'Published')\n", fakeclient: fake.NewClientBuilder().WithScheme(scheme). WithObjects(&porchapi.PackageRevision{ TypeMeta: metav1.TypeMeta{ @@ -128,7 +129,7 @@ func TestCmd(t *testing.T) { }}).Build(), }, "Reject proposed package": { - output: pkgRevName + " rejected\n", + output: pkgRevName + " no longer proposed for approval\n", fakeclient: fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{ //fake subresourceupdate SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { diff --git a/pkg/cli/commands/rpkg/update/command.go b/pkg/cli/commands/rpkg/update/command.go index 810f0f2e..f7f2a97b 100644 --- a/pkg/cli/commands/rpkg/update/command.go +++ b/pkg/cli/commands/rpkg/update/command.go @@ -24,6 +24,7 @@ import ( "github.com/nephio-project/porch/pkg/cli/commands/rpkg/docs" "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -114,7 +115,15 @@ func (r *runner) runE(cmd *cobra.Command, args []string) error { if pr == nil { return errors.E(op, fmt.Errorf("could not find package revision %s", args[0])) } - if err := r.doUpdate(pr); err != nil { + key := client.ObjectKeyFromObject(pr) + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var pr porchapi.PackageRevision + if err := r.client.Get(r.ctx, key, &pr); err != nil { + return err + } + return r.doUpdate(&pr) + }) + if err != nil { return errors.E(op, err) } if _, err := fmt.Fprintf(cmd.OutOrStdout(), "%s updated\n", pr.Name); err != nil { diff --git a/scripts/cleanup-after-tests.sh b/scripts/cleanup-after-tests.sh index e2300ad8..144b42c3 100755 --- a/scripts/cleanup-after-tests.sh +++ b/scripts/cleanup-after-tests.sh @@ -9,4 +9,4 @@ rm -rf "$self_dir/../.cache" kubectl get packagerev -A --no-headers | awk '{ print "-n", $1, $2 }' | xargs -L 1 --no-run-if-empty kubectl patch packagerev --type='json' -p='[{"op": "remove", "path": "/metadata/finalizers"}]' -kubectl get ns -o name --no-headers | grep test | xargs -L 1 --no-run-if-empty kubectl delete +kubectl get ns -o name --no-headers | egrep '(test|rpkg-|repo-)' | xargs -L 1 --no-run-if-empty kubectl delete diff --git a/test/e2e/cli/testdata/rpkg-lifecycle/config.yaml b/test/e2e/cli/testdata/rpkg-lifecycle/config.yaml index d560517f..e3eab160 100644 --- a/test/e2e/cli/testdata/rpkg-lifecycle/config.yaml +++ b/test/e2e/cli/testdata/rpkg-lifecycle/config.yaml @@ -14,7 +14,8 @@ commands: - --repository=git - --workspace=lifecycle - lifecycle-package - stdout: "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 created\n" + stdout: | + git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 created - args: - porchctl - rpkg @@ -29,11 +30,8 @@ commands: - propose-delete - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 - --namespace=rpkg-lifecycle + stderr: "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 failed (can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published)\nError: errors:\n can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published \n" exitCode: 1 - stderr: | - can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published - Error: errors: - can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published - args: - porchctl - rpkg @@ -51,7 +49,7 @@ commands: - --namespace=rpkg-lifecycle - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 stdout: | - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 rejected + git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 no longer proposed for approval - args: - porchctl - rpkg @@ -67,11 +65,8 @@ commands: - propose-delete - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 - --namespace=rpkg-lifecycle + stderr: "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 failed (can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published)\nError: errors:\n can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published \n" exitCode: 1 - stderr: | - can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published - Error: errors: - can only propose published packages for deletion; package git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 is not published - args: - porchctl - rpkg @@ -112,11 +107,8 @@ commands: - delete - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 - --namespace=rpkg-lifecycle + stderr: "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 failed (admission webhook \"packagerevdeletion.google.com\" denied the request: failed to delete package revision \"git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034\": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion)\nError: errors:\n admission webhook \"packagerevdeletion.google.com\" denied the request: failed to delete package revision \"git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034\": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion \n" exitCode: 1 - stderr: | - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 failed (admission webhook "packagerevdeletion.google.com" denied the request: failed to delete package revision "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion) - Error: errors: - admission webhook "packagerevdeletion.google.com" denied the request: failed to delete package revision "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion - args: - porchctl - rpkg @@ -156,10 +148,7 @@ commands: - reject - git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 - --namespace=rpkg-lifecycle - stderr: | - cannot reject git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 with lifecycle 'Published' - Error: errors: - cannot reject git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 with lifecycle 'Published' + stderr: "git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 failed (cannot reject git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 with lifecycle 'Published')\nError: errors:\n cannot reject git-017a8366a5e0d9b35ae6dc489d4d3f68046d6034 with lifecycle 'Published' \n" exitCode: 1 - args: - porchctl