From a83a4bc10e886e43457f3bec60924b663b2b75aa Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Thu, 17 Oct 2024 14:07:48 +0100 Subject: [PATCH 1/3] Add package directory to ListPackageRevision filtering - make revision incrementation on rpkg approve work with --directory repos - add new Directory attribute to PackageRevisionKey - and use it when matching PackageRevisions to filter out from the List operation - package approval flow looks for previous copies of the package to determine new latest revision string (e.g. v1 -> v2) - this involves matching the package-for-approval's path attribute against existing packages' packageName attribute - however: - when a repo is registered using --directory to associate it with a subfolder in the upstream Git repo, packageName is deliberately trimmed of the directory attribute if it exists - but the path in the package-for-approval still has the directory part - so no existing packages can match to be counted as previous copies - therefore the revision can never increase beyond "v1" and the v1 tag is overwritten in Git with each new approval - this also results in previous package copies disappearing --- pkg/git/package.go | 1 + pkg/repository/repository.go | 20 +++++++++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/pkg/git/package.go b/pkg/git/package.go index b23c05f0..551701fe 100644 --- a/pkg/git/package.go +++ b/pkg/git/package.go @@ -102,6 +102,7 @@ func (p *gitPackageRevision) Key() repository.PackageRevisionKey { return repository.PackageRevisionKey{ Repository: p.repo.name, + Directory: p.repo.directory, Package: packageName, Revision: p.revision, WorkspaceName: p.workspaceName, diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 2bd0b041..30e040a1 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -17,6 +17,7 @@ package repository import ( "context" "fmt" + "strings" "github.com/go-git/go-git/v5/plumbing/transport" "github.com/nephio-project/porch/api/porch/v1alpha1" @@ -31,13 +32,13 @@ type PackageResources struct { } type PackageRevisionKey struct { - Repository, Package, Revision string - WorkspaceName v1alpha1.WorkspaceName + Repository, Directory, Package, Revision string + WorkspaceName v1alpha1.WorkspaceName } func (n PackageRevisionKey) String() string { - return fmt.Sprintf("Repository: %q, Package: %q, Revision: %q, WorkspaceName: %q", - n.Repository, n.Package, n.Revision, string(n.WorkspaceName)) + return fmt.Sprintf("Repository: %q, Directory: %q, Package: %q, Revision: %q, WorkspaceName: %q", + n.Repository, n.Directory, n.Package, n.Revision, string(n.WorkspaceName)) } type PackageKey struct { @@ -149,13 +150,18 @@ type ListPackageRevisionFilter struct { // Matches returns true if the provided PackageRevision satisfies the conditions in the filter. func (f *ListPackageRevisionFilter) Matches(p PackageRevision) bool { - if f.Package != "" && f.Package != p.Key().Package { + packageKey := p.Key() + + fullPackagePath := strings.TrimPrefix( + fmt.Sprintf("%s/%s", packageKey.Directory, packageKey.Package), + "/") + if f.Package != "" && f.Package != fullPackagePath { return false } - if f.Revision != "" && f.Revision != p.Key().Revision { + if f.Revision != "" && f.Revision != packageKey.Revision { return false } - if f.WorkspaceName != "" && f.WorkspaceName != p.Key().WorkspaceName { + if f.WorkspaceName != "" && f.WorkspaceName != packageKey.WorkspaceName { return false } if f.KubeObjectName != "" && f.KubeObjectName != p.KubeObjectName() { From 1ae60b4366b99064404315984cdbd0f4a44810f5 Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Fri, 18 Oct 2024 17:29:55 +0100 Subject: [PATCH 2/3] Add E2E test for revision incrementation with --directory repos - also for packages with slashes in the name --- test/e2e/e2e_test.go | 78 +++++++++++++++++++++++++++++++++++++++++ test/e2e/suite_utils.go | 54 +++++++++++++++++----------- 2 files changed, 111 insertions(+), 21 deletions(-) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 5233073d..e24118ec 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -864,6 +864,84 @@ func (t *PorchSuite) TestProposeApprove(ctx context.Context) { } } +func (t *PorchSuite) TestSubfolderPackageRevisionIncrementation(ctx context.Context) { + const ( + repository = "lifecycle" + subfolderRepository = "repo-in-subfolder" + subfolderDirectory = "randomRepoFolder" + normalPackageName = "test-package" + subfolderPackageName = "randomPackageFolder/test-package" + workspace = "workspace" + workspace2 = "workspace2" + ) + + // Register the repositories + t.RegisterMainGitRepositoryF(ctx, repository) + t.RegisterGitRepositoryWithDirectoryF(ctx, subfolderRepository, subfolderDirectory) + + // Create a new package (via init) + subfolderPr := t.CreatePackageDraftF(ctx, repository, subfolderPackageName, workspace) + prInSubfolder := t.CreatePackageDraftF(ctx, subfolderRepository, normalPackageName, workspace) + + // Propose and approve the package revisions + subfolderPr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + prInSubfolder.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(ctx, subfolderPr) + t.UpdateF(ctx, prInSubfolder) + + subfolderPr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + prInSubfolder.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + subfolderPr = t.UpdateApprovalF(ctx, subfolderPr, metav1.UpdateOptions{}) + prInSubfolder = t.UpdateApprovalF(ctx, prInSubfolder, metav1.UpdateOptions{}) + + assert.Equal(t, porchapi.PackageRevisionLifecyclePublished, subfolderPr.Spec.Lifecycle) + assert.Equal(t, "v1", subfolderPr.Spec.Revision) + assert.Equal(t, porchapi.PackageRevisionLifecyclePublished, prInSubfolder.Spec.Lifecycle) + assert.Equal(t, "v1", prInSubfolder.Spec.Revision) + + // Create new package revisions via edit/copy + editedSubfolderPr := t.CreatePackageSkeleton(repository, subfolderPackageName, workspace2) + editedSubfolderPr.Spec.Tasks = []porchapi.Task{ + { + Type: porchapi.TaskTypeEdit, + Edit: &porchapi.PackageEditTaskSpec{ + Source: &porchapi.PackageRevisionRef{ + Name: subfolderPr.Name, + }, + }, + }, + } + t.CreateF(ctx, editedSubfolderPr) + editedPrInSubfolder := t.CreatePackageSkeleton(subfolderRepository, normalPackageName, workspace2) + editedPrInSubfolder.Spec.Tasks = []porchapi.Task{ + { + Type: porchapi.TaskTypeEdit, + Edit: &porchapi.PackageEditTaskSpec{ + Source: &porchapi.PackageRevisionRef{ + Name: prInSubfolder.Name, + }, + }, + }, + } + t.CreateF(ctx, editedPrInSubfolder) + + // Propose and approve these package revisions as well + editedSubfolderPr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + editedPrInSubfolder.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(ctx, editedSubfolderPr) + t.UpdateF(ctx, editedPrInSubfolder) + + editedSubfolderPr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + editedPrInSubfolder.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + editedSubfolderPr = t.UpdateApprovalF(ctx, editedSubfolderPr, metav1.UpdateOptions{}) + editedPrInSubfolder = t.UpdateApprovalF(ctx, editedPrInSubfolder, metav1.UpdateOptions{}) + + assert.Equal(t, porchapi.PackageRevisionLifecyclePublished, editedSubfolderPr.Spec.Lifecycle) + assert.Equal(t, "v2", editedSubfolderPr.Spec.Revision) + assert.Equal(t, porchapi.PackageRevisionLifecyclePublished, editedPrInSubfolder.Spec.Lifecycle) + assert.Equal(t, "v2", editedPrInSubfolder.Spec.Revision) +} + func (t *PorchSuite) TestDeleteDraft(ctx context.Context) { const ( repository = "delete-draft" diff --git a/test/e2e/suite_utils.go b/test/e2e/suite_utils.go index 801325a0..ccf0c170 100644 --- a/test/e2e/suite_utils.go +++ b/test/e2e/suite_utils.go @@ -48,7 +48,8 @@ func (p *TestSuiteWithGit) Initialize(ctx context.Context) { p.gitConfig = p.CreateGitRepo() } -func (p *TestSuiteWithGit) GitConfig(repoID string) GitConfig { +func (p *TestSuiteWithGit) GitConfig(name string) GitConfig { + repoID := p.Namespace + "-" + name config := p.gitConfig config.Repo = config.Repo + "/" + repoID return config @@ -56,8 +57,14 @@ func (p *TestSuiteWithGit) GitConfig(repoID string) GitConfig { func (t *TestSuiteWithGit) RegisterMainGitRepositoryF(ctx context.Context, name string, opts ...RepositoryOption) { t.Helper() - repoID := t.Namespace + "-" + name - config := t.GitConfig(repoID) + config := t.GitConfig(name) + t.registerGitRepositoryFromConfigF(ctx, name, config, opts...) +} + +func (t *TestSuiteWithGit) RegisterGitRepositoryWithDirectoryF(ctx context.Context, name string, directory string, opts ...RepositoryOption) { + t.Helper() + config := t.GitConfig(name) + config.Directory = directory t.registerGitRepositoryFromConfigF(ctx, name, config, opts...) } @@ -213,7 +220,7 @@ func (t *TestSuite) registerGitRepositoryFromConfigF(ctx context.Context, name s t.Cleanup(func() { t.DeleteE(ctx, repository) t.WaitUntilRepositoryDeleted(ctx, name, t.Namespace) - t.WaitUntilAllPackagesDeleted(ctx, name) + t.WaitUntilAllPackagesDeleted(ctx, name, t.Namespace) }) // Make sure the repository is ready before we test to (hopefully) @@ -249,9 +256,21 @@ func InNamespace(ns string) RepositoryOption { } // Creates an empty package draft by initializing an empty package -func (t *TestSuite) CreatePackageDraftF(ctx context.Context, repository, name, workspace string) *porchapi.PackageRevision { +func (t *TestSuite) CreatePackageDraftF(ctx context.Context, repository, packageName, workspace string) *porchapi.PackageRevision { t.Helper() - pr := &porchapi.PackageRevision{ + pr := t.CreatePackageSkeleton(repository, packageName, workspace) + pr.Spec.Tasks = []porchapi.Task{ + { + Type: porchapi.TaskTypeInit, + Init: &porchapi.PackageInitTaskSpec{}, + }, + } + t.CreateF(ctx, pr) + return pr +} + +func (t *TestSuite) CreatePackageSkeleton(repoName, packageName, workspace string) *porchapi.PackageRevision { + return &porchapi.PackageRevision{ TypeMeta: metav1.TypeMeta{ Kind: "PackageRevision", APIVersion: porchapi.SchemeGroupVersion.String(), @@ -260,19 +279,13 @@ func (t *TestSuite) CreatePackageDraftF(ctx context.Context, repository, name, w Namespace: t.Namespace, }, Spec: porchapi.PackageRevisionSpec{ - PackageName: name, + PackageName: packageName, WorkspaceName: porchapi.WorkspaceName(workspace), - RepositoryName: repository, - Tasks: []porchapi.Task{ - { - Type: porchapi.TaskTypeInit, - Init: &porchapi.PackageInitTaskSpec{}, - }, - }, + RepositoryName: repoName, + // empty tasks list - set them as needed in the particular usage + Tasks: []porchapi.Task{}, }, } - t.CreateF(ctx, pr) - return pr } func (t *TestSuite) MustExist(ctx context.Context, key client.ObjectKey, obj client.Object) { @@ -365,7 +378,7 @@ func (t *TestSuite) WaitUntilRepositoryDeleted(ctx context.Context, name, namesp } } -func (t *TestSuite) WaitUntilAllPackagesDeleted(ctx context.Context, repoName string) { +func (t *TestSuite) WaitUntilAllPackagesDeleted(ctx context.Context, repoName string, namespace string) { t.Helper() err := wait.PollUntilContextTimeout(ctx, time.Second, 60*time.Second, true, func(ctx context.Context) (done bool, err error) { t.Helper() @@ -375,20 +388,19 @@ func (t *TestSuite) WaitUntilAllPackagesDeleted(ctx context.Context, repoName st return false, nil } for _, pkgRev := range pkgRevList.Items { - if strings.HasPrefix(fmt.Sprintf("%s-", pkgRev.Name), repoName) { + if pkgRev.Namespace == namespace && strings.HasPrefix(fmt.Sprintf("%s-", pkgRev.Name), repoName) { t.Logf("Found package %s from repo %s", pkgRev.Name, repoName) return false, nil } } - var internalPkgRevList internalapi.PackageRevList if err := t.Client.List(ctx, &internalPkgRevList); err != nil { t.Logf("error list internal packages: %v", err) return false, nil } for _, internalPkgRev := range internalPkgRevList.Items { - if strings.HasPrefix(fmt.Sprintf("%s-", internalPkgRev.Name), repoName) { - t.Logf("Found internalPkg %s from repo %s", internalPkgRev.Name, repoName) + if internalPkgRev.Namespace == namespace && strings.HasPrefix(fmt.Sprintf("%s-", internalPkgRev.Name), repoName) { + t.Logf("Found internalPkg %s/%s from repo %s", internalPkgRev.Namespace, internalPkgRev.Name, repoName) return false, nil } } From 2d087468d76e4ff4aa8c94b1d7981ca71383d467 Mon Sep 17 00:00:00 2001 From: ezmcdja Date: Tue, 22 Oct 2024 16:17:48 +0100 Subject: [PATCH 3/3] #814 - move directory matching alignment to Git side instead of PackageRevisionKey - in Git-side CloseDraft method, trim directory path off full package path before listing PackageRevisions - ensuring only the package name will be used when matching PackageRevisions to filter out, whether or not the repo has a directory set https://github.com/nephio-project/nephio/issues/814 --- pkg/git/git.go | 4 +++- pkg/git/package.go | 1 - pkg/repository/repository.go | 14 +++++--------- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/pkg/git/git.go b/pkg/git/git.go index 0276c23d..985085b9 100644 --- a/pkg/git/git.go +++ b/pkg/git/git.go @@ -1493,8 +1493,10 @@ func (r *gitRepository) CloseDraft(ctx context.Context, d *gitPackageDraft) (*gi switch d.lifecycle { case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed: // Finalize the package revision. Assign it a revision number of latest + 1. + packageDirectory := d.parent.directory + packageName := strings.TrimPrefix(d.path, packageDirectory+"/") revisions, err := r.listPackageRevisions(ctx, repository.ListPackageRevisionFilter{ - Package: d.path, + Package: packageName, }) if err != nil { return nil, err diff --git a/pkg/git/package.go b/pkg/git/package.go index 551701fe..b23c05f0 100644 --- a/pkg/git/package.go +++ b/pkg/git/package.go @@ -102,7 +102,6 @@ func (p *gitPackageRevision) Key() repository.PackageRevisionKey { return repository.PackageRevisionKey{ Repository: p.repo.name, - Directory: p.repo.directory, Package: packageName, Revision: p.revision, WorkspaceName: p.workspaceName, diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 30e040a1..d36dd783 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -17,7 +17,6 @@ package repository import ( "context" "fmt" - "strings" "github.com/go-git/go-git/v5/plumbing/transport" "github.com/nephio-project/porch/api/porch/v1alpha1" @@ -32,13 +31,13 @@ type PackageResources struct { } type PackageRevisionKey struct { - Repository, Directory, Package, Revision string - WorkspaceName v1alpha1.WorkspaceName + Repository, Package, Revision string + WorkspaceName v1alpha1.WorkspaceName } func (n PackageRevisionKey) String() string { - return fmt.Sprintf("Repository: %q, Directory: %q, Package: %q, Revision: %q, WorkspaceName: %q", - n.Repository, n.Directory, n.Package, n.Revision, string(n.WorkspaceName)) + return fmt.Sprintf("Repository: %q, Package: %q, Revision: %q, WorkspaceName: %q", + n.Repository, n.Package, n.Revision, string(n.WorkspaceName)) } type PackageKey struct { @@ -152,10 +151,7 @@ type ListPackageRevisionFilter struct { func (f *ListPackageRevisionFilter) Matches(p PackageRevision) bool { packageKey := p.Key() - fullPackagePath := strings.TrimPrefix( - fmt.Sprintf("%s/%s", packageKey.Directory, packageKey.Package), - "/") - if f.Package != "" && f.Package != fullPackagePath { + if f.Package != "" && f.Package != packageKey.Package { return false } if f.Revision != "" && f.Revision != packageKey.Revision {