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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7dfc0c6
Handle KO_DOCKER_REPO=ko.local/repo and --bare correctly
imjasonh Sep 12, 2022
271d63f
support ko.local/foo for other namers too
imjasonh Sep 12, 2022
eee9639
more robust defaulting
imjasonh Sep 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion pkg/commands/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,21 @@ 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 {
// Default LocalDomain if unset.
if po.LocalDomain == "" {
Copy link

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.

Copy link

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...

Copy link

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.

Copy link

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.

po.LocalDomain = publish.LocalDomain
}
// If repoName is unset with --local, default it to the local domain.
if po.Local && repoName == "" {
repoName = po.LocalDomain
}
// When in doubt, if repoName is under the local domain, default to --local.
po.Local = strings.HasPrefix(repoName, po.LocalDomain)
if po.Local {
// TODO(jonjohnsonjr): I'm assuming that nobody will
// use local with other publishers, but that might
// not be true.
po.LocalDomain = repoName
return publish.NewDaemon(namer, po.Tags,
publish.WithDockerClient(po.DockerClient),
publish.WithLocalDomain(po.LocalDomain),
Expand Down
9 changes: 9 additions & 0 deletions pkg/commands/resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,15 @@ func TestNewPublisherCanPublish(t *testing.T) {
shouldError: true,
wantError: errImageLoad,
},
{
description: "bare with local domain and repo",
wantImageName: strings.ToLower(fmt.Sprintf("%s/foo", dockerRepo)),
po: &options.PublishOptions{
DockerRepo: dockerRepo + "/foo",
Local: true,
Bare: true,
},
},
}
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
Expand Down