Issue #615 - review comment fixes

- Edit operation now includes ReadinessGates and Conditions passed
  in as part of the edit request
  - adding them to existing ones from the Kptfile in the
    PackageRevision being edited
- Init, Edit, and Clone operations now merge their lists of
  Conditions/ReadinessGates (any passed in in the API request
  are added to the defaults/existing ones)
  - instead of using slices.Concat which allowed duplicates
    (with same Condition.Type) to stay
  - Conditions/ReadinessGates passed in in the API request
    will OVERRIDE default/existing ones

nephio-project/nephio#615

api/porch/v1alpha1/types.go

@@ -142,16 +142,6 @@ const (
 	TaskTypeUpdate TaskType = "update"
 )
 
-func (task *Task) TaskTypeOneOf(oneOf ...TaskType) bool {
-	taskType := task.Type
-	for _, tt := range oneOf {
-		if taskType == tt {
-			return true
-		}
-	}
-	return false
-}
-
 type Task struct {
 	Type   TaskType               `json:"type"`
 	Init   *PackageInitTaskSpec   `json:"init,omitempty"`

api/porch/v1alpha1/util.go

@@ -14,6 +14,8 @@
 
 package v1alpha1
 
+import "slices"
+
 func LifecycleIsPublished(lifecycle PackageRevisionLifecycle) bool {
 	return lifecycle == PackageRevisionLifecyclePublished || lifecycle == PackageRevisionLifecycleDeletionProposed
 }
@@ -38,3 +40,8 @@ func PackageRevisionIsReady(readinessGates []ReadinessGate, conditions []Conditi
 
 	return true
 }
+
+func (task *Task) TaskTypeOneOf(oneOf ...TaskType) bool {
+	taskType := task.Type
+	return slices.Contains(oneOf, taskType)
+}

controllers/main.go

@@ -137,8 +137,7 @@ func run(ctx context.Context) error {
 		Client: client.Options{
 			Cache: &client.CacheOptions{
 				DisableFor: []client.Object{
-					&porchapi.PackageRevisionResources{},
-					&porchapi.PackageRevision{}},
+					&porchapi.PackageRevisionResources{}},
 			},
 		},
 	}

controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller.go

@@ -1,4 +1,4 @@
-// Copyright 2022, 2025 The kpt and Nephio Authors
+// Copyright 2022, 2024-2025 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.
@@ -51,6 +51,12 @@ func (o *Options) BindFlags(_ string, _ *flag.FlagSet) {}
 // PackageVariantReconciler reconciles a PackageVariant object
 type PackageVariantReconciler struct {
 	client.Client
+
+	// Reader goes directly to the Kubernetes API server and
+	// may be used for Get and List operations if you need to
+	// bypass the cache in Client (which may be stale).
+	client.Reader
+
 	Options
 }
 
@@ -184,7 +190,7 @@ func (r *PackageVariantReconciler) init(ctx context.Context,
 	}
 
 	var prList porchapi.PackageRevisionList
-	if err := r.Client.List(ctx, &prList, client.InNamespace(pv.Namespace)); err != nil {
+	if err := r.Reader.List(ctx, &prList, client.InNamespace(pv.Namespace)); err != nil {
 		return nil, nil, err
 	}
 
@@ -382,7 +388,7 @@ func (r *PackageVariantReconciler) ensurePackageVariant(ctx context.Context,
 		}
 	}
 
