Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle KO_DOCKER_REPO=ko.local/repo and --bare correctly #820

Merged
merged 3 commits into from
Oct 19, 2022

Conversation

imjasonh
Copy link
Member

Fixes #807

cc @developer-guy

See comments in #807 (comment) -- uniformly setting base: po.LocalDomain in NewDaemon was fine when we assumed/required that KO_DOCKER_REPO=ko.local would only be used without a repo, but we've since changed that assumption/requirement.

$ KO_DOCKER_REPO=ko.local/foo go run ./ build ./test --bare
...
ko.local/foo:3a28632fcb886b7a4c78be8cd36c34ec47e83f71ff55353e5602873400dca564

Note that this also handles ko.local/foo for other namers:

$ KO_DOCKER_REPO=ko.local/foo go run ./ build ./test --preserve-import-paths
...
ko.local/foo/github.com/google/ko/test:3a28632fcb886b7a4c78be8cd36c34ec47e83f71ff55353e5602873400dca564
KO_DOCKER_REPO=ko.local/foo go run ./ build ./test
...
ko.local/foo/test-46c4b272b3716c422d5ff6dfc7547fa9:3a28632fcb886b7a4c78be8cd36c34ec47e83f71ff55353e5602873400dca564

@developer-guy
Copy link
Collaborator

^ @eddycharly

@eddycharly
Copy link

🥳

@@ -168,10 +168,16 @@ func makePublisher(po *options.PublishOptions) (publish.Interface, error) {
innerPublisher, err := func() (publish.Interface, error) {
repoName := po.DockerRepo
namer := options.MakeNamer(po)
if strings.HasPrefix(repoName, publish.LocalDomain) || po.Local {
if po.LocalDomain == "" {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you bake this logic in the namer instead ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, at least not without changing how namers are specified today.

Namers take in the base and importpath, and use them to construct the final image. If they're only passed publish.LocalDomain and the importpath github.com/foo/bar, they don't have the information necessary to construct ko.local/foo/github.com/foo/bar -- they don't know about the first foo component.

We could change the interface so they take a third argument, which we'd append together in the end, but then we'd just have to parse it out of KO_DOCKER_REPO anyway to get it, so we might as well pass it directly through and append them both together and not change the interface.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about something like this:


func bareDockerRepo(po *PublishOptions) func(string, string) string {
	return func(base, _ string) string {
		if po.LocalDomain != "" {
			return po.LocalDomain
		}
		return base
	}
}

func MakeNamer(po *PublishOptions) publish.Namer {
	if po.ImageNamer != nil {
		return po.ImageNamer
	} else if po.PreserveImportPaths {
		return preserveImportPath
	} else if po.BaseImportPaths {
		return baseImportPaths
	} else if po.Bare {
		return bareDockerRepo(po)
	}
	return packageWithMD5
}

Not the real logic but you get the idea, bareDockerRepo acts as a namer factory taking PublishOptions arg...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I might be completely wrong, first time I look at the code ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that works too. That's more or less how I meant changing the interface. We could do that for all the namers, or we could just pass through the actual local domain that was specified, instead of only the ko.local portion.

As part of this we'll also want to support cases where the local domain is configurable by clients using ko as a library, so that if they configure the local domain to be skaffold.local or whatever, then skaffold.local/foo will also work as expected. I don't know if that's something they plan to support, but it would be nice to be consistent.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the main point is to have the naming logic written at a single place so that it can be reused in different parts of the code that need it (I had the feeling it was the responsibility of the namer).

Anyway, it can be changed later if necessary.

@imjasonh imjasonh merged commit e03e444 into ko-build:main Oct 19, 2022
@developer-guy
Copy link
Collaborator

it would be good to release ko v0.13.0 that includes that fix ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we combine ko.local and --bare ?
4 participants