From 94c5d050717e02e60054e0a71a54c307b3930b3d Mon Sep 17 00:00:00 2001 From: liamfallon Date: Tue, 10 Dec 2024 15:34:07 +0000 Subject: [PATCH 1/3] Refactor Porch code to remove callback from PackageRevisionDraft to Repository for draft closing --- pkg/cache/memory/cache_test.go | 4 +- pkg/cache/memory/draft.go | 71 ------------------------------- pkg/cache/memory/repository.go | 53 +++++++++++++++++------ pkg/engine/engine.go | 8 ++-- pkg/git/draft.go | 8 ---- pkg/git/git.go | 7 ++- pkg/git/git_test.go | 12 +++--- pkg/oci/mutate.go | 22 +++++----- pkg/repository/fake/repository.go | 4 ++ pkg/repository/repository.go | 5 ++- 10 files changed, 77 insertions(+), 117 deletions(-) delete mode 100644 pkg/cache/memory/draft.go diff --git a/pkg/cache/memory/cache_test.go b/pkg/cache/memory/cache_test.go index ed8dd323..c2523d8d 100644 --- a/pkg/cache/memory/cache_test.go +++ b/pkg/cache/memory/cache_test.go @@ -112,7 +112,7 @@ func TestPublishedLatest(t *testing.T) { if err := update.UpdateLifecycle(ctx, api.PackageRevisionLifecyclePublished); err != nil { t.Fatalf("UpdateLifecycle failed; %v", err) } - closed, err := update.Close(ctx, "") + closed, err := cachedRepo.ClosePackageRevisionDraft(ctx, update, "") if err != nil { t.Fatalf("Close failed: %v", err) } @@ -158,7 +158,7 @@ func TestDeletePublishedMain(t *testing.T) { if err := update.UpdateLifecycle(ctx, api.PackageRevisionLifecyclePublished); err != nil { t.Fatalf("UpdateLifecycle failed; %v", err) } - closed, err := update.Close(ctx, "") + closed, err := cachedRepo.ClosePackageRevisionDraft(ctx, update, "") if err != nil { t.Fatalf("Close failed: %v", err) } diff --git a/pkg/cache/memory/draft.go b/pkg/cache/memory/draft.go deleted file mode 100644 index ba60354a..00000000 --- a/pkg/cache/memory/draft.go +++ /dev/null @@ -1,71 +0,0 @@ -// Copyright 2022, 2024 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. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package memory - -import ( - "context" - - "github.com/nephio-project/porch/api/porch/v1alpha1" - "github.com/nephio-project/porch/pkg/repository" - "go.opentelemetry.io/otel/trace" -) - -type cachedDraft struct { - repository.PackageRevisionDraft - cache *cachedRepository -} - -var _ repository.PackageRevisionDraft = &cachedDraft{} - -func (cd *cachedDraft) Close(ctx context.Context, version string) (repository.PackageRevision, error) { - ctx, span := tracer.Start(ctx, "cachedDraft::Close", trace.WithAttributes()) - defer span.End() - v, err := cd.cache.Version(ctx) - if err != nil { - return nil, err - } - - if v != cd.cache.lastVersion { - _, _, err = cd.cache.refreshAllCachedPackages(ctx) - if err != nil { - return nil, err - } - } - - revisions, err := cd.cache.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ - Package: cd.GetName(), - }) - if err != nil { - return nil, err - } - - var publishedRevisions []string - for _, rev := range revisions { - if v1alpha1.LifecycleIsPublished(rev.Lifecycle()) { - publishedRevisions = append(publishedRevisions, rev.Key().Revision) - } - } - - nextVersion, err := repository.NextRevisionNumber(publishedRevisions) - if err != nil { - return nil, err - } - - if closed, err := cd.PackageRevisionDraft.Close(ctx, nextVersion); err != nil { - return nil, err - } else { - return cd.cache.update(ctx, closed) - } -} diff --git a/pkg/cache/memory/repository.go b/pkg/cache/memory/repository.go index b50e511e..46dc2be9 100644 --- a/pkg/cache/memory/repository.go +++ b/pkg/cache/memory/repository.go @@ -168,29 +168,56 @@ func (r *cachedRepository) getCachedPackages(ctx context.Context, forceRefresh b } func (r *cachedRepository) CreatePackageRevision(ctx context.Context, obj *v1alpha1.PackageRevision) (repository.PackageRevisionDraft, error) { - created, err := r.repo.CreatePackageRevision(ctx, obj) + return r.repo.CreatePackageRevision(ctx, obj) +} + +func (r *cachedRepository) ClosePackageRevisionDraft(ctx context.Context, prd repository.PackageRevisionDraft, version string) (repository.PackageRevision, error) { + ctx, span := tracer.Start(ctx, "cachedDraft::Close", trace.WithAttributes()) + defer span.End() + + v, err := r.Version(ctx) + if err != nil { + return nil, err + } + + if v != r.lastVersion { + _, _, err = r.refreshAllCachedPackages(ctx) + if err != nil { + return nil, err + } + } + + revisions, err := r.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Package: prd.GetName(), + }) if err != nil { return nil, err } - return &cachedDraft{ - PackageRevisionDraft: created, - cache: r, - }, nil + var publishedRevisions []string + for _, rev := range revisions { + if v1alpha1.LifecycleIsPublished(rev.Lifecycle()) { + publishedRevisions = append(publishedRevisions, rev.Key().Revision) + } + } + + nextVersion, err := repository.NextRevisionNumber(publishedRevisions) + if err != nil { + return nil, err + } + + if closed, err := r.repo.ClosePackageRevisionDraft(ctx, prd, nextVersion); err != nil { + return nil, err + } else { + return r.update(ctx, closed) + } } func (r *cachedRepository) UpdatePackageRevision(ctx context.Context, old repository.PackageRevision) (repository.PackageRevisionDraft, error) { // Unwrap unwrapped := old.(*cachedPackageRevision).PackageRevision - created, err := r.repo.UpdatePackageRevision(ctx, unwrapped) - if err != nil { - return nil, err - } - return &cachedDraft{ - PackageRevisionDraft: created, - cache: r, - }, nil + return r.repo.UpdatePackageRevision(ctx, unwrapped) } func (r *cachedRepository) update(ctx context.Context, updated repository.PackageRevision) (*cachedPackageRevision, error) { diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index e67179f1..13735076 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -184,7 +184,7 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * } // Updates are done. - repoPkgRev, err := draft.Close(ctx, "") + repoPkgRev, err := repo.ClosePackageRevisionDraft(ctx, draft, "") if err != nil { return nil, err } @@ -309,7 +309,7 @@ func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version string, } // Updates are done. - repoPkgRev, err = draft.Close(ctx, version) + repoPkgRev, err = repo.ClosePackageRevisionDraft(ctx, draft, version) if err != nil { return nil, err } @@ -509,7 +509,7 @@ func (cad *cadEngine) UpdatePackageResources(ctx context.Context, repositoryObj return nil, renderStatus, err } // No lifecycle change when updating package resources; updates are done. - repoPkgRev, err := draft.Close(ctx, "") + repoPkgRev, err := repo.ClosePackageRevisionDraft(ctx, draft, "") if err != nil { return nil, renderStatus, err } @@ -562,7 +562,7 @@ func (cad *cadEngine) RecloneAndReplay(ctx context.Context, parentPR repository. return nil, err } - repoPkgRev, err := draft.Close(ctx, version) + repoPkgRev, err := repo.ClosePackageRevisionDraft(ctx, draft, version) if err != nil { return nil, err diff --git a/pkg/git/draft.go b/pkg/git/draft.go index 34048da0..2f78bf0e 100644 --- a/pkg/git/draft.go +++ b/pkg/git/draft.go @@ -63,14 +63,6 @@ func (d *gitPackageRevisionDraft) UpdateLifecycle(ctx context.Context, new v1alp return nil } -// Finish round of updates. -func (d *gitPackageRevisionDraft) Close(ctx context.Context, version string) (repository.PackageRevision, error) { - ctx, span := tracer.Start(ctx, "gitPackageDraft::Close", trace.WithAttributes()) - defer span.End() - - return d.parent.CloseDraft(ctx, version, d) -} - func (d *gitPackageRevisionDraft) GetName() string { packageDirectory := d.parent.directory packageName := strings.TrimPrefix(d.path, packageDirectory+"/") diff --git a/pkg/git/git.go b/pkg/git/git.go index 7a6e84ab..f718662c 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -1480,10 +1480,15 @@ func (r *gitRepository) UpdateDraftResources(ctx context.Context, draft *gitPack return nil } -func (r *gitRepository) CloseDraft(ctx context.Context, version string, d *gitPackageRevisionDraft) (*gitPackageRevision, error) { +func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repository.PackageRevisionDraft, version string) (repository.PackageRevision, error) { + ctx, span := tracer.Start(ctx, "GitRepository::UpdateLifecycle", trace.WithAttributes()) + defer span.End() + r.mutex.Lock() defer r.mutex.Unlock() + d := prd.(*gitPackageRevisionDraft) + refSpecs := newPushRefSpecBuilder() draftBranch := createDraftName(d.path, d.workspaceName) proposedBranch := createProposedName(d.path, d.workspaceName) diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index 1578caeb..dbf7e77f 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -180,7 +180,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { t.Fatalf("draft.UpdateResources(%#v, %#v) failed: %v", newResources, task, err) } - revision, err := draft.Close(ctx, "v1") + revision, err := repo.ClosePackageRevisionDraft(ctx, draft, "v1") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -207,7 +207,7 @@ func (g GitSuite) TestGitPackageRoundTrip(t *testing.T) { if err := update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished); err != nil { t.Fatalf("UpdateLifecycle failed: %v", err) } - approved, err := update.Close(ctx, "v1") + approved, err := repo.ClosePackageRevisionDraft(ctx, update, "v1") if err != nil { t.Fatalf("Close() of %q, %q failed: %v", packageName, workspace, err) } @@ -418,7 +418,7 @@ func (g GitSuite) TestListPackagesTrivial(t *testing.T) { }); err != nil { t.Fatalf("UpdateResources() failed: %v", err) } - newRevision, err := draft.Close(ctx, "") + newRevision, err := git.ClosePackageRevisionDraft(ctx, draft, "") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -506,7 +506,7 @@ func (g GitSuite) TestCreatePackageInTrivialRepository(t *testing.T) { }); err != nil { t.Fatalf("UpdateResources() failed: %v", err) } - newRevision, err := draft.Close(ctx, "") + newRevision, err := git.ClosePackageRevisionDraft(ctx, draft, "") if err != nil { t.Fatalf("draft.Close() failed: %v", err) } @@ -688,7 +688,7 @@ func (g GitSuite) TestApproveDraft(t *testing.T) { update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) - new, err := update.Close(ctx, "v1") + new, err := git.ClosePackageRevisionDraft(ctx, update, "v1") if err != nil { t.Fatalf("Close failed: %v", err) } @@ -750,7 +750,7 @@ func (g GitSuite) TestApproveDraftWithHistory(t *testing.T) { update.UpdateLifecycle(ctx, v1alpha1.PackageRevisionLifecyclePublished) - new, err := update.Close(ctx, "v1") + new, err := git.ClosePackageRevisionDraft(ctx, update, "v1") if err != nil { t.Fatalf("Close failed: %v", err) } diff --git a/pkg/oci/mutate.go b/pkg/oci/mutate.go index 54ab526d..eaa19b61 100644 --- a/pkg/oci/mutate.go +++ b/pkg/oci/mutate.go @@ -55,7 +55,7 @@ func (r *ociRepository) CreatePackageRevision(ctx context.Context, obj *v1alpha1 } // digestName := ImageDigestName{} - return &ociPackageDraft{ + return &ociPackageRevisionDraft{ packageName: packageName, parent: r, tasks: []v1alpha1.Task{}, @@ -92,7 +92,7 @@ func (r *ociRepository) UpdatePackageRevision(ctx context.Context, old repositor return nil, fmt.Errorf("error fetching image %q: %w", ref, err) } - return &ociPackageDraft{ + return &ociPackageRevisionDraft{ packageName: packageName, parent: r, tasks: []v1alpha1.Task{}, @@ -102,7 +102,7 @@ func (r *ociRepository) UpdatePackageRevision(ctx context.Context, old repositor }, nil } -type ociPackageDraft struct { +type ociPackageRevisionDraft struct { packageName string created time.Time @@ -118,10 +118,10 @@ type ociPackageDraft struct { lifecycle v1alpha1.PackageRevisionLifecycle // New value of the package revision lifecycle } -var _ repository.PackageRevisionDraft = (*ociPackageDraft)(nil) +var _ repository.PackageRevisionDraft = (*ociPackageRevisionDraft)(nil) -func (p *ociPackageDraft) UpdateResources(ctx context.Context, new *v1alpha1.PackageRevisionResources, task *v1alpha1.Task) error { - _, span := tracer.Start(ctx, "ociPackageDraft::UpdateResources", trace.WithAttributes()) +func (p *ociPackageRevisionDraft) UpdateResources(ctx context.Context, new *v1alpha1.PackageRevisionResources, task *v1alpha1.Task) error { + _, span := tracer.Start(ctx, "ociPackageRevisionDraft::UpdateResources", trace.WithAttributes()) defer span.End() buf := bytes.NewBuffer(nil) @@ -190,20 +190,22 @@ func (p *ociPackageDraft) UpdateResources(ctx context.Context, new *v1alpha1.Pac return nil } -func (p *ociPackageDraft) UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error { +func (p *ociPackageRevisionDraft) UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error { p.lifecycle = new return nil } -func (p *ociPackageDraft) GetName() string { +func (p *ociPackageRevisionDraft) GetName() string { return p.packageName } // Finish round of updates. -func (p *ociPackageDraft) Close(ctx context.Context, version string) (repository.PackageRevision, error) { - ctx, span := tracer.Start(ctx, "ociPackageDraft::Close", trace.WithAttributes()) +func (r *ociRepository) ClosePackageRevisionDraft(ctx context.Context, prd repository.PackageRevisionDraft, version string) (repository.PackageRevision, error) { + ctx, span := tracer.Start(ctx, "ociRepository::ClosePackageRevisionDraft", trace.WithAttributes()) defer span.End() + p := prd.(*ociPackageRevisionDraft) + ref := p.tag option := remote.WithAuthFromKeychain(gcrane.Keychain) diff --git a/pkg/repository/fake/repository.go b/pkg/repository/fake/repository.go index 689bba28..864d5e89 100644 --- a/pkg/repository/fake/repository.go +++ b/pkg/repository/fake/repository.go @@ -61,6 +61,10 @@ func (r *Repository) CreatePackageRevision(_ context.Context, pr *v1alpha1.Packa return nil, nil } +func (r *Repository) ClosePackageRevisionDraft(ctx context.Context, prd repository.PackageRevisionDraft, version string) (repository.PackageRevision, error) { + return nil, nil +} + func (r *Repository) DeletePackageRevision(context.Context, repository.PackageRevision) error { return nil } diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 154505f4..dfb8cf3b 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -127,8 +127,6 @@ type PackageRevisionDraft interface { UpdateResources(ctx context.Context, new *v1alpha1.PackageRevisionResources, task *v1alpha1.Task) error // Updates desired lifecycle of the package. The lifecycle is applied on Close. UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error - // Finish round of updates. - Close(ctx context.Context, version string) (PackageRevision, error) GetName() string } @@ -203,6 +201,9 @@ type Repository interface { // CreatePackageRevision creates a new package revision CreatePackageRevision(ctx context.Context, obj *v1alpha1.PackageRevision) (PackageRevisionDraft, error) + // ClosePackageRevisionDraft closes out a Package Revision Draft + ClosePackageRevisionDraft(ctx context.Context, prd PackageRevisionDraft, version string) (PackageRevision, error) + // DeletePackageRevision deletes a package revision DeletePackageRevision(ctx context.Context, old PackageRevision) error From 0c2d3e2c294943661aa53b3fb53628716a7ba9a5 Mon Sep 17 00:00:00 2001 From: liamfallon Date: Tue, 10 Dec 2024 17:31:07 +0000 Subject: [PATCH 2/3] Fix copyright dates --- pkg/cache/memory/repository.go | 2 +- pkg/engine/engine.go | 2 +- pkg/git/draft.go | 2 +- pkg/git/git_test.go | 2 +- pkg/oci/mutate.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cache/memory/repository.go b/pkg/cache/memory/repository.go index 46dc2be9..fc6686e6 100644 --- a/pkg/cache/memory/repository.go +++ b/pkg/cache/memory/repository.go @@ -1,4 +1,4 @@ -// Copyright 2022,2024 The kpt and Nephio Authors +// Copyright 2022, 2024 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. diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 13735076..3405d24c 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1,4 +1,4 @@ -// Copyright 2022,2024 The kpt and Nephio Authors +// Copyright 2022, 2024 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. diff --git a/pkg/git/draft.go b/pkg/git/draft.go index 2f78bf0e..cb33d2b1 100644 --- a/pkg/git/draft.go +++ b/pkg/git/draft.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt and Nephio Authors +// Copyright 2022, 2024 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. diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go index dbf7e77f..9c8614cb 100644 --- a/pkg/git/git_test.go +++ b/pkg/git/git_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt and Nephio Authors +// Copyright 2022, 2024 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. diff --git a/pkg/oci/mutate.go b/pkg/oci/mutate.go index eaa19b61..98964500 100644 --- a/pkg/oci/mutate.go +++ b/pkg/oci/mutate.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt and Nephio Authors +// Copyright 2022, 2024 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. From 6b6afe2a9e30c23a9af2ed9e66d535f56bb224cc Mon Sep 17 00:00:00 2001 From: liamfallon Date: Tue, 10 Dec 2024 17:59:13 +0000 Subject: [PATCH 3/3] Update tracing statement --- pkg/cache/memory/repository.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cache/memory/repository.go b/pkg/cache/memory/repository.go index fc6686e6..2412c7fe 100644 --- a/pkg/cache/memory/repository.go +++ b/pkg/cache/memory/repository.go @@ -172,7 +172,7 @@ func (r *cachedRepository) CreatePackageRevision(ctx context.Context, obj *v1alp } func (r *cachedRepository) ClosePackageRevisionDraft(ctx context.Context, prd repository.PackageRevisionDraft, version string) (repository.PackageRevision, error) { - ctx, span := tracer.Start(ctx, "cachedDraft::Close", trace.WithAttributes()) + ctx, span := tracer.Start(ctx, "cachedRepository::ClosePackageRevisionDraft", trace.WithAttributes()) defer span.End() v, err := r.Version(ctx)