-	if err := r.Client.Get(ctx, types.NamespacedName{Name: newPR.GetName(), Namespace: newPR.GetNamespace()}, newPR); err != nil {
+	if err := r.Reader.Get(ctx, types.NamespacedName{Name: newPR.GetName(), Namespace: newPR.GetNamespace()}, newPR); err != nil {
 		return nil, err
 	}
 	setPrStatusCondition(newPR, ConditionPipelinePVRevisionReady)
@@ -835,6 +841,7 @@ func (r *PackageVariantReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	}
 
 	r.Client = mgr.GetClient()
+	r.Reader = mgr.GetAPIReader()
 
 	//TODO: establish watches on resource types injected in all the Package Revisions
 	//      we own, and use those to generate requests

pkg/task/clone.go

@@ -19,7 +19,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"slices"
 
 	api "github.com/nephio-project/porch/api/porch/v1alpha1"
 	configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1"
@@ -29,6 +28,7 @@ import (
 	"github.com/nephio-project/porch/pkg/kpt"
 	kptfile "github.com/nephio-project/porch/pkg/kpt/api/kptfile/v1"
 	"github.com/nephio-project/porch/pkg/repository"
+	"github.com/nephio-project/porch/pkg/util"
 	"go.opentelemetry.io/otel/trace"
 	"k8s.io/klog/v2"
 )
@@ -39,12 +39,7 @@ type clonePackageMutation struct {
 	pkgRev *api.PackageRevision
 	task   *api.Task
 
-	// namespace is the namespace against which we resolve references.
-	// TODO: merge namespace into referenceResolver?
-	namespace string
-
-	name               string // package target name
-	isDeployment       bool   // is the package deployable instance
+	isDeployment       bool // is the package deployable instance
 	repoOpener         repository.RepositoryOpener
 	credentialResolver repository.CredentialResolver
 	referenceResolver  repository.ReferenceResolver
@@ -98,8 +93,10 @@ func (m *clonePackageMutation) apply(ctx context.Context, resources repository.P
 		if file.Status == nil {
 			file.Status = &kptfile.Status{}
 		}
-		file.Status.Conditions = slices.Concat(file.Status.Conditions, kptfile.ConvertApiConditions(m.pkgRev.Status.Conditions))
-		file.Info.ReadinessGates = slices.Concat(file.Info.ReadinessGates, kptfile.ConvertApiReadinessGates(m.pkgRev.Spec.ReadinessGates))
+		file.Status.Conditions = util.MergeFunc(file.Status.Conditions, kptfile.ConvertApiConditions(m.pkgRev.Status.Conditions), func(existingCondition, cloneInput kptfile.Condition) bool {
+			return existingCondition.Type == cloneInput.Type
+		})
+		file.Info.ReadinessGates = util.Merge(file.Info.ReadinessGates, kptfile.ConvertApiReadinessGates(m.pkgRev.Spec.ReadinessGates))
 	})
 
 	// ensure merge-key comment is added to newly added resources.
@@ -122,7 +119,7 @@ func (m *clonePackageMutation) cloneFromRegisteredRepository(ctx context.Context
 	upstreamRevision, err := (&repository.PackageFetcher{
 		RepoOpener:        m.repoOpener,
 		ReferenceResolver: m.referenceResolver,
-	}).FetchRevision(ctx, ref, m.namespace)
+	}).FetchRevision(ctx, ref, m.pkgRev.Namespace)
 	if err != nil {
 		return repository.PackageResources{}, fmt.Errorf("failed to fetch package revision %q: %w", ref.Name, err)
 	}
@@ -138,7 +135,7 @@ func (m *clonePackageMutation) cloneFromRegisteredRepository(ctx context.Context
 	}
 
 	// Update Kptfile
