Merge pull request #115 from Nordix/add_pkgrev_cond_check

Add readiness check to porchctl propose/approve

api/porch/v1alpha1/util.go

@@ -1,4 +1,4 @@
-// Copyright 2022 The kpt and Nephio Authors
+// Copyright 2022-2024 The kpt and Nephio Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -17,3 +17,25 @@ package v1alpha1
 func LifecycleIsPublished(lifecycle PackageRevisionLifecycle) bool {
 	return lifecycle == PackageRevisionLifecyclePublished || lifecycle == PackageRevisionLifecycleDeletionProposed
 }
+
+
+// Check ReadinessGates checks if the package has met all readiness gates
+func PackageRevisionIsReady(readinessGates []ReadinessGate, conditions []Condition) bool {
+	// Index our conditions
+	conds := make(map[string]Condition)
+	for _, c := range conditions {
+		conds[c.Type] = c
+	}
+
+	// Check if the readiness gates are met
+	for _, g := range readinessGates {
+		if _, ok := conds[g.ConditionType]; !ok {
+			return false
+		}
+		if conds[g.ConditionType].Status != "True" {
+			return false
+		}
+	}
+
+	return true
+}

internal/kpt/util/porch/approval.go

@@ -21,23 +21,25 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+const ApproveErrorOut = "cannot change approval from %s to %s"
+
 func UpdatePackageRevisionApproval(ctx context.Context, client client.Client, pr *v1alpha1.PackageRevision, new v1alpha1.PackageRevisionLifecycle) error {
 
 	switch lifecycle := pr.Spec.Lifecycle; lifecycle {
 	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)
+			return fmt.Errorf(ApproveErrorOut, lifecycle, new)
 		}
 	case v1alpha1.PackageRevisionLifecycleDeletionProposed:
 		if new != v1alpha1.PackageRevisionLifecyclePublished {
-			return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new)
+			return fmt.Errorf(ApproveErrorOut, lifecycle, new)
 		}
 	case new:
 		// already correct value
 		return nil
 	default:
-		return fmt.Errorf("cannot change approval from %s to %s", lifecycle, new)
+		return fmt.Errorf(ApproveErrorOut, lifecycle, new)
 	}
 
 	pr.Spec.Lifecycle = new

pkg/apiserver/webhooks_test.go

@@ -167,7 +167,7 @@ func TestWatchCertificates(t *testing.T) {
 	go watchCertificates(ctx, "Dummy Directory that does not exist", certFile, keyFile)
 
 	invalid_watch_entity_logs := captureStderr(func() {
-		time.Sleep(100 * time.Millisecond) // Give some time for the logs to be flushed
+		time.Sleep(1 * time.Second) // Give some time for the logs to be flushed
 	})
 	t.Log(invalid_watch_entity_logs)
 	err = assertLogMessages(invalid_watch_entity_logs)
@@ -183,7 +183,7 @@ func TestWatchCertificates(t *testing.T) {
 	require.NoError(t, err)
 
 	valid_reload_logs := captureStderr(func() {
-		time.Sleep(100 * time.Millisecond) // Give some time for the logs to be flushed
+		time.Sleep(1 * time.Second) // Give some time for the logs to be flushed
 	})
 	t.Log(valid_reload_logs)
 	err = assertLogMessages(valid_reload_logs)
@@ -196,7 +196,7 @@ func TestWatchCertificates(t *testing.T) {
 	require.NoError(t, err)
 
 	invalid_reload_logs := captureStderr(func() {
-		time.Sleep(100 * time.Millisecond)
+		time.Sleep(1 * time.Second)
 	})
 	t.Log(invalid_reload_logs)
 	err = assertLogMessages(invalid_reload_logs)
@@ -206,7 +206,7 @@ func TestWatchCertificates(t *testing.T) {
 	// trigger graceful termination
 	cancel()
 	graceful_termination_logs := captureStderr(func() {
-		time.Sleep(100 * time.Millisecond)
+		time.Sleep(1 * time.Second)
 	})
 	t.Log(graceful_termination_logs)
 	err = assertLogMessages(graceful_termination_logs)

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

@@ -93,6 +93,9 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
 			if err := r.client.Get(r.ctx, key, &pr); err != nil {
 				return err
 			}
+			if !v1alpha1.PackageRevisionIsReady(pr.Spec.ReadinessGates, pr.Status.Conditions) {
+				return fmt.Errorf("readiness conditions not met")
+			}
 			return porch.UpdatePackageRevisionApproval(r.ctx, r.client, &pr, v1alpha1.PackageRevisionLifecyclePublished)
 		})
 		if err != nil {

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

@@ -71,7 +71,7 @@ func TestCmd(t *testing.T) {
 						Name:      pkgRevName,
 					}}).Build(),
 		},
-		"Approve draft package": {
+		"Fail to Approve draft package": {
 			wantErr: true,
 			output:  pkgRevName + " failed (cannot change approval from Draft to Published)\n",
 			fakeclient: fake.NewClientBuilder().WithScheme(scheme).
@@ -89,6 +89,37 @@ func TestCmd(t *testing.T) {
 						Name:      pkgRevName,
 					}}).Build(),
 		},
