porch: fix issue with package names when repo dir is specified (#3889)

kptdev · Mar 30, 2023 · d91845e · d91845e
1 parent 244a1c2
commit d91845e
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 18 deletions.
diff --git a/porch/pkg/git/draft.go b/porch/pkg/git/draft.go
@@ -33,8 +33,8 @@ import (
 )
 
 type gitPackageDraft struct {
-	parent        *gitRepository
-	path          string
+	parent        *gitRepository // repo is repo containing the package
+	path          string         // the path to the package from the repo root
 	revision      string
 	workspaceName v1alpha1.WorkspaceName
 	updated       time.Time

diff --git a/porch/pkg/git/git.go b/porch/pkg/git/git.go
@@ -277,10 +277,6 @@ func (r *gitRepository) CreatePackageRevision(ctx context.Context, obj *v1alpha1
 	ctx, span := tracer.Start(ctx, "gitRepository::CreatePackageRevision", trace.WithAttributes())
 	defer span.End()
 
-	if !packageInDirectory(obj.Spec.PackageName, r.directory) {
-		return nil, fmt.Errorf("cannot create %s outside of repository directory %q", obj.Spec.PackageName, r.directory)
-	}
-
 	var base plumbing.Hash
 	refName := r.branch.RefInLocal()
 	switch main, err := r.repo.Reference(refName, true); {
@@ -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)
 
-	draft := createDraftName(obj.Spec.PackageName, obj.Spec.WorkspaceName)
+	// TODO use git branches to leverage uniqueness
+	draft := createDraftName(packagePath, obj.Spec.WorkspaceName)
 
 	// TODO: This should also create a new 'Package' resource if one does not already exist
 
 	return &gitPackageDraft{
 		parent:        r,
-		path:          obj.Spec.PackageName,
+		path:          packagePath,
 		workspaceName: obj.Spec.WorkspaceName,
 		lifecycle:     v1alpha1.PackageRevisionLifecycleDraft,
 		updated:       time.Now(),

diff --git a/porch/pkg/git/git_test.go b/porch/pkg/git/git_test.go
@@ -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},
 		},
 		{
 			directory: "nonexistent",
@@ -1140,11 +1140,11 @@ func (g GitSuite) TestNestedDirectories(t *testing.T) {
 		{
 			directory: "catalog/gcp",
 			packages: []string{
-				"catalog/gcp/cloud-sql/v1",
-				"catalog/gcp/spanner/v1",
-				"catalog/gcp/bucket/v2",
-				"catalog/gcp/bucket/v1",
-				"catalog/gcp/bucket/" + g.branch,
+				"cloud-sql/v1",
+				"spanner/v1",
+				"bucket/v2",
+				"bucket/v1",
+				"bucket/" + g.branch,
 			},
 		},
 	} {

diff --git a/porch/pkg/git/package.go b/porch/pkg/git/package.go
@@ -20,6 +20,7 @@ import (
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"path/filepath"
 	"strings"
 	"time"
 
@@ -36,7 +37,7 @@ import (
 
 type gitPackageRevision struct {
 	repo          *gitRepository // repo is repo containing the package
-	path          string
+	path          string         // the path to the package from the repo root
 	revision      string
 	workspaceName v1alpha1.WorkspaceName
 	updated       time.Time
@@ -80,9 +81,20 @@ func (p *gitPackageRevision) UID() types.UID {
 }
 
 func (p *gitPackageRevision) Key() repository.PackageRevisionKey {
+	// if the repository has been registered with a directory, then the
+	// package name is the package path relative to the registered directory
+	packageName := p.path
+	if p.repo.directory != "" {
+		pn, err := filepath.Rel(p.repo.directory, packageName)
+		if err != nil {
+			klog.Errorf("error computing package name relative to registered directory: %w", err)
+		}
+		packageName = strings.TrimLeft(pn, "./")
+	}
+
 	return repository.PackageRevisionKey{
 		Repository:    p.repo.name,
-		Package:       p.path,
+		Package:       packageName,
 		Revision:      p.revision,
 		WorkspaceName: p.workspaceName,
 	}

diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go
@@ -169,11 +169,17 @@ func (t *PorchSuite) TestGitRepository(ctx context.Context) {
 	}
 }
 
-func (t *PorchSuite) TestGitRepositoryWithReleaseTags(ctx context.Context) {
+func (t *PorchSuite) TestGitRepositoryWithReleaseTagsAndDirectory(ctx context.Context) {
 	t.registerGitRepositoryF(ctx, kptRepo, "kpt-repo", "package-examples")
 
 	var list porchapi.PackageRevisionList
 	t.ListF(ctx, &list, client.InNamespace(t.namespace))
+
+	for _, pr := range list.Items {
+		if strings.HasPrefix(pr.Spec.PackageName, "package-examples") {
+			t.Errorf("package name %q should not include repo directory %q as prefix", pr.Spec.PackageName, "package-examples")
+		}
+	}
 }
 
 func (t *PorchSuite) TestCloneFromUpstream(ctx context.Context) {