-	if err := kpt.UpdateKptfileUpstream(m.name, resources.Spec.Resources, upstream, lock); err != nil {
+	if err := kpt.UpdateKptfileUpstream(m.pkgRev.Spec.PackageName, resources.Spec.Resources, upstream, lock); err != nil {
 		return repository.PackageResources{}, fmt.Errorf("failed to apply upstream lock to package %q: %w", ref.Name, err)
 	}
 
@@ -188,7 +185,7 @@ func (m *clonePackageMutation) cloneFromGit(ctx context.Context, gitPackage *api
 	contents := resources.Spec.Resources
 
 	// Update Kptfile
-	if err := kpt.UpdateKptfileUpstream(m.name, contents, kptfile.Upstream{
+	if err := kpt.UpdateKptfileUpstream(m.pkgRev.Spec.PackageName, contents, kptfile.Upstream{
 		Type: kptfile.GitOrigin,
 		Git: &kptfile.Git{
 			Repo:      lock.Repo,

pkg/task/clone_test.go

@@ -227,7 +227,11 @@ func TestCloneGitBasicAuth(t *testing.T) {
 	addr := startGitServer(t, repo)
 
 	cpm := clonePackageMutation{
-		pkgRev: &v1alpha1.PackageRevision{},
+		pkgRev: &v1alpha1.PackageRevision{
+			Spec: v1alpha1.PackageRevisionSpec{
+				PackageName: "test-configmap",
+			},
+		},
 		task: &v1alpha1.Task{
 			Type: "clone",
 			Clone: &v1alpha1.PackageCloneTaskSpec{
@@ -244,8 +248,7 @@ func TestCloneGitBasicAuth(t *testing.T) {
 				},
 			},
 		},
-		namespace: "test-namespace",
-		name:      "test-configmap",
+
 		credentialResolver: &credentialResolver{
 			username: "",
 			password: "",

pkg/task/edit.go

@@ -19,15 +19,15 @@ import (
 	"fmt"
 
 	api "github.com/nephio-project/porch/api/porch/v1alpha1"
+	kptfile "github.com/nephio-project/porch/pkg/kpt/api/kptfile/v1"
 	"github.com/nephio-project/porch/pkg/repository"
+	"github.com/nephio-project/porch/pkg/util"
 	"go.opentelemetry.io/otel/trace"
 )
 
 type editPackageMutation struct {
+	pkgRev            *api.PackageRevision
 	task              *api.Task
-	namespace         string
-	repositoryName    string
-	packageName       string
 	repoOpener        repository.RepositoryOpener
 	referenceResolver repository.ReferenceResolver
 }
@@ -39,32 +39,63 @@ func (m *editPackageMutation) apply(ctx context.Context, resources repository.Pa
 	defer span.End()
 
 	sourceRef := m.task.Edit.Source
+	oldPkgRev := m.pkgRev
 
-	revision, err := (&repository.PackageFetcher{
+	newPkgRev, err := (&repository.PackageFetcher{
 		RepoOpener:        m.repoOpener,
 		ReferenceResolver: m.referenceResolver,
-	}).FetchRevision(ctx, sourceRef, m.namespace)
+	}).FetchRevision(ctx, sourceRef, oldPkgRev.Namespace)
 	if err != nil {
 		return repository.PackageResources{}, nil, fmt.Errorf("failed to fetch package %q: %w", sourceRef.Name, err)
 	}
 
 	// We only allow edit to create new revision from the same package.
-	if revision.Key().Package != m.packageName ||
-		revision.Key().Repository != m.repositoryName {
-		return repository.PackageResources{}, nil, fmt.Errorf("source revision must be from same package %s/%s", m.repositoryName, m.packageName)
+	if newPkgRev.Key().Package != oldPkgRev.Spec.PackageName ||
+		newPkgRev.Key().Repository != oldPkgRev.Spec.RepositoryName {
+		return repository.PackageResources{}, nil, fmt.Errorf(
+			"source revision must be from same package %s/%s (got: %s/%s)",
+			oldPkgRev.Spec.RepositoryName,
+			oldPkgRev.Spec.PackageName,
+			newPkgRev.Key().Repository,
+			newPkgRev.Key().Package)
 	}
 
 	// We only allow edit to create new revisions from published packages.
-	if !api.LifecycleIsPublished(revision.Lifecycle(ctx)) {
+	if !api.LifecycleIsPublished(newPkgRev.Lifecycle(ctx)) {
 		return repository.PackageResources{}, nil, fmt.Errorf("source revision must be published")
 	}
 
-	sourceResources, err := revision.GetResources(ctx)
+	sourceResources, err := newPkgRev.GetResources(ctx)
 	if err != nil {
 		return repository.PackageResources{}, nil, fmt.Errorf("cannot read contents of package %q: %w", sourceRef.Name, err)
 	}
 
-	return repository.PackageResources{
+	editedResources := repository.PackageResources{
 		Contents: sourceResources.Spec.Resources,
-	}, &api.TaskResult{Task: m.task}, nil
+	}
+	editedResources.EditKptfile(func(file kptfile.KptFile) {
+		file.Status = &kptfile.Status{
+			Conditions: func() (inputConditions []kptfile.Condition) {
+				if file.Status == nil || file.Status.Conditions == nil {
+					inputConditions = kptfile.ConvertApiConditions(defaultConditions)
+				} else {
+					inputConditions = file.Status.Conditions
+				}
+
+				return util.MergeFunc(inputConditions, kptfile.ConvertApiConditions(oldPkgRev.Status.Conditions), func(inputCondition, oldCondition kptfile.Condition) bool {
+					return oldCondition.Type == inputCondition.Type
+				})
+			}(),
+		}
+		file.Info.ReadinessGates = func() (kptfileGates []kptfile.ReadinessGate) {
+			for _, each := range oldPkgRev.Status.Conditions {
+				kptfileGates = append(kptfileGates, kptfile.ReadinessGate{
+					ConditionType: each.Type,
+				})
+			}
+			return kptfileGates
+		}()
+	})
+
+	return editedResources, &api.TaskResult{Task: m.task}, nil
 }

pkg/task/edit_test.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/nephio-project/porch/api/porch/v1alpha1"
+	api "github.com/nephio-project/porch/api/porch/v1alpha1"
 	configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1"
 	"github.com/nephio-project/porch/pkg/externalrepo/fake"
 	kptfile "github.com/nephio-project/porch/pkg/kpt/api/kptfile/v1"
@@ -69,7 +70,19 @@ info:
 		repository: repo,
 	}
 
+	testpkg := &api.PackageRevision{
+		Spec: api.PackageRevisionSpec{
+			PackageName:    pkg,
+			RepositoryName: repositoryName,
+			ReadinessGates: []api.ReadinessGate{},
+		},
+		Status: api.PackageRevisionStatus{
+			Conditions: []api.Condition{},
+		},
+	}
+
 	epm := editPackageMutation{
+		pkgRev: testpkg,
 		task: &v1alpha1.Task{
 			Type: "edit",
 			Edit: &v1alpha1.PackageEditTaskSpec{
@@ -79,9 +92,6 @@ info:
 			},
 		},
 
-		namespace:         "test-namespace",
-		packageName:       pkg,
-		repositoryName:    repositoryName,
 		referenceResolver: &fakeReferenceResolver{},
 		repoOpener:        repoOpener,
 	}

pkg/task/generictaskhandler.go

@@ -294,8 +294,6 @@ func (th *genericTaskHandler) mapTaskToMutation(ctx context.Context, pkgRev *api
 		return &clonePackageMutation{
 			pkgRev:             pkgRev,
 			task:               task,
-			namespace:          pkgRev.Namespace,
-			name:               pkgRev.Spec.PackageName,
 			isDeployment:       isDeployment,
 			repoOpener:         th.repoOpener,
 			credentialResolver: th.credentialResolver,
@@ -328,10 +326,8 @@ func (th *genericTaskHandler) mapTaskToMutation(ctx context.Context, pkgRev *api
 			return nil, fmt.Errorf("edit not set for task of type %q", task.Type)
 		}
 		return &editPackageMutation{
+			pkgRev:            pkgRev,
 			task:              task,
-			namespace:         pkgRev.Namespace,
-			packageName:       pkgRev.Spec.PackageName,
-			repositoryName:    pkgRev.Spec.RepositoryName,
 			repoOpener:        th.repoOpener,
 			referenceResolver: th.referenceResolver,
 		}, nil

pkg/task/init.go

@@ -17,13 +17,13 @@ package task
 import (
 	"context"
 	"fmt"
-	"slices"
 
 	api "github.com/nephio-project/porch/api/porch/v1alpha1"
 	"github.com/nephio-project/porch/pkg/kpt/kptpkg"
 	"github.com/nephio-project/porch/pkg/kpt/printer"
 	"github.com/nephio-project/porch/pkg/kpt/printer/fake"
 	"github.com/nephio-project/porch/pkg/repository"
+	"github.com/nephio-project/porch/pkg/util"
 	"go.opentelemetry.io/otel/trace"
 	"sigs.k8s.io/kustomize/kyaml/filesys"
 )
@@ -55,7 +55,9 @@ func (m *initPackageMutation) apply(ctx context.Context, resources repository.Pa
 		return repository.PackageResources{}, nil, err
 	}
 
-	readinessConditions := slices.Concat(defaultConditions, m.pkgRev.Status.Conditions)
+	readinessConditions := util.MergeFunc(defaultConditions, m.pkgRev.Status.Conditions, func(aDefault, anInput api.Condition) bool {
+		return aDefault.Type == anInput.Type
+	})
 
 	name := m.pkgRev.Spec.PackageName
 	initSpec := m.task.Init