Remove deletionProposed branch for main packages (#4040)

* Remove deletionProposed branch for main packages Signed-off-by: Morten Torkildsen <mortent@google.com> * Added e2e test Signed-off-by: Morten Torkildsen <mortent@google.com> * Allow undo propose-delete from kpt cli Signed-off-by: Morten Torkildsen <mortent@google.com> --------- Signed-off-by: Morten Torkildsen <mortent@google.com>
kptdev · Sep 20, 2023 · 0604fa8 · 0604fa8
1 parent d90ce91
commit 0604fa8
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 40 deletions.
diff --git a/internal/util/porch/approval.go b/internal/util/porch/approval.go
@@ -44,7 +44,7 @@ func UpdatePackageRevisionApproval(ctx context.Context, client rest.Interface, k
 	}
 
 	switch lifecycle := pr.Spec.Lifecycle; lifecycle {
-	case v1alpha1.PackageRevisionLifecycleProposed:
+	case v1alpha1.PackageRevisionLifecycleProposed, v1alpha1.PackageRevisionLifecycleDeletionProposed:
 		// ok
 	case new:
 		// already correct value

diff --git a/porch/pkg/git/git.go b/porch/pkg/git/git.go
@@ -415,21 +415,8 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor
 		refSpecs.AddRefToDelete(ref)
 
 		// If this revision was proposed for deletion, we need to delete the associated branch.
-		refSpecsForDeletionProposed := newPushRefSpecBuilder()
-		deletionProposedBranch := createDeletionProposedName(oldGit.path, oldGit.revision)
-		refSpecsForDeletionProposed.AddRefToDelete(plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), oldGit.commit))
-		if err := r.pushAndCleanup(ctx, refSpecsForDeletionProposed); err != nil {
-			if strings.HasPrefix(err.Error(),
-				fmt.Sprintf("remote ref %s%s required to be", branchPrefixInRemoteRepo, deletionProposedBranch)) &&
-				strings.HasSuffix(err.Error(), "but is absent") {
-
-				// the deletionProposed branch might not have existed, so we ignore this error
-				klog.Warningf("branch %s does not exist", deletionProposedBranch)
-
-			} else {
-				klog.Errorf("unexpected error while removing deletionProposed branch: %v", err)
-				return err
-			}
+		if err := r.removeDeletionProposedBranchIfExists(ctx, oldGit.path, oldGit.revision, oldGit.commit); err != nil {
+			return err
 		}
 
 	case isDraftBranchNameInLocal(rn), isProposedBranchNameInLocal(rn):
@@ -443,6 +430,12 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor
 			return err
 		}
 
+		// Remove the proposed for deletion branch. We end up here when users
+		// try to delete the main branch version of a packagerevision.
+		if err := r.removeDeletionProposedBranchIfExists(ctx, oldGit.path, oldGit.revision, oldGit.commit); err != nil {
+			return err
+		}
+
 		// Update the reference
 		refSpecs.AddRefToPush(commitHash, rn)
 
@@ -457,6 +450,26 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor
 	return nil
 }
 
+func (r *gitRepository) removeDeletionProposedBranchIfExists(ctx context.Context, path, revision string, commit plumbing.Hash) error {
+	refSpecsForDeletionProposed := newPushRefSpecBuilder()
+	deletionProposedBranch := createDeletionProposedName(path, revision)
+	refSpecsForDeletionProposed.AddRefToDelete(plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), commit))
+	if err := r.pushAndCleanup(ctx, refSpecsForDeletionProposed); err != nil {
+		if strings.HasPrefix(err.Error(),
+			fmt.Sprintf("remote ref %s%s required to be", branchPrefixInRemoteRepo, deletionProposedBranch)) &&
+			strings.HasSuffix(err.Error(), "but is absent") {
+
+			// the deletionProposed branch might not have existed, so we ignore this error
+			klog.Warningf("branch %s does not exist", deletionProposedBranch)
+
+		} else {
+			klog.Errorf("unexpected error while removing deletionProposed branch: %v", err)
+			return err
+		}
+	}
+	return nil
+}
+
 func (r *gitRepository) GetPackageRevision(ctx context.Context, version, path string) (repository.PackageRevision, kptfilev1.GitLock, error) {
 	ctx, span := tracer.Start(ctx, "gitRepository::GetPackageRevision", trace.WithAttributes())
 	defer span.End()

diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go
@@ -1007,35 +1007,53 @@ func (t *PorchSuite) TestProposeDeleteAndUndo(ctx context.Context) {
 	t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{})
 	t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg)
 