+		"Fail to Approve unready package": {
+			wantErr: true,
+			output:  pkgRevName + " failed (readiness conditions not met)\n",
+			fakeclient: fake.NewClientBuilder().WithScheme(scheme).
+				WithObjects(&porchapi.PackageRevision{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       "PackageRevision",
+						APIVersion: porchapi.SchemeGroupVersion.Identifier(),
+					},
+					Spec: porchapi.PackageRevisionSpec{
+						Lifecycle:      porchapi.PackageRevisionLifecycleProposed,
+						RepositoryName: repoName,
+						ReadinessGates: []porchapi.ReadinessGate {
+							{
+								ConditionType: "nephio.org.Specializer.specialize",
+							},
+						},
+					},
+					Status: porchapi.PackageRevisionStatus{
+						Conditions: []porchapi.Condition {
+							{
+								Type: "nephio.org.Specializer.specialize",
+								Status: "False",
+							},
+						},
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: ns,
+						Name:      pkgRevName,
+					}}).Build(),
+		},
 		"Approve published package": {
 			output: pkgRevName + " approved\n",
 			fakeclient: fake.NewClientBuilder().WithScheme(scheme).

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

@@ -93,7 +93,9 @@ func (r *runner) runE(_ *cobra.Command, args []string) error {
 			if err := r.client.Get(r.ctx, key, &pr); err != nil {
 				return err
 			}
-
+			if !v1alpha1.PackageRevisionIsReady(pr.Spec.ReadinessGates, pr.Status.Conditions) {
+				return fmt.Errorf("readiness conditions not met")
+			}
 			switch pr.Spec.Lifecycle {
 			case v1alpha1.PackageRevisionLifecycleDraft:
 				pr.Spec.Lifecycle = v1alpha1.PackageRevisionLifecycleProposed

test/e2e/cli/testdata/rpkg-unready/config.yaml

@@ -0,0 +1,94 @@
+commands:
+  - args:
+      - porchctl
+      - repo
+      - register
+      - --namespace=rpkg-unready
+      - --name=git
+      - http://git-server.test-git-namespace.svc.cluster.local:8080/rpkg-unready
+  - args:
+      - porchctl
+      - rpkg
+      - clone
+      - --namespace=rpkg-unready
+      - https://github.com/nephio-project/porch.git
+      - --directory=test/pkgs/unready/
+      - --ref=test/pkgs/unready/v1
+      - --repository=git
+      - --workspace=clone-1
+      - unready-edit
+    stdout: |
+      git-0e15bfcaef2194d2369902c6e77c8905e9190cbb created
+  - args:
+      - porchctl
+      - rpkg
+      - copy
+      - --namespace=rpkg-unready
+      - --workspace=copy-2
+      - --replay-strategy=true
+      - git-0e15bfcaef2194d2369902c6e77c8905e9190cbb
+    stdout: "git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c created\n"
+  - args:
+      - porchctl
+      - rpkg
+      - propose
+      - --namespace=rpkg-unready
+      - git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+    stderr: "git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c failed (readiness conditions not met)\nError: errors:\n  readiness conditions not met \n"
+    exitCode: 1
+  - args:
+      - porchctl
+      - rpkg
+      - approve
+      - --namespace=rpkg-unready
+      - git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+    stderr: "git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c failed (readiness conditions not met)\nError: errors:\n  readiness conditions not met \n"
+    exitCode: 1
+  - args:
+      - porchctl
+      - rpkg
+      - pull
+      - --namespace=rpkg-unready
+      - git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+      - /tmp/porch-e2e/pkg-unready-git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+  - args:
+      - kpt
+      - fn
+      - eval
+      - --image
+      - gcr.io/kpt-fn/search-replace:v0.2.0
+      - --match-kind
+      - Kptfile
+      - /tmp/porch-e2e/pkg-unready-git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+      - --
+      - by-path=status.conditions[0].status
+      - put-value=True
+    stderr: "[RUNNING] \"gcr.io/kpt-fn/search-replace:v0.2.0\" on 1 resource(s)\n  Results:\n    [info] status.conditions[0].status: Mutated field value to \"\\\"True\\\"\"\n"
+  - args:
+      - porchctl
+      - rpkg
+      - push
+      - --namespace=rpkg-unready
+      - git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+      - /tmp/porch-e2e/pkg-unready-git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+    stdout: |
+      git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c pushed
+  - args:
+      - porchctl
+      - rpkg
+      - propose
+      - --namespace=rpkg-unready
+      - git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+    stdout: |
+      git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c proposed
+    exitCode: 0
+  - args:
+      - porchctl
+      - rpkg
+      - approve
+      - --namespace=rpkg-unready
+      - git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c
+    stdout: |
+      git-8387be408ec830b27c9dff4f90bbc6aacaccdd1c approved
+    exitCode: 0
+