From 0604fa8cc076192baad7c2b49c76f78a1e9a4a6d Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Wed, 20 Sep 2023 18:12:42 +0000 Subject: [PATCH] Remove deletionProposed branch for main packages (#4040) * Remove deletionProposed branch for main packages Signed-off-by: Morten Torkildsen * Added e2e test Signed-off-by: Morten Torkildsen * Allow undo propose-delete from kpt cli Signed-off-by: Morten Torkildsen --------- Signed-off-by: Morten Torkildsen --- internal/util/porch/approval.go | 2 +- porch/pkg/git/git.go | 43 ++++++++++------ porch/test/e2e/e2e_test.go | 88 ++++++++++++++++++++++++--------- 3 files changed, 93 insertions(+), 40 deletions(-) diff --git a/internal/util/porch/approval.go b/internal/util/porch/approval.go index 57bae25ac..40e76256d 100644 --- 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 index ae82ba4b2..3245e0896 100644 --- 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 index d3c55220d..e751d21fe 100644 --- 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) + } +}