From 8af6e4ef452a169ffb1c89714b0c13f260f6fd8a Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 30 Apr 2025 15:20:04 +0200 Subject: [PATCH 1/9] Add retries packageRevisionDraft is closed --- pkg/externalrepo/git/git.go | 159 ++++++++------- pkg/externalrepo/git/git_test.go | 336 +++++++++++++++++++++++++++++++ 2 files changed, 418 insertions(+), 77 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 4c9adda0c..7c48599f6 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -28,7 +28,6 @@ import ( "time" "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/object" @@ -1491,79 +1490,99 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos var newRef *plumbing.Reference - switch d.lifecycle { - case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed: + err := util.RetryOnErrorConditional(3, func(err error) bool { + return pkgerrors.Is(err, conflictingRequiredRemoteRefError) + }, + func(retryNumber int) error { + klog.Infof("Deleting PackageRevision try number %d", retryNumber) - if version == 0 { - return nil, pkgerrors.New("Version cannot be empty for the next package revision") - } - d.prKey.Revision = version + if retryNumber > 0 { + err := r.fetchRemoteRepository(ctx) + if err != nil { + return err + } + } - // Finalize the package revision. Commit it to main branch. - commitHash, newTreeHash, commitBase, err := r.commitPackageToMain(ctx, d) - if err != nil { - return nil, err - } + switch d.lifecycle { + case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed: - tag := createFinalTagNameInLocal(d.Key()) - refSpecs.AddRefToPush(commitHash, r.branch.RefInLocal()) // Push new main branch - refSpecs.AddRefToPush(commitHash, tag) // Push the tag - refSpecs.RequireRef(commitBase) // Make sure main didn't advance + if version == 0 { + return pkgerrors.New("Version cannot be empty for the next package revision") + } + d.prKey.Revision = version - // Delete base branch (if one exists and should be deleted) - switch base := d.base; { - case base == nil: // no branch to delete - case base.Name() == draftBranch.RefInLocal(), base.Name() == proposedBranch.RefInLocal(): - refSpecs.AddRefToDelete(base) - } + // Finalize the package revision. Commit it to main branch. + commitHash, newTreeHash, commitBase, err := r.commitPackageToMain(ctx, d) + if err != nil { + return err + } - // Update package draft - d.commit = commitHash - d.tree = newTreeHash - newRef = plumbing.NewHashReference(tag, commitHash) + tag := createFinalTagNameInLocal(d.Key()) + refSpecs.AddRefToPush(commitHash, r.branch.RefInLocal()) // Push new main branch + refSpecs.AddRefToPush(commitHash, tag) // Push the tag + refSpecs.RequireRef(commitBase) // Make sure main didn't advance - case v1alpha1.PackageRevisionLifecycleProposed: - // Push the package revision into a proposed branch. - refSpecs.AddRefToPush(d.commit, proposedBranch.RefInLocal()) + // Delete base branch (if one exists and should be deleted) + switch base := d.base; { + case base == nil: // no branch to delete + case base.Name() == draftBranch.RefInLocal(), base.Name() == proposedBranch.RefInLocal(): + refSpecs.AddRefToDelete(base) + } - // Delete base branch (if one exists and should be deleted) - switch base := d.base; { - case base == nil: // no branch to delete - case base.Name() != proposedBranch.RefInLocal(): - refSpecs.AddRefToDelete(base) - } + // Update package draft + d.commit = commitHash + d.tree = newTreeHash + newRef = plumbing.NewHashReference(tag, commitHash) - // Update package referemce (commit and tree hash stay the same) - newRef = plumbing.NewHashReference(proposedBranch.RefInLocal(), d.commit) - - case v1alpha1.PackageRevisionLifecycleDraft: - // Push the package revision into a draft branch. - refSpecs.AddRefToPush(d.commit, draftBranch.RefInLocal()) - // Delete base branch (if one exists and should be deleted) - switch base := d.base; { - case base == nil: // no branch to delete - case base.Name() != draftBranch.RefInLocal(): - refSpecs.AddRefToDelete(base) - } + case v1alpha1.PackageRevisionLifecycleProposed: + // Push the package revision into a proposed branch. + refSpecs.AddRefToPush(d.commit, proposedBranch.RefInLocal()) - // Update package reference (commit and tree hash stay the same) - newRef = plumbing.NewHashReference(draftBranch.RefInLocal(), d.commit) + // Delete base branch (if one exists and should be deleted) + switch base := d.base; { + case base == nil: // no branch to delete + case base.Name() != proposedBranch.RefInLocal(): + refSpecs.AddRefToDelete(base) + } - default: - return nil, fmt.Errorf("package has unrecognized lifecycle: %q", d.lifecycle) - } + // Update package referemce (commit and tree hash stay the same) + newRef = plumbing.NewHashReference(proposedBranch.RefInLocal(), d.commit) + + case v1alpha1.PackageRevisionLifecycleDraft: + // Push the package revision into a draft branch. + refSpecs.AddRefToPush(d.commit, draftBranch.RefInLocal()) + // Delete base branch (if one exists and should be deleted) + switch base := d.base; { + case base == nil: // no branch to delete + case base.Name() != draftBranch.RefInLocal(): + refSpecs.AddRefToDelete(base) + } - if err := d.repo.pushAndCleanup(ctx, refSpecs); err != nil { - // No changes is fine. No need to return an error. - if !pkgerrors.Is(err, git.NoErrAlreadyUpToDate) { - return nil, err - } - } + // Update package reference (commit and tree hash stay the same) + newRef = plumbing.NewHashReference(draftBranch.RefInLocal(), d.commit) + + default: + return fmt.Errorf("package has unrecognized lifecycle: %q", d.lifecycle) + } - // for backwards compatibility with packages that existed before porch supported - // descriptions, we populate the workspaceName as the revision number if it is empty - if d.prKey.WorkspaceName == "" { - d.prKey.WorkspaceName = "v" + repository.Revision2Str(d.Key().Revision) + if err := d.repo.pushAndCleanup(ctx, refSpecs); err != nil { + // No changes is fine. No need to return an error. + if !pkgerrors.Is(err, git.NoErrAlreadyUpToDate) { + return err + } + } + + // for backwards compatibility with packages that existed before porch supported + // descriptions, we populate the workspaceName as the revision number if it is empty + if d.prKey.WorkspaceName == "" { + d.prKey.WorkspaceName = "v" + repository.Revision2Str(d.Key().Revision) + } + + return nil + }, + ) + if err != nil { + return nil, err } return &gitPackageRevision{ @@ -1575,6 +1594,7 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos commit: newRef.Hash(), tasks: d.tasks, }, nil + } // doGitWithAuth fetches auth information for git and provides it @@ -1608,21 +1628,6 @@ func (r *gitRepository) commitPackageToMain(ctx context.Context, d *gitPackageRe var zero plumbing.Hash - // Fetch main - switch err := r.doGitWithAuth(ctx, func(auth transport.AuthMethod) error { - return r.repo.Fetch(&git.FetchOptions{ - RemoteName: OriginName, - RefSpecs: []config.RefSpec{branch.ForceFetchSpec()}, - Auth: auth, - CABundle: r.caBundle, - }) - }); err { - case nil, git.NoErrAlreadyUpToDate: - // ok - default: - return zero, zero, nil, fmt.Errorf("failed to fetch remote repository: %w", err) - } - // Find localTarget branch localTarget, err := r.repo.Reference(localRef, false) if err != nil { diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index 26c9ecc57..1127c26de 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -20,6 +20,7 @@ import ( "flag" "fmt" "os" + "path" "path/filepath" "reflect" "strings" @@ -1858,10 +1859,345 @@ func TestApproveOnManuallyMovedMain(t *testing.T) { err = draft1.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) + if err != nil { + t.Fatalf("Failed to update lifecycle: %v", err) + } + + uip := makeUserInfoProvider(repoSpec, &mockK8sUsp{}) + + t.Logf("Adding new file to main branch") + + mainBranchCommitHash := resolveReference(t, gitRepo, DefaultMainReferenceName).Hash() + ch, err := newCommitHelper(gitRepo, uip, mainBranchCommitHash, "", plumbing.ZeroHash) + if err != nil { + t.Fatalf("Failed to create commit helper: %v", err) + } + + err = ch.storeFile(newFile, newFileContent) + if err != nil { + t.Fatalf("Failed to store new file: %v", err) + } + + newHash, _, err := ch.commit(ctx, "Add new file", "") + + if err != nil { + t.Fatalf("Failed to create commit: %v", err) + } + + err = gitRepo.Storer.SetReference(plumbing.NewHashReference(DefaultMainReferenceName, newHash)) + if err != nil { + t.Fatalf("Failed to set reference: %v", err) + } + t.Logf("Moved %s from %s to %s", DefaultMainReferenceName, mainBranchCommitHash, newHash) + + _, err = localRepo.ClosePackageRevisionDraft(ctx, draft1, 1) + + if err != nil { + t.Fatalf("Failed to close draft PackageRevision: %v", err) + } +} + +func TestApproveOnManuallyMovedProposed(t *testing.T) { + const ( + repoName = "approve-on-manually-moved-proposed-repo" + namespace = "default" + packageName = "approve-on-manually-moved-proposed-pkg" + workspace = "approve-on-manually-moved-proposed-ws" + newFile = "new-file.md" + newFileContent = "new-file-content" + ) + + tempdir := t.TempDir() + tarfile := filepath.Join("testdata", "trivial-repository.tar") + remotepath := filepath.Join(tempdir, "remote") + localpath := filepath.Join(tempdir, "local") + gitRepo, address := ServeGitRepository(t, tarfile, remotepath) + + repoSpec := &configapi.GitRepository{ + Repo: address, + } + + ctx := context.Background() + + localRepo, err := OpenRepository(ctx, repoName, namespace, repoSpec, false, localpath, GitRepositoryOptions{}) + if err != nil { + t.Fatalf("Failed to open Git repository loaded from %q: %v", remotepath, err) + } + + pr := &v1alpha1.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: repoName, + }, + Spec: v1alpha1.PackageRevisionSpec{ + PackageName: packageName, + WorkspaceName: workspace, + RepositoryName: repoName, + Tasks: []v1alpha1.Task{ + { + Type: v1alpha1.TaskTypeInit, + Init: &v1alpha1.PackageInitTaskSpec{ + Description: "Empty Package", + }, + }, + }, + }, + } + + resources := &v1alpha1.PackageRevisionResources{ + Spec: v1alpha1.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": Kptfile, + }, + }, + } + + t.Logf("Creating and pushing a PackageRevision with lifecycle Draft.") + + draft1, err := localRepo.CreatePackageRevisionDraft(ctx, pr) + if err != nil { + t.Fatalf("Failed to create draft PackageRevision %v", err) + } + + err = draft1.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDraft) + + if err != nil { + t.Fatalf("Failed to update lifecycle: %v", err) + } + + err = draft1.UpdateResources(ctx, resources, nil) + if err != nil { + t.Fatalf("Failed to update resources: %v", err) + } + + draftFinalized, err := localRepo.ClosePackageRevisionDraft(ctx, draft1, 1) + if err != nil { + t.Fatalf("Failed to close draft PackageRevision: %v", err) + } + + draft2, err := localRepo.UpdatePackageRevision(ctx, draftFinalized) + if err != nil { + t.Fatalf("Failed to create draft PackageRevision %v", err) + } + + draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleProposed) + + uip := makeUserInfoProvider(repoSpec, &mockK8sUsp{}) + + t.Logf("Adding new file to main branch") + + ch, err := newCommitHelper(gitRepo, uip, draftFinalized.(*gitPackageRevision).commit, "", plumbing.ZeroHash) + if err != nil { + t.Fatalf("Failed to create commit helper: %v", err) + } + + err = ch.storeFile(path.Join(packageName, newFile), newFileContent) + if err != nil { + t.Fatalf("Failed to store new file: %v", err) + } + + newHash, _, err := ch.commit(ctx, "Add new file", "") + + if err != nil { + t.Fatalf("Failed to create commit: %v", err) + } + + err = gitRepo.Storer.SetReference(plumbing.NewHashReference(plumbing.ReferenceName(createDraftName(draftFinalized.Key())), newHash)) + if err != nil { + t.Fatalf("Failed to set reference: %v", err) + } + t.Logf("Moved %s from %s to %s", plumbing.ReferenceName(createDraftName(draftFinalized.Key())), draftFinalized.(*gitPackageRevision).commit, newHash) + + _, err = localRepo.ClosePackageRevisionDraft(ctx, draft2, 1) + + if err != nil { + t.Fatalf("Failed to close draft PackageRevision: %v", err) + } +} + +func TestApproveOnManuallyMovedDraft(t *testing.T) { + const ( + repoName = "approve-on-manually-moved-proposed-repo" + namespace = "default" + packageName = "approve-on-manually-moved-proposed-pkg" + workspace = "approve-on-manually-moved-proposed-ws" + newFile = "new-file.md" + newFileContent = "new-file-content" + ) + + tempdir := t.TempDir() + tarfile := filepath.Join("testdata", "trivial-repository.tar") + remotepath := filepath.Join(tempdir, "remote") + localpath := filepath.Join(tempdir, "local") + gitRepo, address := ServeGitRepository(t, tarfile, remotepath) + + repoSpec := &configapi.GitRepository{ + Repo: address, + } + + ctx := context.Background() + + localRepo, err := OpenRepository(ctx, repoName, namespace, repoSpec, false, localpath, GitRepositoryOptions{}) + if err != nil { + t.Fatalf("Failed to open Git repository loaded from %q: %v", remotepath, err) + } + + pr := &v1alpha1.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: repoName, + }, + Spec: v1alpha1.PackageRevisionSpec{ + PackageName: packageName, + WorkspaceName: workspace, + RepositoryName: repoName, + Tasks: []v1alpha1.Task{ + { + Type: v1alpha1.TaskTypeInit, + Init: &v1alpha1.PackageInitTaskSpec{ + Description: "Empty Package", + }, + }, + }, + }, + } + + resources := &v1alpha1.PackageRevisionResources{ + Spec: v1alpha1.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": Kptfile, + }, + }, + } + + t.Logf("Creating and pushing a PackageRevision with lifecycle proposed.") + + draft1, err := localRepo.CreatePackageRevisionDraft(ctx, pr) + if err != nil { + t.Fatalf("Failed to create draft PackageRevision %v", err) + } + + err = draft1.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleProposed) + + if err != nil { + t.Fatalf("Failed to update lifecycle: %v", err) + } + + err = draft1.UpdateResources(ctx, resources, nil) + if err != nil { + t.Fatalf("Failed to update resources: %v", err) + } + + draftFinalized, err := localRepo.ClosePackageRevisionDraft(ctx, draft1, 1) + if err != nil { + t.Fatalf("Failed to close draft PackageRevision: %v", err) + } + + draft2, err := localRepo.UpdatePackageRevision(ctx, draftFinalized) + if err != nil { + t.Fatalf("Failed to create draft PackageRevision %v", err) + } + + draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDraft) + + uip := makeUserInfoProvider(repoSpec, &mockK8sUsp{}) + + t.Logf("Adding new file to main branch") + + ch, err := newCommitHelper(gitRepo, uip, draftFinalized.(*gitPackageRevision).commit, "", plumbing.ZeroHash) + if err != nil { + t.Fatalf("Failed to create commit helper: %v", err) + } + + err = ch.storeFile(filepath.Join(packageName, newFile), newFileContent) + if err != nil { + t.Fatalf("Failed to store new file: %v", err) + } + + newHash, _, err := ch.commit(ctx, "Add new file", "") + if err != nil { t.Fatalf("Failed to create commit: %v", err) } + err = gitRepo.Storer.SetReference(plumbing.NewHashReference(plumbing.ReferenceName(createProposedName(draftFinalized.Key())), newHash)) + if err != nil { + t.Fatalf("Failed to set reference: %v", err) + } + t.Logf("Moved %s from %s to %s", plumbing.ReferenceName(createProposedName(draftFinalized.Key())), draftFinalized.(*gitPackageRevision).commit, newHash) + + _, err = localRepo.ClosePackageRevisionDraft(ctx, draft2, 1) + + if err != nil { + t.Fatalf("Failed to close draft PackageRevision: %v", err) + } +} + +func TestDeletionProposedOnManuallyMovedMain(t *testing.T) { + const ( + repoName = "deletion-proposed-on-manually-moved-repo" + namespace = "default" + packageName = "deletion-proposed-on-manually-moved-pkg" + workspace = "deletion-proposed-on-manually-moved-ws" + newFile = "new-file.md" + newFileContent = "new-file-content" + ) + + tempdir := t.TempDir() + tarfile := filepath.Join("testdata", "trivial-repository.tar") + remotepath := filepath.Join(tempdir, "remote") + localpath := filepath.Join(tempdir, "local") + gitRepo, address := ServeGitRepository(t, tarfile, remotepath) + + repoSpec := &configapi.GitRepository{ + Repo: address, + } + + ctx := context.Background() + + localRepo, err := OpenRepository(ctx, repoName, namespace, repoSpec, false, localpath, GitRepositoryOptions{}) + if err != nil { + t.Fatalf("Failed to open Git repository loaded from %q: %v", remotepath, err) + } + + pr := &v1alpha1.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: repoName, + }, + Spec: v1alpha1.PackageRevisionSpec{ + PackageName: packageName, + WorkspaceName: workspace, + RepositoryName: repoName, + Tasks: []v1alpha1.Task{ + { + Type: v1alpha1.TaskTypeInit, + Init: &v1alpha1.PackageInitTaskSpec{ + Description: "Empty Package", + }, + }, + }, + }, + } + + prv1, err := createAndPublishPR(ctx, localRepo, pr) + if err != nil { + t.Fatalf("Failed to create published PackageRevision %v", err) + } + + apiPrV1, err := prv1.GetPackageRevision(ctx) + if err != nil { + t.Fatalf("Failed to get api PackageRevision: %v", err) + } + + draft1, err := localRepo.CreatePackageRevisionDraft(ctx, apiPrV1) + if err != nil { + t.Fatalf("Failed to create draft PackageRevision %v", err) + } + + err = draft1.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDeletionProposed) + + if err != nil { + t.Fatalf("Failed to create: %v", err) + } + uip := makeUserInfoProvider(repoSpec, &mockK8sUsp{}) mainBranchCommitHash := resolveReference(t, gitRepo, DefaultMainReferenceName).Hash() ch, err := newCommitHelper(gitRepo, uip, mainBranchCommitHash, "", plumbing.ZeroHash) From 3a0aedadabf2efd239fa7e897b6770c7161a0395 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 30 Apr 2025 15:50:57 +0200 Subject: [PATCH 2/9] Remove unused public method --- pkg/externalrepo/git/git.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 7c48599f6..8d762f7a4 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -827,12 +827,6 @@ func (r *gitRepository) loadDraft(ctx context.Context, ref *plumbing.Reference) return packageRevision, nil } -func (r *gitRepository) UpdateDeletionProposedCache() error { - r.mutex.Lock() - defer r.mutex.Unlock() - return r.updateDeletionProposedCache() -} - func (r *gitRepository) updateDeletionProposedCache() error { r.deletionProposedCache = make(map[BranchName]bool) From 02bdc9ec8dbee3138219aeac25187bac72c7172b Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 8 May 2025 13:04:06 +0200 Subject: [PATCH 3/9] :Remove unused public function from git.go --- pkg/externalrepo/git/git.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 8d762f7a4..706e68d01 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -87,7 +87,6 @@ func formatCommitMessage(changeType string) string { type GitRepository interface { repository.Repository GetPackageRevision(ctx context.Context, ref, path string) (repository.PackageRevision, kptfilev1.GitLock, error) - UpdateDeletionProposedCache() error } //go:generate go run golang.org/x/tools/cmd/stringer -type=MainBranchStrategy -linecomment @@ -1709,7 +1708,7 @@ func (r *gitRepository) discoverPackagesInTree(commit *object.Commit, opt Discov } func (r *gitRepository) Refresh(_ context.Context) error { - return r.UpdateDeletionProposedCache() + return r.updateDeletionProposedCache() } // getPkgWorkspace returns the workspace name as parsed from the kpt annotations from the latest commit for the package. From 2218bf24311e751808cce1227d956e3a71bff2ad Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Fri, 9 May 2025 14:59:54 +0200 Subject: [PATCH 4/9] Remove deletionProposedCache --- pkg/cache/crcache/repository.go | 8 -- pkg/externalrepo/git/git.go | 112 +++++++------------- pkg/externalrepo/git/git_test.go | 153 ++++++++++++++++++++++++++- pkg/externalrepo/git/package.go | 11 +- pkg/externalrepo/git/package_tree.go | 10 +- pkg/externalrepo/git/ref.go | 5 - pkg/task/clone.go | 2 +- 7 files changed, 211 insertions(+), 90 deletions(-) diff --git a/pkg/cache/crcache/repository.go b/pkg/cache/crcache/repository.go index d54ad6e8a..284bbfd5b 100644 --- a/pkg/cache/crcache/repository.go +++ b/pkg/cache/crcache/repository.go @@ -176,14 +176,6 @@ func (r *cachedRepository) getCachedPackages(ctx context.Context, forceRefresh b if forceRefresh { packages = nil packageRevisions = nil - - r.mutex.Lock() - err := r.repo.Refresh(ctx) - r.mutex.Unlock() - - if err != nil { - return nil, nil, err - } } if packages == nil { diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 706e68d01..ed81ff7f4 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -86,7 +86,7 @@ func formatCommitMessage(changeType string) string { type GitRepository interface { repository.Repository - GetPackageRevision(ctx context.Context, ref, path string) (repository.PackageRevision, kptfilev1.GitLock, error) + GetPackageRevisionWithoutFetch(ctx context.Context, ref, path string) (repository.PackageRevision, kptfilev1.GitLock, error) } //go:generate go run golang.org/x/tools/cmd/stringer -type=MainBranchStrategy -linecomment @@ -249,11 +249,6 @@ type gitRepository struct { // a git repository. credential repository.Credential - // deletionProposedCache contains the deletionProposed branches that - // exist in the repo so that we can easily check them without iterating - // through all the refs each time - deletionProposedCache map[BranchName]bool - mutex sync.Mutex // caBundle to use for TLS communication towards git @@ -375,6 +370,10 @@ func (r *gitRepository) listPackageRevisions(ctx context.Context, filter reposit mainBranch := r.branch.RefInLocal() // Looking for the registered branch + if err != nil { + return nil, err + } + for ref, err := refs.Next(); err == nil; ref, err = refs.Next() { switch name := ref.Name(); { case name == mainBranch: @@ -510,7 +509,10 @@ func (r *gitRepository) UpdatePackageRevision(ctx context.Context, old repositor // Fetch lifecycle directly from the repository rather than from the gitPackageRevision. This makes // sure we don't end up requesting the same lock twice. - lifecycle := r.getLifecycle(oldGitPackage) + lifecycle, err := r.getLifecycle(oldGitPackage) + if err != nil { + return nil, err + } return &gitPackageRevisionDraft{ prKey: oldGitPackage.prKey, @@ -628,15 +630,12 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor return fmt.Errorf("failed to update git references: %w", err) } - // Remove the deletionProposed branch from the cache - delete(r.deletionProposedCache, deletionProposedBranch) - return nil }, ) } -func (r *gitRepository) GetPackageRevision(ctx context.Context, version, path string) (repository.PackageRevision, kptfilev1.GitLock, error) { +func (r *gitRepository) GetPackageRevisionWithoutFetch(ctx context.Context, version, path string) (repository.PackageRevision, kptfilev1.GitLock, error) { ctx, span := tracer.Start(ctx, "gitRepository::GetPackageRevision", trace.WithAttributes()) defer span.End() r.mutex.Lock() @@ -826,37 +825,6 @@ func (r *gitRepository) loadDraft(ctx context.Context, ref *plumbing.Reference) return packageRevision, nil } -func (r *gitRepository) updateDeletionProposedCache() error { - r.deletionProposedCache = make(map[BranchName]bool) - - err := r.fetchRemoteRepository(context.Background()) - if err != nil { - return err - } - refs, err := r.repo.References() - if err != nil { - return err - } - - for { - ref, err := refs.Next() - if err == io.EOF { - break - } - if err != nil { - klog.Errorf("error getting next ref: %v", err) - break - } - - branch, isDeletionProposedBranch := getdeletionProposedBranchNameInLocal(ref.Name()) - if isDeletionProposedBranch { - r.deletionProposedCache[deletionProposedPrefix+branch] = true - } - } - - return nil -} - func parseDraftName(draft *plumbing.Reference) (pkgPathAndName, workspaceName string, err error) { refName := draft.Name() var suffix string @@ -1333,7 +1301,7 @@ func (r *gitRepository) findLatestPackageCommit(startCommit *object.Commit, key // commitCallback is the function type that needs to be provided to the history iterator functions. type commitCallback func(*object.Commit) error -func (r *gitRepository) GetLifecycle(ctx context.Context, pkgRev *gitPackageRevision) v1alpha1.PackageRevisionLifecycle { +func (r *gitRepository) GetLifecycle(ctx context.Context, pkgRev *gitPackageRevision) (v1alpha1.PackageRevisionLifecycle, error) { _, span := tracer.Start(ctx, "gitRepository::GetLifecycle", trace.WithAttributes()) defer span.End() r.mutex.Lock() @@ -1342,33 +1310,32 @@ func (r *gitRepository) GetLifecycle(ctx context.Context, pkgRev *gitPackageRevi return r.getLifecycle(pkgRev) } -func (r *gitRepository) getLifecycle(pkgRev *gitPackageRevision) v1alpha1.PackageRevisionLifecycle { +// As this operation is checking a file on the repo, a wrapping function needs to acquire the repository lock. +func (r *gitRepository) getLifecycle(pkgRev *gitPackageRevision) (v1alpha1.PackageRevisionLifecycle, error) { switch ref := pkgRev.ref; { case ref == nil: return r.checkPublishedLifecycle(pkgRev) case isDraftBranchNameInLocal(ref.Name()): - return v1alpha1.PackageRevisionLifecycleDraft + return v1alpha1.PackageRevisionLifecycleDraft, nil case isProposedBranchNameInLocal(ref.Name()): - return v1alpha1.PackageRevisionLifecycleProposed + return v1alpha1.PackageRevisionLifecycleProposed, nil default: return r.checkPublishedLifecycle(pkgRev) } } -func (r *gitRepository) checkPublishedLifecycle(pkgRev *gitPackageRevision) v1alpha1.PackageRevisionLifecycle { - if r.deletionProposedCache == nil { - if err := r.updateDeletionProposedCache(); err != nil { - klog.Errorf("failed to update deletionProposed cache: %v", err) - return v1alpha1.PackageRevisionLifecyclePublished - } - } - +// CheckPublishedLifecycle checks whether the request had a published or a deletion proposed package. +// As this operation is checking a file on the repo, a wrapping function needs to acquire the repository lock. +func (r *gitRepository) checkPublishedLifecycle(pkgRev *gitPackageRevision) (v1alpha1.PackageRevisionLifecycle, error) { branchName := createDeletionProposedName(pkgRev.Key()) - if _, found := r.deletionProposedCache[branchName]; found { - return v1alpha1.PackageRevisionLifecycleDeletionProposed + // As per the writing of this one, the only other error that can happen here is File IO errors. + _, err := r.repo.Branch(branchName.RefInLocal().String()) + if err == git.ErrBranchNotFound { + return v1alpha1.PackageRevisionLifecyclePublished, nil + } else if err != nil { + return "", err } - - return v1alpha1.PackageRevisionLifecyclePublished + return v1alpha1.PackageRevisionLifecycleDeletionProposed, nil } func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageRevision, newLifecycle v1alpha1.PackageRevisionLifecycle) error { @@ -1378,7 +1345,10 @@ func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageR r.mutex.Lock() defer r.mutex.Unlock() - old := r.getLifecycle(pkgRev) + old, err := r.getLifecycle(pkgRev) + if err != nil { + return err + } if !v1alpha1.LifecycleIsPublished(old) { return fmt.Errorf("cannot update lifecycle for draft package revision") } @@ -1390,7 +1360,6 @@ func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageR return fmt.Errorf("invalid new lifecycle value: %q", newLifecycle) } // Push the package revision into a deletionProposed branch. - r.deletionProposedCache[deletionProposedBranch] = true refSpecs.AddRefToPush(pkgRev.commit, deletionProposedBranch.RefInLocal()) } if old == v1alpha1.PackageRevisionLifecycleDeletionProposed { @@ -1398,8 +1367,6 @@ func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageR return fmt.Errorf("invalid new lifecycle value: %q", newLifecycle) } - // Delete the deletionProposed branch - delete(r.deletionProposedCache, deletionProposedBranch) ref := plumbing.NewHashReference(deletionProposedBranch.RefInLocal(), pkgRev.commit) refSpecs.AddRefToDelete(ref) } @@ -1579,13 +1546,14 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos } return &gitPackageRevision{ - prKey: d.prKey, - repo: d.repo, - updated: d.updated, - ref: newRef, - tree: d.tree, - commit: newRef.Hash(), - tasks: d.tasks, + prKey: d.prKey, + repo: d.repo, + updated: d.updated, + ref: newRef, + tree: d.tree, + commit: newRef.Hash(), + tasks: d.tasks, + lifecycle: d.lifecycle, }, nil } @@ -1707,8 +1675,10 @@ func (r *gitRepository) discoverPackagesInTree(commit *object.Commit, opt Discov return t, nil } -func (r *gitRepository) Refresh(_ context.Context) error { - return r.updateDeletionProposedCache() +// Deprecated: use ListPackageRevisions instead +func (r *gitRepository) Refresh(ctx context.Context) error { + _, err := r.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + return err } // getPkgWorkspace returns the workspace name as parsed from the kpt annotations from the latest commit for the package. diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index 1127c26de..13ccb908b 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -259,7 +259,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { version := "v1" path := "test-package" - packageRevision, gitLock, err := repo.GetPackageRevision(ctx, version, path) + packageRevision, gitLock, err := repo.GetPackageRevisionWithoutFetch(ctx, version, path) if err != nil { t.Fatalf("GetPackageRevision(%q, %q) failed: %v", version, path, err) } @@ -1534,6 +1534,48 @@ func createAndPublishPR(ctx context.Context, repo repository.Repository, pr *v1a return prr, nil } +func createAndPublishPRWithResources(ctx context.Context, repo repository.Repository, pr *v1alpha1.PackageRevision, prr *v1alpha1.PackageRevisionResources) (repository.PackageRevision, error) { + draft, err := repo.CreatePackageRevisionDraft(ctx, pr) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to create package revision draft") + } + + err = draft.UpdateResources(ctx, prr, nil) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to update draft resources") + } + + prdraftv1, err := repo.ClosePackageRevisionDraft(ctx, draft, 0) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to close draft packageRevision") + } + + draft2, err := repo.UpdatePackageRevision(ctx, prdraftv1) + + if err := draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleProposed); err != nil { + return nil, pkgerrors.Wrap(err, "Failed to update draft lifecycle") + } + + prdraftv2, err := repo.ClosePackageRevisionDraft(ctx, draft2, 0) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to finalize draft") + } + + draft3, err := repo.UpdatePackageRevision(ctx, prdraftv2) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to open draft packageRevision") + } + + err = draft3.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to update draft lifecycle") + } + + prv, err := repo.ClosePackageRevisionDraft(ctx, draft3, 1) + + return prv, err +} + func findCommitWithAuthorAndEmail(t *testing.T, repo *gogit.Repository, author, email string) error { t.Logf("Looking for commit with author %q and email %q", author, email) log, err := repo.Log(&gogit.LogOptions{Order: gogit.LogOrderCommitterTime}) @@ -2600,3 +2642,112 @@ func TestFormatCommitMessage(t *testing.T) { }) } } + +func TestDeletionProposedPackageIsRediscovered(t *testing.T) { + const ( + repoName = "deletion-proposed-on-rediscovered-repo" + namespace = "default" + packageName = "deletion-proposed-on-rediscovered-pkg" + workspace = "deletion-proposed-on-rediscovered-ws" + ) + + tempdir := t.TempDir() + tarfile := filepath.Join("testdata", "trivial-repository.tar") + remotepath := filepath.Join(tempdir, "remote") + localpath := filepath.Join(tempdir, "local") + _, address := ServeGitRepository(t, tarfile, remotepath) + + repoSpec := &configapi.GitRepository{ + Repo: address, + } + + ctx := context.Background() + + localRepo, err := OpenRepository(ctx, repoName, namespace, repoSpec, false, localpath, GitRepositoryOptions{}) + if err != nil { + t.Fatalf("Failed to open Git repository loaded from %q: %v", remotepath, err) + } + + pr1 := &v1alpha1.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: repoName, + }, + Spec: v1alpha1.PackageRevisionSpec{ + PackageName: packageName, + WorkspaceName: workspace, + RepositoryName: repoName, + Tasks: []v1alpha1.Task{ + { + Type: v1alpha1.TaskTypeInit, + Init: &v1alpha1.PackageInitTaskSpec{ + Description: "Empty Package", + }, + }, + }, + }, + } + + resources := &v1alpha1.PackageRevisionResources{ + Spec: v1alpha1.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": Kptfile, + }, + }, + } + + t.Logf("Creating initial Published PackageRevision %s", pr1.Spec.PackageName) + + _, err = createAndPublishPRWithResources(ctx, localRepo, pr1, resources) + if err != nil { + t.Fatalf("Failed to create and publish package revision: %v", err) + } + + prs, err := localRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{Lifecycle: v1alpha1.PackageRevisionLifecyclePublished}) + if err != nil { + t.Fatalf("Failed to list package revisions: %v", err) + } + + if len(prs) != 2 { + t.Fatalf("Expected 2 published package revisions, got %d", len(prs)) + } + + prv2draft, err := localRepo.UpdatePackageRevision(ctx, prs[0]) + if err != nil { + t.Fatalf("Failed to create draft package revision: %v", err) + } + + err = prv2draft.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDeletionProposed) + if err != nil { + t.Fatalf("Failed to update lifecycle: %v", err) + } + + _, err = localRepo.ClosePackageRevisionDraft(ctx, prv2draft, 0) + if err != nil { + t.Fatalf("Failed to finalize draft: %v", err) + } + + t.Logf("Clean up local repo") + + localRepo.Close() + + t.Logf("Opening repository for the second time") + + localRepov2, err := OpenRepository(ctx, repoName, namespace, repoSpec, false, localpath, GitRepositoryOptions{}) + if err != nil { + t.Fatalf("Failed to open Git repository loaded from %q: %v", remotepath, err) + } + + prs2, err := localRepov2.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + if err != nil { + t.Fatalf("Failed to list PackageRevisions: %v", err) + } + + if len(prs2) != 2 { + t.Fatalf("Expected 2 PackageRevisions, got %d", len(prs)) + } + + if prs[0].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecycleDeletionProposed { + t.Fatalf("Expected PackageRevision to be in deletion proposed state, got %s", prs[0].Lifecycle(ctx)) + } + +} diff --git a/pkg/externalrepo/git/package.go b/pkg/externalrepo/git/package.go index a095020c9..ef8975495 100644 --- a/pkg/externalrepo/git/package.go +++ b/pkg/externalrepo/git/package.go @@ -43,6 +43,7 @@ type gitPackageRevision struct { tasks []v1alpha1.Task metadata metav1.ObjectMeta mutex sync.Mutex + lifecycle v1alpha1.PackageRevisionLifecycle } var _ repository.PackageRevision = &gitPackageRevision{} @@ -189,6 +190,7 @@ func (p *gitPackageRevision) ToMainPackageRevision(context.Context) repository.P tree: p.tree, commit: p.commit, tasks: p.tasks, + lifecycle: p.lifecycle, } mainPr.prKey.Revision = -1 mainPr.prKey.WorkspaceName = mainPr.Key().RKey().PlaceholderWSname @@ -263,14 +265,19 @@ func (p *gitPackageRevision) GetLock() (kptfile.Upstream, kptfile.UpstreamLock, } func (p *gitPackageRevision) Lifecycle(ctx context.Context) v1alpha1.PackageRevisionLifecycle { - return p.repo.GetLifecycle(ctx, p) + return p.lifecycle } func (p *gitPackageRevision) UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error { ctx, span := tracer.Start(ctx, "gitPackageRevision::UpdateLifecycle", trace.WithAttributes()) defer span.End() + err := p.repo.UpdateLifecycle(ctx, p, new) + if err != nil { + return err + } + p.lifecycle = new - return p.repo.UpdateLifecycle(ctx, p, new) + return nil } func (p *gitPackageRevision) GetMeta() metav1.ObjectMeta { diff --git a/pkg/externalrepo/git/package_tree.go b/pkg/externalrepo/git/package_tree.go index ae0e06a42..dc8723541 100644 --- a/pkg/externalrepo/git/package_tree.go +++ b/pkg/externalrepo/git/package_tree.go @@ -119,7 +119,7 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision klog.Infof("Loaded tasks for package %s: %s", gitPrKey, marshalledTasks) } - return &gitPackageRevision{ + pr := &gitPackageRevision{ prKey: gitPrKey, repo: repo, updated: updated, @@ -128,7 +128,13 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision tree: p.treeHash, commit: p.parent.commit.Hash, tasks: tasks, - }, nil + } + + pr.lifecycle, err = p.parent.parent.getLifecycle(pr) + if err != nil { + return nil, err + } + return pr, nil } // DiscoveryPackagesOptions holds the configuration for walking a git tree diff --git a/pkg/externalrepo/git/ref.go b/pkg/externalrepo/git/ref.go index 1d94813a4..fe2d6949d 100644 --- a/pkg/externalrepo/git/ref.go +++ b/pkg/externalrepo/git/ref.go @@ -100,11 +100,6 @@ func getDraftBranchNameInLocal(n plumbing.ReferenceName) (BranchName, bool) { return BranchName(b), ok } -func getdeletionProposedBranchNameInLocal(n plumbing.ReferenceName) (BranchName, bool) { - b, ok := trimOptionalPrefix(n.String(), deletionProposedPrefixInLocalRepo) - return BranchName(b), ok -} - func isBranchInLocalRepo(n plumbing.ReferenceName) bool { return strings.HasPrefix(n.String(), branchPrefixInLocalRepo) } diff --git a/pkg/task/clone.go b/pkg/task/clone.go index f34e97aa4..c8223b193 100644 --- a/pkg/task/clone.go +++ b/pkg/task/clone.go @@ -165,7 +165,7 @@ func (m *clonePackageMutation) cloneFromGit(ctx context.Context, gitPackage *api return repository.PackageResources{}, pkgerrors.Wrap(err, "cannot clone Git repository") } - revision, lock, err := r.GetPackageRevision(ctx, gitPackage.Ref, gitPackage.Directory) + revision, lock, err := r.GetPackageRevisionWithoutFetch(ctx, gitPackage.Ref, gitPackage.Directory) if err != nil { return repository.PackageResources{}, pkgerrors.Wrapf(err, "cannot find package %s@%s:", gitPackage.Directory, gitPackage.Ref) } From d9a6274ece8e5fa3c76a16a7fa6b7fbea6efb898 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Fri, 9 May 2025 15:37:53 +0200 Subject: [PATCH 5/9] Fix sonarlint issue --- pkg/externalrepo/git/git_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index 13ccb908b..f94796eea 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -2728,7 +2728,10 @@ func TestDeletionProposedPackageIsRediscovered(t *testing.T) { t.Logf("Clean up local repo") - localRepo.Close() + err = localRepo.Close() + if err != nil { + t.Fatalf("Failed to close local repo: %v", err) + } t.Logf("Opening repository for the second time") From 586d0b61114413d529d249e2c2739a390bbe144b Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 26 Jun 2025 15:36:41 +0200 Subject: [PATCH 6/9] :Changing git version production replacing way to compute deletion proposed packages --- pkg/externalrepo/git/git.go | 80 ++++++++++++++++------------ pkg/externalrepo/git/git_test.go | 34 ++++++------ pkg/externalrepo/git/package_tree.go | 2 +- pkg/externalrepo/git/push.go | 16 ++++++ pkg/externalrepo/git/ref.go | 5 +- 5 files changed, 84 insertions(+), 53 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index ed81ff7f4..d34b49d38 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -15,14 +15,13 @@ package git import ( - "bytes" "context" - "crypto/sha256" - "encoding/hex" "fmt" "io" + "math/rand/v2" "os" "path/filepath" + "strconv" "strings" "sync" "time" @@ -95,9 +94,12 @@ type MainBranchStrategy int const ( ErrorIfMissing MainBranchStrategy = iota // ErrorIsMissing CreateIfMissing // CreateIfMissing - SkipVerification // SkipVerification + SkipVerification // SkipVerification\ ) +// Random number generator for initial repository version +var initialRepoVersionRandom = rand.New(rand.NewPCG(uint64(time.Now().UnixNano()), uint64(time.Now().UnixNano()))) + type GitRepositoryOptions struct { externalrepotypes.ExternalRepoOptions MainBranchStrategy MainBranchStrategy @@ -175,6 +177,7 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi if err := util.ValidateDirectoryName(string(branch), false); err != nil { return nil, fmt.Errorf("branch name %s invalid: %v", branch, err) } + repository := &gitRepository{ key: repository.RepositoryKey{ @@ -190,6 +193,14 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi userInfoProvider: makeUserInfoProvider(spec, opts.ExternalRepoOptions.UserInfoProvider), cacheDir: dir, deployment: deployment, + // A random high number is generated as the initial repository version. + // In case the user of this library (the cache for example) wants to remember the + // repository version between re-opens, there is very little chance + /// 2^32 > 1 billion possible initial repository versions can happen, + // so collisions are very unlikely. + // Also even if incrementing from 2^32 to 2^64 before the uint64 every second, + // it would take about 500 billion years to get uint64 to overlflow. + version: initialRepoVersionRandom.Uint64N(1 << 32), } if opts.ExternalRepoOptions.UseUserDefinedCaBundle { @@ -199,7 +210,8 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi repository.caBundle = []byte(caBundle.ToString()) } } - + repository.mutex.Lock() + defer repository.mutex.Unlock() if err := repository.fetchRemoteRepository(ctx); err != nil { return nil, err } @@ -238,6 +250,15 @@ type gitRepository struct { credentialResolver repository.CredentialResolver userInfoProvider repository.UserInfoProvider + // Version is an always incrementing integer, + // but it's lifecycle is tied to porch's gitRepository object. + // It can be used to detect if there were changes to the local gitrepository done + // by other processes in porch. + // For all external clients of this library, it should be an opaque string, + // that can be checked for equality, but should not be compared to determine order. + // It's behavior is similar to the .metadata.resourceVersion field in k8s resources. + version uint64 + // Folder used for the local git cache. cacheDir string @@ -283,27 +304,7 @@ func (r *gitRepository) Version(ctx context.Context) (string, error) { r.mutex.Lock() defer r.mutex.Unlock() - if err := r.fetchRemoteRepository(ctx); err != nil { - return "", err - } - - refs, err := r.repo.References() - if err != nil { - return "", err - } - - b := bytes.Buffer{} - for { - ref, err := refs.Next() - if err == io.EOF { - break - } - - b.WriteString(ref.String()) - } - - hash := sha256.Sum256(b.Bytes()) - return hex.EncodeToString(hash[:]), nil + return strconv.FormatUint(r.version, 10), nil } func (r *gitRepository) ListPackages(ctx context.Context, filter repository.ListPackageFilter) ([]repository.Package, error) { @@ -559,7 +560,7 @@ func (r *gitRepository) DeletePackageRevision(ctx context.Context, old repositor } } refSpecs := newPushRefSpecBuilder() - deletionProposedBranch := createDeletionProposedName(oldGit.Key()) + deletionProposedBranch := createDeletionProposedName(oldGit.Key(), r.branch) // We can only delete packages which have their own ref. Refs that are shared with other packages // (main branch, tag that doesn't contain package path in its name, ...) cannot be deleted. @@ -985,6 +986,7 @@ func (r *gitRepository) fetchRemoteRepository(ctx context.Context) error { default: return fmt.Errorf("cannot fetch repository %s/%s: %w", r.Key().Namespace, r.Key().Name, err) } + r.updateRepositoryVersion() return nil } @@ -1111,6 +1113,8 @@ func (r *gitRepository) createPackageDeleteCommit(ctx context.Context, branch pl return commitHash, nil } +// Pushes the needed refspecs to the remote. +// Mutex must be held by caller func (r *gitRepository) pushAndCleanup(ctx context.Context, ph *pushRefSpecBuilder) error { ctx, span := tracer.Start(ctx, "gitRepository::pushAndCleanup", trace.WithAttributes()) defer span.End() @@ -1140,6 +1144,7 @@ func (r *gitRepository) pushAndCleanup(ctx context.Context, ph *pushRefSpecBuild } return err } + r.updateRepositoryVersion() return nil } @@ -1323,17 +1328,19 @@ func (r *gitRepository) getLifecycle(pkgRev *gitPackageRevision) (v1alpha1.Packa return r.checkPublishedLifecycle(pkgRev) } } - + // CheckPublishedLifecycle checks whether the request had a published or a deletion proposed package. // As this operation is checking a file on the repo, a wrapping function needs to acquire the repository lock. func (r *gitRepository) checkPublishedLifecycle(pkgRev *gitPackageRevision) (v1alpha1.PackageRevisionLifecycle, error) { - branchName := createDeletionProposedName(pkgRev.Key()) - // As per the writing of this one, the only other error that can happen here is File IO errors. - _, err := r.repo.Branch(branchName.RefInLocal().String()) - if err == git.ErrBranchNotFound { + + branchNameInLocal := createDeletionProposedName(pkgRev.Key(), r.branch).RefInLocal() + klog.Infof("looking for branch %q", branchNameInLocal) + _, err := r.repo.Reference(branchNameInLocal, true) + if err == plumbing.ErrReferenceNotFound { return v1alpha1.PackageRevisionLifecyclePublished, nil } else if err != nil { - return "", err + // As per the writing of this, the only other error that can happen here are File IO errors. + return "", fmt.Errorf("error checking whether package %q is published or deletion proposed: %w", pkgRev.Key().String(), err) } return v1alpha1.PackageRevisionLifecycleDeletionProposed, nil } @@ -1353,7 +1360,7 @@ func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageR return fmt.Errorf("cannot update lifecycle for draft package revision") } refSpecs := newPushRefSpecBuilder() - deletionProposedBranch := createDeletionProposedName(pkgRev.Key()) + deletionProposedBranch := createDeletionProposedName(pkgRev.Key(), r.branch) if old == v1alpha1.PackageRevisionLifecyclePublished { if newLifecycle != v1alpha1.PackageRevisionLifecycleDeletionProposed { @@ -1675,6 +1682,11 @@ func (r *gitRepository) discoverPackagesInTree(commit *object.Commit, opt Discov return t, nil } +// updates the repository version. Mutex must be held by caller +func (r *gitRepository) updateRepositoryVersion() { + r.version = r.version + 1 +} + // Deprecated: use ListPackageRevisions instead func (r *gitRepository) Refresh(ctx context.Context) error { _, err := r.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index f94796eea..785d5c032 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -2697,7 +2697,7 @@ func TestDeletionProposedPackageIsRediscovered(t *testing.T) { t.Logf("Creating initial Published PackageRevision %s", pr1.Spec.PackageName) - _, err = createAndPublishPRWithResources(ctx, localRepo, pr1, resources) + pr, err := createAndPublishPRWithResources(ctx, localRepo, pr1, resources) if err != nil { t.Fatalf("Failed to create and publish package revision: %v", err) } @@ -2707,28 +2707,20 @@ func TestDeletionProposedPackageIsRediscovered(t *testing.T) { t.Fatalf("Failed to list package revisions: %v", err) } + t.Logf("Ensuring that the both v1 and main packageRevisions exist.") + if len(prs) != 2 { t.Fatalf("Expected 2 published package revisions, got %d", len(prs)) } - prv2draft, err := localRepo.UpdatePackageRevision(ctx, prs[0]) - if err != nil { - t.Fatalf("Failed to create draft package revision: %v", err) - } - - err = prv2draft.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDeletionProposed) + err = pr.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDeletionProposed) if err != nil { t.Fatalf("Failed to update lifecycle: %v", err) } - _, err = localRepo.ClosePackageRevisionDraft(ctx, prv2draft, 0) - if err != nil { - t.Fatalf("Failed to finalize draft: %v", err) - } - t.Logf("Clean up local repo") - err = localRepo.Close() + err = localRepo.Close(ctx) if err != nil { t.Fatalf("Failed to close local repo: %v", err) } @@ -2746,11 +2738,19 @@ func TestDeletionProposedPackageIsRediscovered(t *testing.T) { } if len(prs2) != 2 { - t.Fatalf("Expected 2 PackageRevisions, got %d", len(prs)) + t.Fatalf("Expected 2 PackageRevisions, got %d, list: %v", len(prs2), prs2) } - - if prs[0].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecycleDeletionProposed { - t.Fatalf("Expected PackageRevision to be in deletion proposed state, got %s", prs[0].Lifecycle(ctx)) + + for prToBeVerified := range prs2 { + if prs2[prToBeVerified].Key().Revision == 1 { + if prs2[prToBeVerified].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecycleDeletionProposed { + t.Fatalf("Expected PackageRevision to be in deletion proposed state, got %s", prs[0].Lifecycle(ctx)) + } + } else { + if prs2[prToBeVerified].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecyclePublished { + t.Fatalf("Expected PackageRevision to be in published state, got %s", prs[prToBeVerified].Lifecycle(ctx)) + } + } } } diff --git a/pkg/externalrepo/git/package_tree.go b/pkg/externalrepo/git/package_tree.go index dc8723541..7a7914f96 100644 --- a/pkg/externalrepo/git/package_tree.go +++ b/pkg/externalrepo/git/package_tree.go @@ -129,7 +129,7 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision commit: p.parent.commit.Hash, tasks: tasks, } - + //TODO:This is resolving a circular dependency with looking up the repository. Maybe there is a better way? pr.lifecycle, err = p.parent.parent.getLifecycle(pr) if err != nil { return nil, err diff --git a/pkg/externalrepo/git/push.go b/pkg/externalrepo/git/push.go index 4414c7e0e..890a0fc0c 100644 --- a/pkg/externalrepo/git/push.go +++ b/pkg/externalrepo/git/push.go @@ -24,6 +24,7 @@ import ( type pushRefSpecBuilder struct { pushRefs map[plumbing.ReferenceName]plumbing.Hash require map[plumbing.ReferenceName]plumbing.Hash + refForLease *plumbing.Reference } func newPushRefSpecBuilder() *pushRefSpecBuilder { @@ -42,12 +43,27 @@ func (b *pushRefSpecBuilder) AddRefToDelete(ref *plumbing.Reference) { b.RequireRef(ref) } +// Requries a reference to be existent with the correct during the push. +// The ref is evaluated on the client side only during a send-pack session in go-git, and doesn't provide a guarantee +// that the reference will be locked on the server side during the push. +// Reference cannot be pointing to plumbing.ZeroHash. func (b *pushRefSpecBuilder) RequireRef(ref *plumbing.Reference) { if ref != nil { b.require[ref.Name()] = ref.Hash() } } +func (b *pushRefSpecBuilder) RequireRefDoesntExistWithLease(ref *plumbing.Reference) error { + if ref.Hash() != plumbing.ZeroHash { + return fmt.Errorf("Reference %s refers to a non-zero hash: %s", ref.Name().String(), ref.Hash().String()) + } + if b.refForLease != nil { + return fmt.Errorf("Only a single reference can be locked during a push: %s", b.refForLease.Name().String()) + } + b.refForLease = ref + return nil +} + func (b *pushRefSpecBuilder) BuildRefSpecs() (push []config.RefSpec, require []config.RefSpec, err error) { for local, hash := range b.pushRefs { remote, err := refInRemoteFromRefInLocal(local) diff --git a/pkg/externalrepo/git/ref.go b/pkg/externalrepo/git/ref.go index fe2d6949d..113dfec50 100644 --- a/pkg/externalrepo/git/ref.go +++ b/pkg/externalrepo/git/ref.go @@ -124,7 +124,10 @@ func createProposedName(key repository.PackageRevisionKey) BranchName { return BranchName(proposedPrefix + filepath.Join(key.PkgKey.ToFullPathname(), string(key.WorkspaceName))) } -func createDeletionProposedName(key repository.PackageRevisionKey) BranchName { +func createDeletionProposedName(key repository.PackageRevisionKey, mainBranch BranchName) BranchName { + if key.Revision == -1 { + return BranchName(deletionProposedPrefix + filepath.Join(key.PkgKey.ToFullPathname(), "/"+ string(mainBranch))) + } return BranchName(deletionProposedPrefix + filepath.Join(key.PkgKey.ToFullPathname(), "/v"+repository.Revision2Str(key.Revision))) } From f77f5dabb32a978e8caf241342da1b1c323c9a44 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 6 Jul 2025 07:14:06 +0200 Subject: [PATCH 7/9] Fix sonarlint --- pkg/externalrepo/git/git.go | 11 +++-------- pkg/externalrepo/git/git_test.go | 4 ++-- pkg/externalrepo/git/push.go | 12 ------------ 3 files changed, 5 insertions(+), 22 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index d34b49d38..548301952 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -177,7 +177,6 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi if err := util.ValidateDirectoryName(string(branch), false); err != nil { return nil, fmt.Errorf("branch name %s invalid: %v", branch, err) } - repository := &gitRepository{ key: repository.RepositoryKey{ @@ -195,7 +194,7 @@ func OpenRepository(ctx context.Context, name, namespace string, spec *configapi deployment: deployment, // A random high number is generated as the initial repository version. // In case the user of this library (the cache for example) wants to remember the - // repository version between re-opens, there is very little chance + // repository version between re-opens, there is very little chance /// 2^32 > 1 billion possible initial repository versions can happen, // so collisions are very unlikely. // Also even if incrementing from 2^32 to 2^64 before the uint64 every second, @@ -371,10 +370,6 @@ func (r *gitRepository) listPackageRevisions(ctx context.Context, filter reposit mainBranch := r.branch.RefInLocal() // Looking for the registered branch - if err != nil { - return nil, err - } - for ref, err := refs.Next(); err == nil; ref, err = refs.Next() { switch name := ref.Name(); { case name == mainBranch: @@ -1113,7 +1108,7 @@ func (r *gitRepository) createPackageDeleteCommit(ctx context.Context, branch pl return commitHash, nil } -// Pushes the needed refspecs to the remote. +// Pushes the needed refspecs to the remote. // Mutex must be held by caller func (r *gitRepository) pushAndCleanup(ctx context.Context, ph *pushRefSpecBuilder) error { ctx, span := tracer.Start(ctx, "gitRepository::pushAndCleanup", trace.WithAttributes()) @@ -1328,7 +1323,7 @@ func (r *gitRepository) getLifecycle(pkgRev *gitPackageRevision) (v1alpha1.Packa return r.checkPublishedLifecycle(pkgRev) } } - + // CheckPublishedLifecycle checks whether the request had a published or a deletion proposed package. // As this operation is checking a file on the repo, a wrapping function needs to acquire the repository lock. func (r *gitRepository) checkPublishedLifecycle(pkgRev *gitPackageRevision) (v1alpha1.PackageRevisionLifecycle, error) { diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index 785d5c032..c60470ad5 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -2702,7 +2702,7 @@ func TestDeletionProposedPackageIsRediscovered(t *testing.T) { t.Fatalf("Failed to create and publish package revision: %v", err) } - prs, err := localRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{Lifecycle: v1alpha1.PackageRevisionLifecyclePublished}) + prs, err := localRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{Lifecycles: []v1alpha1.PackageRevisionLifecycle{v1alpha1.PackageRevisionLifecyclePublished}}) if err != nil { t.Fatalf("Failed to list package revisions: %v", err) } @@ -2740,7 +2740,7 @@ func TestDeletionProposedPackageIsRediscovered(t *testing.T) { if len(prs2) != 2 { t.Fatalf("Expected 2 PackageRevisions, got %d, list: %v", len(prs2), prs2) } - + for prToBeVerified := range prs2 { if prs2[prToBeVerified].Key().Revision == 1 { if prs2[prToBeVerified].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecycleDeletionProposed { diff --git a/pkg/externalrepo/git/push.go b/pkg/externalrepo/git/push.go index 890a0fc0c..500c759af 100644 --- a/pkg/externalrepo/git/push.go +++ b/pkg/externalrepo/git/push.go @@ -24,7 +24,6 @@ import ( type pushRefSpecBuilder struct { pushRefs map[plumbing.ReferenceName]plumbing.Hash require map[plumbing.ReferenceName]plumbing.Hash - refForLease *plumbing.Reference } func newPushRefSpecBuilder() *pushRefSpecBuilder { @@ -53,17 +52,6 @@ func (b *pushRefSpecBuilder) RequireRef(ref *plumbing.Reference) { } } -func (b *pushRefSpecBuilder) RequireRefDoesntExistWithLease(ref *plumbing.Reference) error { - if ref.Hash() != plumbing.ZeroHash { - return fmt.Errorf("Reference %s refers to a non-zero hash: %s", ref.Name().String(), ref.Hash().String()) - } - if b.refForLease != nil { - return fmt.Errorf("Only a single reference can be locked during a push: %s", b.refForLease.Name().String()) - } - b.refForLease = ref - return nil -} - func (b *pushRefSpecBuilder) BuildRefSpecs() (push []config.RefSpec, require []config.RefSpec, err error) { for local, hash := range b.pushRefs { remote, err := refInRemoteFromRefInLocal(local) From 96d159512b56367dad792a41a67a85054a2063f4 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 6 Jul 2025 07:21:53 +0200 Subject: [PATCH 8/9] Fix lint errchecks --- pkg/externalrepo/git/git.go | 2 +- pkg/externalrepo/git/git_test.go | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 548301952..5785fe0da 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -298,7 +298,7 @@ func (r *gitRepository) Close(context.Context) error { } func (r *gitRepository) Version(ctx context.Context) (string, error) { - ctx, span := tracer.Start(ctx, "gitRepository::Version", trace.WithAttributes()) + _, span := tracer.Start(ctx, "gitRepository::Version", trace.WithAttributes()) defer span.End() r.mutex.Lock() defer r.mutex.Unlock() diff --git a/pkg/externalrepo/git/git_test.go b/pkg/externalrepo/git/git_test.go index c60470ad5..8a1938396 100644 --- a/pkg/externalrepo/git/git_test.go +++ b/pkg/externalrepo/git/git_test.go @@ -1551,6 +1551,9 @@ func createAndPublishPRWithResources(ctx context.Context, repo repository.Reposi } draft2, err := repo.UpdatePackageRevision(ctx, prdraftv1) + if err != nil { + return nil, pkgerrors.Wrap(err, "Failed to open draft packageRevision") + } if err := draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleProposed); err != nil { return nil, pkgerrors.Wrap(err, "Failed to update draft lifecycle") @@ -2021,7 +2024,10 @@ func TestApproveOnManuallyMovedProposed(t *testing.T) { t.Fatalf("Failed to create draft PackageRevision %v", err) } - draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleProposed) + err = draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleProposed) + if err != nil { + t.Fatalf("Failed to update lifecycle: %v", err) + } uip := makeUserInfoProvider(repoSpec, &mockK8sUsp{}) @@ -2138,7 +2144,10 @@ func TestApproveOnManuallyMovedDraft(t *testing.T) { t.Fatalf("Failed to create draft PackageRevision %v", err) } - draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDraft) + err = draft2.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecycleDraft) + if err != nil { + t.Fatalf("Failed to update lifecycle: %v", err) + } uip := makeUserInfoProvider(repoSpec, &mockK8sUsp{}) From c65c54a4f680d6793980785ddd4265f91d23e4f8 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 23 Jul 2025 10:34:40 +0200 Subject: [PATCH 9/9] Fix missed merge conflict --- pkg/externalrepo/git/git.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index dda3af7f1..1a0cc5e88 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -1363,7 +1363,7 @@ func (r *gitRepository) getLifecycle(pkgRev *gitPackageRevision) (v1alpha1.Packa // As this operation is checking a file on the repo, a wrapping function needs to acquire the repository lock. func (r *gitRepository) checkPublishedLifecycle(pkgRev *gitPackageRevision) (v1alpha1.PackageRevisionLifecycle, error) { - branchNameInLocal := createDeletionProposedName(pkgRev.Key(), r.branch).RefInLocal() + branchNameInLocal := createDeletionProposedName(pkgRev.Key()).RefInLocal() klog.Infof("looking for branch %q", branchNameInLocal) _, err := r.repo.Reference(branchNameInLocal, true) if err == plumbing.ErrReferenceNotFound { @@ -1390,7 +1390,7 @@ func (r *gitRepository) UpdateLifecycle(ctx context.Context, pkgRev *gitPackageR return fmt.Errorf("cannot update lifecycle for draft package revision") } refSpecs := newPushRefSpecBuilder() - deletionProposedBranch := createDeletionProposedName(pkgRev.Key(), r.branch) + deletionProposedBranch := createDeletionProposedName(pkgRev.Key()) if old == v1alpha1.PackageRevisionLifecyclePublished { if newLifecycle != v1alpha1.PackageRevisionLifecycleDeletionProposed {