-	// Propose deletion
-	pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
-	t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{})
+	t.waitUntilMainBranchPackageRevisionExists(ctx, packageName)
 
-	// Undo proposal of deletion
-	pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished
-	t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{})
+	var list porchapi.PackageRevisionList
+	t.ListF(ctx, &list, client.InNamespace(t.namespace))
 
-	// Try to delete the package. This should fail because the lifecycle should be changed back to Published.
-	t.DeleteL(ctx, &porchapi.PackageRevision{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: t.namespace,
-			Name:      created.Name,
-		},
-	})
-	t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: created.Name}, &pkg)
+	for i := range list.Items {
+		pkgRev := list.Items[i]
+		t.Run(fmt.Sprintf("revision %s", pkgRev.Spec.Revision), func(newT *testing.T) {
+			// This is a bit awkward, we should find a better way to allow subtests
+			// with our custom implmentation of t.
+			oldT := t.T
+			t.T = newT
+			defer func() {
+				t.T = oldT
+			}()
+
+			// Propose deletion
+			pkgRev.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
+			t.UpdateApprovalF(ctx, &pkgRev, metav1.UpdateOptions{})
+
+			// Undo proposal of deletion
+			pkgRev.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished
+			t.UpdateApprovalF(ctx, &pkgRev, metav1.UpdateOptions{})
+
+			// Try to delete the package. This should fail because the lifecycle should be changed back to Published.
+			t.DeleteL(ctx, &porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: t.namespace,
+					Name:      pkgRev.Name,
+				},
+			})
+			t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: pkgRev.Name}, &pkgRev)
 
-	// Propose deletion and then delete the package
-	pkg.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
-	t.UpdateApprovalF(ctx, &pkg, metav1.UpdateOptions{})
+			// Propose deletion and then delete the package
+			pkgRev.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
+			t.UpdateApprovalF(ctx, &pkgRev, metav1.UpdateOptions{})
 
-	t.DeleteE(ctx, &porchapi.PackageRevision{
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: t.namespace,
-			Name:      created.Name,
-		},
-	})
+			t.DeleteE(ctx, &porchapi.PackageRevision{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: t.namespace,
+					Name:      pkgRev.Name,
+				},
+			})
 
-	t.mustNotExist(ctx, &pkg)
+			t.mustNotExist(ctx, &pkgRev)
+		})
+	}
 }
 
 func (t *PorchSuite) TestDeleteAndRecreate(ctx context.Context) {
@@ -2570,3 +2588,25 @@ func (t *PorchSuite) waitUntilObjectDeleted(ctx context.Context, gvk schema.Grou
 		t.Errorf("Object %s not deleted after %s: %v", namespacedName.String(), d.String(), innerErr)
 	}
 }
+
+func (t *PorchSuite) waitUntilMainBranchPackageRevisionExists(ctx context.Context, pkgName string) {
+	err := wait.PollImmediateWithContext(ctx, time.Second, 120*time.Second, func(ctx context.Context) (done bool, err error) {
+		var pkgRevList porchapi.PackageRevisionList
+		if err := t.client.List(ctx, &pkgRevList); err != nil {
+			t.Logf("error listing packages: %v", err)
+			return false, nil
+		}
+		for _, pkgRev := range pkgRevList.Items {
+			pkgName := pkgRev.Spec.PackageName
+			pkgRevision := pkgRev.Spec.Revision
+			if pkgRevision == "main" &&
+				pkgName == pkgRev.Spec.PackageName {
+				return true, nil
+			}
+		}
+		return false, nil
+	})
+	if err != nil {
+		t.Fatalf("Main branch package revision for %s not found", pkgName)
+	}
+}