Repository: GoogleContainerTools/kpt
Pull request #3081 (2022-05-03 20:22:59)

Use trap to delete temporary file
Allows the last command to be `kpt fn eval` which will
propagate its exit status.



Patch:
diff --git a/Makefile b/Makefile
index 405d392c..cd6be768 100644
--- a/Makefile
+++ b/Makefile
@@ -15,6 +15,7 @@
 .PHONY: docs license fix vet fmt lint test build tidy
 
 GOBIN := $(shell go env GOPATH)/bin
+GIT_COMMIT := $(shell git rev-parse --short HEAD)
 
 # T refers to an e2e test case matcher. This enables running e2e tests
 # selectively.  For example,
@@ -27,7 +28,7 @@ T ?= ".*"
 all: generate license fix vet fmt lint license-check test build buildall tidy
 
 build:
-	go build -o $(GOBIN)/kpt -v .
+	go build -ldflags "-X github.com/GoogleContainerTools/kpt/run.version=${GIT_COMMIT}" -o $(GOBIN)/kpt -v .
 
 buildall:
 	GOOS=windows go build -o /dev/null
diff --git a/internal/cmdrpkgclone/command.go b/internal/cmdrpkgclone/command.go
index 60f48aba..2817319b 100644
--- a/internal/cmdrpkgclone/command.go
+++ b/internal/cmdrpkgclone/command.go
@@ -20,6 +20,7 @@ import (
 	"strings"
 
 	"github.com/GoogleContainerTools/kpt/internal/errors"
+	"github.com/GoogleContainerTools/kpt/internal/util/parse"
 	"github.com/GoogleContainerTools/kpt/internal/util/porch"
 	porchapi "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
 	"github.com/spf13/cobra"
@@ -43,6 +44,11 @@ SOURCE_PACKAGE:
     * http://git-repository.git/package-name
     * package-revision-name
 
+  With git URLs, you can either include the package directory and ref in SOURCE_PACKAGE, or
+  you can individually specify the repo, ref, and directory using flags:
+    * kpt alpha rpkg clone http://git-repository.git/package-name@main target --repository=repo
+    * kpt alpha rpkg clone http://git-repository.git target --directory=package-name --ref=main --repository=repo
+
 NAME:
   Target package revision name (downstream package)
   Example: package-name
@@ -53,7 +59,7 @@ Flags:
   Directory within the repository where the upstream package is located.
 
 --ref
-	Ref in the repository where the upstream package is located (branch, tag, SHA)
+  Ref in the repository where the upstream package is located (branch, tag, SHA). The default is 'main'.
 
 --repository
   Repository to which package will be cloned (downstream repository).
@@ -62,7 +68,7 @@ Flags:
   Revision of the downstream package.
 
 --strategy
-  Update strategy that should be used when updating this package; one of: resource-merge, fast-forward, force-delete-replace
+  Update strategy that should be used when updating this package; one of: resource-merge, fast-forward, force-delete-replace.
 `
 )
 
@@ -96,8 +102,8 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner
 
 	c.Flags().StringVar(&r.strategy, "strategy", string(porchapi.ResourceMerge),
 		"update strategy that should be used when updating this package; one of: "+strings.Join(strategies, ","))
-	c.Flags().StringVar(&r.directory, "directory", "/", "Directory within the repository where the upstream package is located.")
-	c.Flags().StringVar(&r.ref, "ref", "main", "Branch in the repository where the upstream package is located.")
+	c.Flags().StringVar(&r.directory, "directory", "", "Directory within the repository where the upstream package is located.")
+	c.Flags().StringVar(&r.ref, "ref", "", "Branch in the repository where the upstream package is located.")
 	c.Flags().StringVar(&r.repository, "repository", "", "Repository to which package will be cloned (downstream repository).")
 	c.Flags().StringVar(&r.revision, "revision", "v1", "Revision of the downstream package.")
 
@@ -154,6 +160,35 @@ func (r *runner) preRunE(cmd *cobra.Command, args []string) error {
 		}
 
 	case strings.Contains(source, "/"):
+		if parse.HasGitSuffix(source) { // extra parsing required
+			repo, dir, ref, err := parse.ParseURL(source)
+			if err != nil {
+				return err
+			}
+			// throw error if values set by flags contradict values parsed from SOURCE_PACKAGE
+			if r.directory != "" && dir != "" && r.directory != dir {
+				return errors.E(op, fmt.Errorf("directory %s specified by --directory contradicts directory %s specified by SOURCE_PACKAGE",
+					r.directory, dir))
+			}
+			if r.ref != "" && ref != "" && r.ref != ref {
+				return errors.E(op, fmt.Errorf("ref %s specified by --ref contradicts ref %s specified by SOURCE_PACKAGE",
+					r.ref, ref))
+			}
+			// grab the values parsed from SOURCE_PACKAGE
+			if r.directory == "" {
+				r.directory = dir
+			}
+			if r.ref == "" {
+				r.ref = ref
+			}
+			source = repo + ".git" // parse.ParseURL removes the git suffix, we need to add it back
+		}
+		if r.ref == "" {
+			r.ref = "main"
+		}
+		if r.directory == "" {
+			r.directory = "/"
+		}
 		r.clone.Upstream.Type = porchapi.RepositoryTypeGit
 		r.clone.Upstream.Git = &porchapi.GitPackage{
 			Repo:      source,
@@ -169,7 +204,6 @@ func (r *runner) preRunE(cmd *cobra.Command, args []string) error {
 	}
 
 	r.target = target
-
 	return nil
 }
 
diff --git a/internal/util/parse/parse.go b/internal/util/parse/parse.go
index cfaaaf18..1cd9d5ab 100644
--- a/internal/util/parse/parse.go
+++ b/internal/util/parse/parse.go
@@ -42,11 +42,7 @@ func GitParseArgs(ctx context.Context, args []string) (Target, error) {
 	}
 
 	// Simple parsing if contains .git{$|/)
-	matched, err := regexp.Match(gitSuffixRegexp, []byte(args[0]))
-	if err != nil {
-		return g, err
-	}
-	if matched {
+	if HasGitSuffix(args[0]) {
 		return targetFromPkgURL(ctx, args[0], args[1])
 	}
 
@@ -93,49 +89,51 @@ func GitParseArgs(ctx context.Context, args []string) (Target, error) {
 // targetFromPkgURL parses a pkg url and destination into kptfile git info and local destination Target
 func targetFromPkgURL(ctx context.Context, pkgURL string, dest string) (Target, error) {
 	g := Target{}
-	repo, dir, version, err := parseURL(ctx, pkgURL)
+	repo, dir, ref, err := ParseURL(pkgURL)
 	if err != nil {
 		return g, err
 	}
+	if dir == "" {
+		dir = "/"
+	}
+	if ref == "" {
+		gur, err := gitutil.NewGitUpstreamRepo(ctx, repo)
+		if err != nil {
+			return g, err
+		}
+		defaultRef, err := gur.GetDefaultBranch(ctx)
+		if err != nil {
+			return g, err
+		}
+		ref = defaultRef
+	}
 	destination, err := getDest(dest, repo, dir)
 	if err != nil {
 		return g, err
 	}
-	g.Ref = version
+	g.Ref = ref
 	g.Directory = path.Clean(dir)
 	g.Repo = repo
 	g.Destination = filepath.Clean(destination)
 	return g, nil
 }
 
-// parseURL parses a pkg url and returns the repo, directory, and version
-func parseURL(ctx context.Context, pkgURL string) (repo string, dir string, version string, err error) {
+// ParseURL parses a pkg url (must contain ".git") and returns the repo, directory, and version
+func ParseURL(pkgURL string) (repo string, dir string, ref string, err error) {
 	parts := regexp.MustCompile(gitSuffixRegexp).Split(pkgURL, 2)
 	index := strings.Index(pkgURL, parts[0])
 	repo = strings.Join([]string{pkgURL[:index], parts[0]}, "")
 	switch {
-	case len(parts) == 1:
+	case len(parts) == 1 || parts[1] == "":
 		// do nothing
 	case strings.Contains(parts[1], "@"):
 		parts := strings.Split(parts[1], "@")
-		version = strings.TrimSuffix(parts[1], "/")
+		ref = strings.TrimSuffix(parts[1], "/")
 		dir = string(filepath.Separator) + parts[0]
 	default:
 		dir = string(filepath.Separator) + parts[1]
 	}
-	if version == "" {
-		gur, err := gitutil.NewGitUpstreamRepo(ctx, repo)
-		if err != nil {
-			return "", "", "", err
-		}
-		defaultRef, err := gur.GetDefaultBranch(ctx)
-		if err != nil {
-			return "", "", "", err
-		}
-		version = defaultRef
-	}
-
-	return repo, dir, version, nil
+	return repo, dir, ref, nil
 }
 
 // pkgURLFromGHURL converts a GitHub URL into a well formed pkg url
@@ -287,3 +285,9 @@ func getDest(v, repo, subdir string) (string, error) {
 	}
 	return v, nil
 }
+
+// HasGitSuffix returns true if the provided pkgURL is a git repo containing the ".git" suffix
+func HasGitSuffix(pkgURL string) bool {
+	matched, err := regexp.Match(gitSuffixRegexp, []byte(pkgURL))
+	return matched && err == nil
+}
diff --git a/porch/Makefile b/porch/Makefile
index 05630d8f..1a398bcb 100644
--- a/porch/Makefile
+++ b/porch/Makefile
@@ -28,7 +28,16 @@ MODULES = \
 # GCP project to use for development
 export GCP_PROJECT_ID ?= $(shell gcloud config get-value project)
 export IMAGE_REPO ?= gcr.io/$(GCP_PROJECT_ID)
-export IMAGE_TAG ?= latest
+export IMAGE_TAG
+ifndef IMAGE_TAG
+  git_tag := $(shell git rev-parse --short HEAD || "latest" )
+  $(shell git diff --quiet)
+  ifneq ($(.SHELLSTATUS), 0)
+    git_tag := $(git_tag)-dirty
+  endif
+
+  IMAGE_TAG=$(git_tag)
+endif
 
 PORCH_SERVER_IMAGE ?= porch-server
 PORCH_FUNCTION_RUNNER_IMAGE ?= porch-function-runner
diff --git a/porch/scripts/create-deployment-blueprint.sh b/porch/scripts/create-deployment-blueprint.sh
index efec9c0e..55783a13 100755
--- a/porch/scripts/create-deployment-blueprint.sh
+++ b/porch/scripts/create-deployment-blueprint.sh
@@ -131,8 +131,9 @@ function customize-image-in-env {
   local NEW="${2}"
   local TAG="${NEW##*:}"
   local IMG="${NEW%:*}"
+  local FN_CONFIG="${PORCH_DIR}/.build/set-image-config.yaml"
 
-  cat > set-image-config.yaml << EOF
+  cat > "${FN_CONFIG}" << EOF
 apiVersion: fn.kpt.dev/v1alpha1
 kind: SetImage
 metadata:
@@ -140,7 +141,7 @@ metadata:
 image:
   name: ${OLD}
   newName: ${IMG}
-  newTag: "${TAG}"
+  newTag: ${TAG}
 additionalImageFields:
 - group: apps
   version: v1
@@ -148,8 +149,9 @@ additionalImageFields:
   path: spec/template/spec/containers[]/env[]/value
 EOF
 
-  kpt fn eval "${DESTINATION}" --image set-image:v0.1.0 --fn-config set-image-config.yaml
-  rm set-image-config.yaml
+  trap "rm -f ${FN_CONFIG}" EXIT
+
+  kpt fn eval "${DESTINATION}" --image set-image:v0.1.0 --fn-config "${FN_CONFIG}" || echo "kpt fn eval failed"
 }
 
 function customize-sa {


Test patch:
diff --git a/e2e/testdata/porch/rpkg-clone/config.yaml b/e2e/testdata/porch/rpkg-clone/config.yaml
index d235d975..8c23448f 100644
--- a/e2e/testdata/porch/rpkg-clone/config.yaml
+++ b/e2e/testdata/porch/rpkg-clone/config.yaml
@@ -11,9 +11,7 @@ commands:
       - rpkg
       - clone
       - --namespace=rpkg-clone
-      - https://github.com/platkrm/test-blueprints.git
-      - --directory=basens
-      - --ref=basens/v1
+      - https://github.com/platkrm/test-blueprints.git/basens@basens/v1
       - --repository=git
       - --revision=v0
       - basens-clone
diff --git a/internal/util/parse/parse_test.go b/internal/util/parse/parse_test.go
index cec11b66..47799d9d 100644
--- a/internal/util/parse/parse_test.go
+++ b/internal/util/parse/parse_test.go
@@ -201,7 +201,7 @@ func Test_parseURL(t *testing.T) {
 		},
 		"no ref": {
 			ghURL:    "https://github.com/GoogleContainerTools/kpt-functions-catalog.git/examples/apply-replacements-simple",
-			expected: expected{repo: "https://github.com/GoogleContainerTools/kpt-functions-catalog", dir: "/examples/apply-replacements-simple", version: "master"},
+			expected: expected{repo: "https://github.com/GoogleContainerTools/kpt-functions-catalog", dir: "/examples/apply-replacements-simple", version: ""},
 		},
 		".git appears in the middle": {
 			ghURL:    "https://my-site.com/root.gitops.git/foo@main",
@@ -211,8 +211,7 @@ func Test_parseURL(t *testing.T) {
 	for name, test := range tests {
 		test := test // capture range variable
 		t.Run(name, func(t *testing.T) {
-			ctx := printer.WithContext(context.Background(), printer.New(nil, nil))
-			repo, dir, version, err := parseURL(ctx, test.ghURL)
+			repo, dir, version, err := ParseURL(test.ghURL)
 			assert.NoError(t, err)
 			assert.Equal(t, expected{repo: repo, dir: dir, version: version}, test.expected)
 		})