Fix revision incrementation when approving a package in a repo registered with --directory

[JamesMcDermott]

Fixes #814

• 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
• also copied suite_utils testing improvements from Add session-conflict resolution for concurrent CUD operations on package revisions #118

[nagygergo]

https://github.com/nordix/porch/blob/1ae60b4366b99064404315984cdbd0f4a44810f5/pkg/cache/repository.go#L249 -- There is also a creation of a PackageRevisionKey, which actually looks like a partial copy, but it seems it would need to refer to the directory as well.

[liamfallon]

The reason the v1 package revision is getting overwritten is because internally the Git porch repo code uses package-name to keep tack of the packages in k8s but uses `/dir/subdir/subsubdir/package-name' to keep track of the packages in git.

Now, as you have shown in this PR, on the key it resolves this by prepending the directory to the package name on the key here:

porch/pkg/git/package.go

Line 94 in dca2f6d

packageName := p.path

Now, this means that when we do a lookup of a package by a package name, we need to use a filter that removes the prepended directory from the package name because the Matches function is in the repository package, and is agnostic of git stuff, see:

porch/pkg/repository/repository.go

Line 151 in dca2f6d

func (f *ListPackageRevisionFilter) Matches(p PackageRevision) bool {

So if we do the following change, everything should be ok:
diff --git a/pkg/git/git.go b/pkg/git/git.go
index 0276c23..bd9738e 100644
--- a/pkg/git/git.go
+++ b/pkg/git/git.go
@@ -1490,11 +1490,13 @@ func (r *gitRepository) CloseDraft(ctx context.Context, d *gitPackageDraft) (*gi

    var newRef *plumbing.Reference

•   splitPath := strings.Split(d.path, "/")
  

•   packageName := splitPath[len(splitPath)-1]
    switch d.lifecycle {
    case v1alpha1.PackageRevisionLifecyclePublished, v1alpha1.PackageRevisionLifecycleDeletionProposed:
            // Finalize the package revision. Assign it a revision number of latest + 1.
            revisions, err := r.listPackageRevisions(ctx, repository.ListPackageRevisionFilter{
  

•                   Package: d.path,
  

•                   Package: packageName,
            })
            if err != nil {
                    return nil, err
  

[liamfallon]

Now, having different package names on packages and in Git strikes me as being dangerous. It would probably be better to have a package name like "dirname.subdirname.subsubdirname.package-name" and just use that everywhere and be consistent. In the current implementation what happens I wonder when we have the same package name in two directories? It might be OK because Porch considers those as two separate "repos" but it seems to me to be very confusing for users and probably should not be allowed. I think fully distinguisned package names would be better. Still the fix above will probably do for now.

[JamesMcDermott]

/retest

[liamfallon]

/approve

[liamfallon]

/approve

[nagygergo]

Now, having different package names on packages and in Git strikes me as being dangerous. It would probably be better to have a package name like "dirname.subdirname.subsubdirname.package-name" and just use that everywhere and be consistent. In the current implementation what happens I wonder when we have the same package name in two directories? It might be OK because Porch considers those as two separate "repos" but it seems to me to be very confusing for users and probably should not be allowed. I think fully distinguisned package names would be better. Still the fix above will probably do for now.

@liamfallon See the following comment:

porch/pkg/git/package.go

Line 57 in b78d785

// Kubernetes resource names requirements do not allow to encode arbitrary directory

[nagygergo]

/approve

[liamfallon]

Now, having different package names on packages and in Git strikes me as being dangerous. It would probably be better to have a package name like "dirname.subdirname.subsubdirname.package-name" and just use that everywhere and be consistent. In the current implementation what happens I wonder when we have the same package name in two directories? It might be OK because Porch considers those as two separate "repos" but it seems to me to be very confusing for users and probably should not be allowed. I think fully distinguisned package names would be better. Still the fix above will probably do for now.

@liamfallon See the following comment:

porch/pkg/git/package.go

Line 57 in b78d785

// Kubernetes resource names requirements do not allow to encode arbitrary directory

Yes, that's how it is today but the question is, should it be like this? Machine generated IDs are fine for internal IDs but they are terrible for user interaction and Porch forces users to use them on CLI etc.

@JamesMcDermott We shouldn't hold up your PR on this conversation.

[liamfallon]

/approve
/lgtm

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JamesMcDermott, liamfallon, nagygergo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [liamfallon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[JamesMcDermott]

/retest

[Catalin-Stratulat-Ericsson]

/test presubmit-nephio-go-test