fix: deletionProposed branch throws error for main revisions (#4056)

porch/pkg/git/git.go

@@ -426,7 +426,7 @@ 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.
-		if err := r.removeDeletionProposedBranchIfExists(ctx, oldGit.path, oldGit.revision, oldGit.commit); err != nil {
+		if err := r.removeDeletionProposedBranchIfExists(ctx, oldGit.path, oldGit.revision); err != nil {
 			return err
 		}
 
@@ -443,7 +443,7 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor
 
 		// 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 {
+		if err := r.removeDeletionProposedBranchIfExists(ctx, oldGit.path, oldGit.revision); err != nil {
 			return err
 		}
 
@@ -461,18 +461,14 @@ 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 {
+func (r *gitRepository) removeDeletionProposedBranchIfExists(ctx context.Context, path, revision string) error {
 	refSpecsForDeletionProposed := newPushRefSpecBuilder()
 	deletionProposedBranch := createDeletionProposedName(path, revision)
-	refSpecsForDeletionProposed.AddRefToDelete(plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), commit))
+	refSpecsForDeletionProposed.AddRefToDelete(plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), plumbing.ZeroHash))
 	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") {
-
+		if errors.Is(err, git.NoErrAlreadyUpToDate) {
 			// 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

porch/test/e2e/e2e_test.go

@@ -1115,6 +1115,106 @@ func (t *PorchSuite) TestDeleteAndRecreate(ctx context.Context) {
 	t.Logf("successfully recreated package revision %q", packageName)
 }
 
+func (t *PorchSuite) TestDeleteFromMain(ctx context.Context) {
+	const (
+		repository        = "delete-main"
+		packageNameFirst  = "test-delete-main-first"
+		packageNameSecond = "test-delete-main-second"
+		workspace         = "workspace"
+	)
+
+	// Register the repository
+	t.registerMainGitRepositoryF(ctx, repository)
+
+	// Create the first draft package
+	createdFirst := t.createPackageDraftF(ctx, repository, packageNameFirst, workspace)
+
+	// Check the package exists
+	var pkgFirst porchapi.PackageRevision
+	t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: createdFirst.Name}, &pkgFirst)
+
+	// Propose the package revision to be finalized
+	pkgFirst.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed
+	t.UpdateF(ctx, &pkgFirst)
+
+	pkgFirst.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished
+	t.UpdateApprovalF(ctx, &pkgFirst, metav1.UpdateOptions{})
+
+	t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: createdFirst.Name}, &pkgFirst)
+
+	// Create the second draft package
+	createdSecond := t.createPackageDraftF(ctx, repository, packageNameSecond, workspace)
+
+	// Check the package exists
+	var pkgSecond porchapi.PackageRevision
+	t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: createdSecond.Name}, &pkgSecond)
+
+	// Propose the package revision to be finalized
+	pkgSecond.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed
+	t.UpdateF(ctx, &pkgSecond)
+
+	pkgSecond.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished
+	t.UpdateApprovalF(ctx, &pkgSecond, metav1.UpdateOptions{})
+
+	t.mustExist(ctx, client.ObjectKey{Namespace: t.namespace, Name: createdSecond.Name}, &pkgSecond)
+
+	// We need to wait for the sync for the "main" revisions to get created
+	time.Sleep(75 * time.Second)
+
+	var list porchapi.PackageRevisionList
+	t.ListE(ctx, &list, client.InNamespace(t.namespace))
+
+	var firstPkgRevFromMain porchapi.PackageRevision
+	var secondPkgRevFromMain porchapi.PackageRevision
+
+	for _, pkgrev := range list.Items {
+		if pkgrev.Spec.PackageName == packageNameFirst && pkgrev.Spec.Revision == "main" {
+			firstPkgRevFromMain = pkgrev
+		}
+		if pkgrev.Spec.PackageName == packageNameSecond && pkgrev.Spec.Revision == "main" {
+			secondPkgRevFromMain = pkgrev
+		}
+	}
+
+	// Propose deletion of both main packages
+	firstPkgRevFromMain.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
+	t.UpdateApprovalF(ctx, &firstPkgRevFromMain, metav1.UpdateOptions{})
+	secondPkgRevFromMain.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
+	t.UpdateApprovalF(ctx, &secondPkgRevFromMain, metav1.UpdateOptions{})
+
+	// Delete the first package revision from main
+	t.DeleteE(ctx, &porchapi.PackageRevision{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: t.namespace,
+			Name:      firstPkgRevFromMain.Name,
+		},
+	})
+
+	// We need to wait for the sync
+	time.Sleep(75 * time.Second)
+
+	// Delete the second package revision from main
+	t.DeleteE(ctx, &porchapi.PackageRevision{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: t.namespace,
+			Name:      secondPkgRevFromMain.Name,
+		},
+	})
+
+	// Propose and delete the original package revisions (cleanup)
+	t.ListE(ctx, &list, client.InNamespace(t.namespace))
+	for _, pkgrev := range list.Items {
+		pkgrev.Spec.Lifecycle = porchapi.PackageRevisionLifecycleDeletionProposed
+		t.UpdateApprovalF(ctx, &pkgrev, metav1.UpdateOptions{})
+		t.DeleteE(ctx, &porchapi.PackageRevision{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: t.namespace,
+				Name:      pkgrev.Name,
+			},
+		})
+	}
+}
+
 func (t *PorchSuite) TestCloneLeadingSlash(ctx context.Context) {
 	const (
 		repository  = "clone-ls"