Repository: google/ko
Pull request #234 (2020-11-02 22:55:48)

Refactor the publish.Namer, add `--bare` option for image naming
In some environments (e.g. ECR) each repository needs to be pre-created, which gets tedious.

Given that `ko` does everything by digest, this collapses all of the images published into the same repository: `${KO_DOCKER_REPO}` with no suffix to discriminate between images.

This is also beneficial from a security standpoint in certain contexts, as `KO_DOCKER_REPO=docker.io/{user}` might publish new repositories as public by default.

mattmoor: For example with ECR, my registry domain is `912237561242.dkr.ecr.us-west-2.amazonaws.com`, and I have to create each of: `912237561242.dkr.ecr.us-west-2.amazonaws.com/{foo}` today.

With this, I can use `KO_DOCKER_REPO=912237561242.dkr.ecr.us-west-2.amazonaws.com/ko-images` and `ko apply --naked` will publish all images to the above repository and substitute their digests into the yamls, e.g. `912237561242.dkr.ecr.us-west-2.amazonaws.com/ko-images@sha256:deadbeef`

In these cases, no part of the import path is incorporated into the repository name, which is where the name "naked" comes from.  I'm open to alternative naming suggestions, but struggled to find a more descriptive term than this.
jonjohnsonjr: My only real issue with this is having a global flag called `--naked`. I wish we had a separate mechanism for selecting a namer that wasn't flag-based, ref:

https://github.com/google/ko/issues/172
https://github.com/google/ko/issues/143
mattmoor: @ImJasonH also doesn't like the name either.  He suggested `--bare`, which I think bears the same meaning as `--naked`, but without the same (level of) vulgar connotation 🤷 

I am happy to switch to `--bare`, though I'd probably drop the short flag since we already have `-B`.
jonjohnsonjr: I'd prefer dropping the short flag regardless, since they're such a precious resource. In fact, it would be great if this was really cumbersome to type so that you got annoyed into implementing a better mechanism for this :) 

Some dumb ideas that might inspire better ideas:
* `--flat`
* `--direct`
* `--nosuffix`
* `--norepo[sitory]`
* `--noname`
* `--nemo`

In this mode, we should probably warn or error if you pass a single tag and we publish multiple images, because the tag will get overwritten. Alternatively (lazily), just document this caveat in the README or flag description.

Patch:
diff --git a/pkg/commands/options/publish.go b/pkg/commands/options/publish.go
index 21265231..09a74e09 100644
--- a/pkg/commands/options/publish.go
+++ b/pkg/commands/options/publish.go
@@ -41,11 +41,14 @@ type PublishOptions struct {
 	PreserveImportPaths bool
 	// BaseImportPaths uses the base path without MD5 hash after KO_DOCKER_REPO.
 	BaseImportPaths bool
+	// Base uses a tag on the KO_DOCKER_REPO without anything additional.
+	Bare bool
 }
 
 func AddPublishArg(cmd *cobra.Command, po *PublishOptions) {
 	cmd.Flags().StringSliceVarP(&po.Tags, "tags", "t", []string{"latest"},
-		"Which tags to use for the produced image instead of the default 'latest' tag.")
+		"Which tags to use for the produced image instead of the default 'latest' tag "+
+			"(may not work properly with --base-import-paths or --bare).")
 
 	cmd.Flags().BoolVar(&po.Push, "push", true, "Push images to KO_DOCKER_REPO")
 
@@ -60,21 +63,27 @@ func AddPublishArg(cmd *cobra.Command, po *PublishOptions) {
 	cmd.Flags().BoolVarP(&po.PreserveImportPaths, "preserve-import-paths", "P", po.PreserveImportPaths,
 		"Whether to preserve the full import path after KO_DOCKER_REPO.")
 	cmd.Flags().BoolVarP(&po.BaseImportPaths, "base-import-paths", "B", po.BaseImportPaths,
-		"Whether to use the base path without MD5 hash after KO_DOCKER_REPO.")
+		"Whether to use the base path without MD5 hash after KO_DOCKER_REPO (may not work properly with --tags).")
+	cmd.Flags().BoolVar(&po.Bare, "bare", po.Bare,
+		"Whether to just use KO_DOCKER_REPO without additional context (will not work properly with --tags).")
 }
 
