porch: fix issue with package names when repo dir is specified #3889
Conversation
natasha41575
force-pushed
the
porch/directory
branch
from
dea9de9
to
abe081c
Compare
porch/pkg/git/git.go
Outdated
| // if the repository has been registered with a directory, add the directory name to the name of the package to ensure it is | ||
| // created in the repository directory | ||
| if r.directory != "" { | ||
| obj.Spec.PackageName = r.directory + "/" + obj.Spec.PackageName |
There was a problem hiding this comment.
So it seems like with this solution we are between different patterns for the package name. Is there a way we can avoid this? It seems to create a gotcha that will be surprising for anyone looking at this in the future.
I think the way we want this to work, is that the package name can contain one or more slashes, and these will affect the directory where the package is installed. But these will be considered as relative to the base directory of the repository. So if a package is named foo/my-package and the repository is set up with directory bar, then the package will be created under bar/foo/my-package in git and similar in an OCI repository. Do we know how these "folders" in the name are handled for OCI?
There was a problem hiding this comment.
So it seems like with this solution we are between different patterns for the package name. Is there a way we can avoid this? It seems to create a gotcha that will be surprising for anyone looking at this in the future.
That's fair, I'll update this PR to keep the package name as-is, but have it be created relative to the base directory.
Do we know how these "folders" in the name are handled for OCI?
AFAICT when I create packages in Artifact Registry, there are no "folders" or directories. If I create an packagerevision with a package name e.g. foo/bar, then I see an image named foo/bar in the repo without any folder structure, and the OCI repository struct that we have doesn't appear to store any equivalent to directories in git. So I don't think we need to make any changes to the OCI code for this.
natasha41575
force-pushed
the
porch/directory
branch
3 times, most recently
from
5a46675
to
e7c5e42
Compare
| @@ -296,15 +292,16 @@ func (r *gitRepository) CreatePackageRevision(ctx context.Context, obj *v1alpha1 | |||
| return nil, fmt.Errorf("failed to create packagerevision: %w", err) | |||
| } | |||
|
|
|||
| // TODO use git branches to leverage uniqueness | |||
| packagePath := filepath.Join(r.directory, obj.Spec.PackageName) | |||
There was a problem hiding this comment.
I updated the PR in a way that hopefully makes it more obvious what is going on - the package name is now treated as relative to the directory registered with the repo.
So the package path stored in our structs contains the base directory. And when we get the package name, we get it relative to the base directory.
natasha41575
force-pushed
the
porch/directory
branch
from
e7c5e42
to
aeea337
Compare
natasha41575
force-pushed
the
porch/directory
branch
from
aeea337
to
c0d02fa
Compare
|
Converting this to a draft while I investigate the test failures |
natasha41575
force-pushed
the
porch/directory
branch
from
c0d02fa
to
dcb350b
Compare
| @@ -1131,7 +1131,7 @@ func (g GitSuite) TestNestedDirectories(t *testing.T) { | |||
| }{ | |||
| { | |||
| directory: "sample", | |||
| packages: []string{"sample/v1", "sample/v2", "sample/" + g.branch}, | |||
| packages: []string{"/v1", "/v2", "/" + g.branch}, | |||
There was a problem hiding this comment.
This demonstrates an interesting case. If the registered directory is foo/bar and you have a package at the root of foo/bar, the package name is empty ("").
This is consistent with porch's current behavior; currently when build from head, if there is a package at the root of a repository, and you register the repository without specifying a directory, the package is treated as having an empty name:
$ kpt alpha rpkg get
NAME PACKAGE WORKSPACENAME REVISION LATEST LIFECYCLE REPOSITORY
deployments-333d0669c638bb12e47928ef0d40303a5503099d main main false Published deployments
There was a problem hiding this comment.
Yeah, I think I've experienced this. I think we should consider this a bug. Even if a repo is registered with an empty directory, all packages should exist in subdirectories. We can fix this separately from this PR.
natasha41575
force-pushed
the
porch/directory
branch
from
dcb350b
to
67f3e04
Compare
| @@ -1131,7 +1131,7 @@ func (g GitSuite) TestNestedDirectories(t *testing.T) { | |||
| }{ | |||
| { | |||
| directory: "sample", | |||
| packages: []string{"sample/v1", "sample/v2", "sample/" + g.branch}, | |||
| packages: []string{"/v1", "/v2", "/" + g.branch}, | |||
There was a problem hiding this comment.
Yeah, I think I've experienced this. I think we should consider this a bug. Even if a repo is registered with an empty directory, all packages should exist in subdirectories. We can fix this separately from this PR.
Fixes #3870.
If a repository is registered with a directory, porch should automatically put the package in the registered directory upon creation, and omit the directory name from the package name when fetching the package revision.