-func packageWithMD5(importpath string) string {
+func packageWithMD5(base, importpath string) string {
 	hasher := md5.New() //nolint: gosec // No strong cryptography needed.
 	hasher.Write([]byte(importpath))
-	return filepath.Base(importpath) + "-" + hex.EncodeToString(hasher.Sum(nil))
+	return filepath.Join(base, filepath.Base(importpath)+"-"+hex.EncodeToString(hasher.Sum(nil)))
 }
 
-func preserveImportPath(importpath string) string {
-	return importpath
+func preserveImportPath(base, importpath string) string {
+	return filepath.Join(base, importpath)
 }
 
-func baseImportPaths(importpath string) string {
-	return filepath.Base(importpath)
+func baseImportPaths(base, importpath string) string {
+	return filepath.Join(base, filepath.Base(importpath))
+}
+
+func bareDockerRepo(base, _ string) string {
+	return base
 }
 
 func MakeNamer(po *PublishOptions) publish.Namer {
@@ -82,6 +91,8 @@ func MakeNamer(po *PublishOptions) publish.Namer {
 		return preserveImportPath
 	} else if po.BaseImportPaths {
 		return baseImportPaths
+	} else if po.Bare {
+		return bareDockerRepo
 	}
 	return packageWithMD5
 }
diff --git a/pkg/commands/resolver.go b/pkg/commands/resolver.go
index 24d18961..89b29d00 100644
--- a/pkg/commands/resolver.go
+++ b/pkg/commands/resolver.go
@@ -192,7 +192,7 @@ func (n nopPublisher) Publish(br build.Result, s string) (name.Reference, error)
 	if err != nil {
 		return nil, err
 	}
-	return name.NewDigest(fmt.Sprintf("%s/%s@%s", n.repoName, n.namer(s), h))
+	return name.NewDigest(fmt.Sprintf("%s@%s", n.namer(n.repoName, s), h))
 }
 
 func (n nopPublisher) Close() error { return nil }
diff --git a/pkg/publish/daemon.go b/pkg/publish/daemon.go
index ce4778e9..a7a200bb 100644
--- a/pkg/publish/daemon.go
+++ b/pkg/publish/daemon.go
@@ -93,7 +93,7 @@ func (d *demon) Publish(br build.Result, s string) (name.Reference, error) {
 		return nil, err
 	}
 
-	digestTag, err := name.NewTag(fmt.Sprintf("%s/%s:%s", LocalDomain, d.namer(s), h.Hex))
+	digestTag, err := name.NewTag(fmt.Sprintf("%s:%s", d.namer(LocalDomain, s), h.Hex))
 	if err != nil {
 		return nil, err
 	}
@@ -106,7 +106,7 @@ func (d *demon) Publish(br build.Result, s string) (name.Reference, error) {
 
 	for _, tagName := range d.tags {
 		log.Printf("Adding tag %v", tagName)
-		tag, err := name.NewTag(fmt.Sprintf("%s/%s:%s", LocalDomain, d.namer(s), tagName))
+		tag, err := name.NewTag(fmt.Sprintf("%s:%s", d.namer(LocalDomain, s), tagName))
 		if err != nil {
 			return nil, err
 		}
diff --git a/pkg/publish/default.go b/pkg/publish/default.go
index 7724efdc..a1f64acd 100644
--- a/pkg/publish/default.go
+++ b/pkg/publish/default.go
@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"log"
 	"net/http"
+	"path/filepath"
 	"strings"
 
 	"github.com/google/go-containerregistry/pkg/authn"
@@ -52,13 +53,13 @@ type defaultOpener struct {
 
 // Namer is a function from a supported import path to the portion of the resulting
 // image name that follows the "base" repository name.
-type Namer func(string) string
+type Namer func(string, string) string
 
 // identity is the default namer, so import paths are affixed as-is under the repository
 // name for maximum clarity, e.g.
 //   gcr.io/foo/github.com/bar/baz/cmd/blah
 //   ^--base--^ ^-------import path-------^
-func identity(in string) string { return in }
+func identity(base, in string) string { return filepath.Join(base, in) }
 
 // As some registries do not support pushing an image by digest, the default tag for pushing
 // is the 'latest' tag.
@@ -132,7 +133,7 @@ func (d *defalt) Publish(br build.Result, s string) (name.Reference, error) {
 	}
 
 	for i, tagName := range d.tags {
-		tag, err := name.NewTag(fmt.Sprintf("%s/%s:%s", d.base, d.namer(s), tagName), no...)
+		tag, err := name.NewTag(fmt.Sprintf("%s:%s", d.namer(d.base, s), tagName), no...)
 		if err != nil {
 			return nil, err
 		}
@@ -154,11 +155,11 @@ func (d *defalt) Publish(br build.Result, s string) (name.Reference, error) {
 	if err != nil {
 		return nil, err
 	}
-	ref := fmt.Sprintf("%s/%s@%s", d.base, d.namer(s), h)
+	ref := fmt.Sprintf("%s@%s", d.namer(d.base, s), h)
 	if len(d.tags) == 1 && d.tags[0] != defaultTags[0] {
 		// If a single tag is explicitly set (not latest), then this
 		// is probably a release, so include the tag in the reference.
-		ref = fmt.Sprintf("%s/%s:%s@%s", d.base, d.namer(s), d.tags[0], h)
+		ref = fmt.Sprintf("%s:%s@%s", d.namer(d.base, s), d.tags[0], h)
 	}
 	dig, err := name.NewDigest(ref)
 	if err != nil {
diff --git a/pkg/publish/kind.go b/pkg/publish/kind.go
index 21f9b976..a949dbf3 100644
--- a/pkg/publish/kind.go
+++ b/pkg/publish/kind.go
@@ -95,7 +95,7 @@ func (t *kindPublisher) Publish(br build.Result, s string) (name.Reference, erro
 		return nil, err
 	}
 
-	digestTag, err := name.NewTag(fmt.Sprintf("%s/%s:%s", KindDomain, t.namer(s), h.Hex))
+	digestTag, err := name.NewTag(fmt.Sprintf("%s:%s", t.namer(KindDomain, s), h.Hex))
 	if err != nil {
 		return nil, err
 	}
@@ -108,7 +108,7 @@ func (t *kindPublisher) Publish(br build.Result, s string) (name.Reference, erro
 
 	for _, tagName := range t.tags {
 		log.Printf("Adding tag %v", tagName)
-		tag, err := name.NewTag(fmt.Sprintf("%s/%s:%s", KindDomain, t.namer(s), tagName))
+		tag, err := name.NewTag(fmt.Sprintf("%s:%s", t.namer(KindDomain, s), tagName))
 		if err != nil {
 			return nil, err
 		}
diff --git a/pkg/publish/tarball.go b/pkg/publish/tarball.go
index 9a008a4f..4691b58a 100644
--- a/pkg/publish/tarball.go
+++ b/pkg/publish/tarball.go
@@ -57,7 +57,7 @@ func (t *tar) Publish(br build.Result, s string) (name.Reference, error) {
 	}
 
 	for _, tagName := range t.tags {
-		tag, err := name.ParseReference(fmt.Sprintf("%s/%s:%s", t.base, t.namer(s), tagName))
+		tag, err := name.ParseReference(fmt.Sprintf("%s:%s", t.namer(t.base, s), tagName))
 		if err != nil {
 			return nil, err
 		}
@@ -70,18 +70,18 @@ func (t *tar) Publish(br build.Result, s string) (name.Reference, error) {
 	}
 
 	if len(t.tags) == 0 {
-		ref, err := name.ParseReference(fmt.Sprintf("%s/%s@%s", t.base, t.namer(s), h))
+		ref, err := name.ParseReference(fmt.Sprintf("%s@%s", t.namer(t.base, s), h))
 		if err != nil {
 			return nil, err
 		}
 		t.refs[ref] = img
 	}
 
-	ref := fmt.Sprintf("%s/%s@%s", t.base, t.namer(s), h)
+	ref := fmt.Sprintf("%s@%s", t.namer(t.base, s), h)
 	if len(t.tags) == 1 && t.tags[0] != defaultTags[0] {
 		// If a single tag is explicitly set (not latest), then this
 		// is probably a release, so include the tag in the reference.
-		ref = fmt.Sprintf("%s/%s:%s@%s", t.base, t.namer(s), t.tags[0], h)
+		ref = fmt.Sprintf("%s:%s@%s", t.namer(t.base, s), t.tags[0], h)
 	}
 	dig, err := name.NewDigest(ref)
 	if err != nil {


Test patch:
diff --git a/pkg/publish/daemon_test.go b/pkg/publish/daemon_test.go
index 70efdec7..e6b9d223 100644
--- a/pkg/publish/daemon_test.go
+++ b/pkg/publish/daemon_test.go
@@ -57,7 +57,7 @@ func TestDaemon(t *testing.T) {
 	def := NewDaemon(md5Hash, []string{})
 	if d, err := def.Publish(img, importpath); err != nil {
 		t.Errorf("Publish() = %v", err)
-	} else if got, want := d.String(), "ko.local/"+md5Hash(importpath); !strings.HasPrefix(got, want) {
+	} else if got, want := d.String(), md5Hash("ko.local", importpath); !strings.HasPrefix(got, want) {
 		t.Errorf("Publish() = %v, wanted prefix %v", got, want)
 	}
 }
@@ -74,7 +74,7 @@ func TestDaemonTags(t *testing.T) {
 	def := NewDaemon(md5Hash, []string{"v2.0.0", "v1.2.3", "production"})
 	if d, err := def.Publish(img, importpath); err != nil {
 		t.Errorf("Publish() = %v", err)
-	} else if got, want := d.String(), "ko.local/"+md5Hash(importpath); !strings.HasPrefix(got, want) {
+	} else if got, want := d.String(), md5Hash("ko.local", importpath); !strings.HasPrefix(got, want) {
 		t.Errorf("Publish() = %v, wanted prefix %v", got, want)
 	}
 
diff --git a/pkg/publish/default_test.go b/pkg/publish/default_test.go
index 02001990..27749f10 100644
--- a/pkg/publish/default_test.go
+++ b/pkg/publish/default_test.go
@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -66,11 +67,11 @@ func TestDefault(t *testing.T) {
 	}
 }
 
-func md5Hash(s string) string {
+func md5Hash(base, s string) string {
 	// md5 as hex.
 	hasher := md5.New()
 	hasher.Write([]byte(s))
-	return hex.EncodeToString(hasher.Sum(nil))
+	return filepath.Join(base, hex.EncodeToString(hasher.Sum(nil)))
 }
 
 func TestDefaultWithCustomNamer(t *testing.T) {
@@ -100,8 +101,8 @@ func TestDefaultWithCustomNamer(t *testing.T) {
 			t.Errorf("Publish() = %v", err)
 		} else if !strings.HasPrefix(d.String(), repoName) {
 			t.Errorf("Publish() = %v, wanted prefix %v", d, tag.Repository)
-		} else if !strings.HasSuffix(d.Context().String(), md5Hash(strings.ToLower(importpath))) {
-			t.Errorf("Publish() = %v, wanted suffix %v", d.Context(), md5Hash(importpath))
+		} else if !strings.HasSuffix(d.Context().String(), md5Hash("", strings.ToLower(importpath))) {
+			t.Errorf("Publish() = %v, wanted suffix %v", d.Context(), md5Hash("", importpath))
 		}
 	}
 }
@@ -133,7 +134,7 @@ func TestDefaultWithTags(t *testing.T) {
 		} else if !strings.HasPrefix(d.String(), repoName) {
 			t.Errorf("Publish() = %v, wanted prefix %v", d, tag.Repository)
 		} else if !strings.HasSuffix(d.Context().String(), strings.ToLower(importpath)) {
-			t.Errorf("Publish() = %v, wanted suffix %v", d.Context(), md5Hash(importpath))
+			t.Errorf("Publish() = %v, wanted suffix %v", d.Context(), md5Hash("", importpath))
 		}
 
 		otherTag := fmt.Sprintf("%s/%s:v1.2.3", u.Host, expectedRepo)
@@ -214,7 +215,7 @@ func TestDefaultWithReleaseTag(t *testing.T) {
 	} else if !strings.HasPrefix(d.String(), repoName) {
 		t.Errorf("Publish() = %v, wanted prefix %v", d, tag.Repository)
 	} else if !strings.HasSuffix(d.Context().String(), strings.ToLower(importpath)) {
-		t.Errorf("Publish() = %v, wanted suffix %v", d.Context(), md5Hash(importpath))
+		t.Errorf("Publish() = %v, wanted suffix %v", d.Context(), md5Hash("", importpath))
 	} else if !strings.Contains(d.String(), releaseTag) {
 		t.Errorf("Publish() = %v, wanted tag included: %v", d.String(), releaseTag)
 	}
diff --git a/pkg/publish/tarball_test.go b/pkg/publish/tarball_test.go
index 07d7e897..d2d71dd4 100644
--- a/pkg/publish/tarball_test.go
+++ b/pkg/publish/tarball_test.go
@@ -40,7 +40,7 @@ func TestTarball(t *testing.T) {
 	defer fp.Close()
 	defer os.Remove(fp.Name())
 
-	expectedRepo := fmt.Sprintf("%s/%s", base, md5Hash(strings.ToLower(importpath)))
+	expectedRepo := md5Hash(base, strings.ToLower(importpath))
 
 	tag, err := name.NewTag(fmt.Sprintf("%s/%s:latest", "example.com", expectedRepo))
 	if err != nil {