Enable embedding of ko publish #348

halvards · 2021-04-28T13:12:03Z

Enable embedding of ko's publish functionality to be in other tools.

Export MakeBuilder(), MakePublisher() and PublishImages() from pkg/commands
Allow overriding UserAgent()
Add DockerRepo flag to PublishOptions in pkg/commands/options

codecov-commenter · 2021-04-28T13:15:39Z

Codecov Report

Merging #348 (392d767) into main (d5ec166) will increase coverage by 5.36%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##             main     #348      +/-   ##
==========================================
+ Coverage   38.27%   43.64%   +5.36%     
==========================================
  Files          33       33              
  Lines        1523     1558      +35     
==========================================
+ Hits          583      680      +97     
+ Misses        846      753      -93     
- Partials       94      125      +31

Impacted Files	Coverage Δ
pkg/commands/resolver.go	`29.47% <55.55%> (+17.80%)`	⬆️
pkg/commands/config.go	`56.52% <100.00%> (+40.57%)`	⬆️
pkg/commands/publisher.go	`53.84% <100.00%> (+53.84%)`	⬆️
pkg/publish/default.go	`61.84% <0.00%> (-2.78%)`	⬇️
pkg/publish/options.go	`28.12% <0.00%> (+7.43%)`	⬆️
pkg/publish/daemon.go	`37.70% <0.00%> (+8.29%)`	⬆️
pkg/commands/version.go	`26.66% <0.00%> (+26.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5ec166...392d767. Read the comment docs.

imjasonh

Thanks for proposing this change! I'm excited to see ko being used and adopted in more contexts and tools. 👍

imjasonh · 2021-04-28T13:20:36Z

pkg/commands/options/publish.go

@@ -25,6 +25,9 @@ import (

 // PublishOptions encapsulates options when publishing.
 type PublishOptions struct {
+	// DockerRepo overrides the KO_DOCKER_REPO environment variable, if present
+	DockerRepo string


I'd like to push back on this change, or at least consider it in the larger goal of unifying ko's configuration options.

Today we have .ko.yaml, env vars to override .ko.yaml provided by viper, env vars for KO_DOCKER_REPO, flags, etc., and I think all in all we don't have a cohesive story for how people should configure the tool.

This one in particular raises alarms because KO_DOCKER_REPO is necessary to use ko today, and historically we chose to configure it with an env var because we expect each developer to push to their own registry location. Making this overrideable with a flag might encourage teams/tools to document setting this for all calls, which isn't bad necessarily, it's just a departure I want to make sure we're making intentionally.

In the meantime, is it possible to extract this change from this PR?

I have similar feelings. I'd like to formalize the differences between project config, user config, and invocation-specific config a bit better, and this goes in the opposite direction (I think).

I'd be happy to consider alternatives here, and I agree that formalizing the different config types in ko would be helpful.

Currently, makePublisher() grabs the envvar directly, but this feels like depending too much on the execution context in the core implementation logic. I'd expect something that's closer to the command-line logic to have parsed the config option at this point.

Do you have any alternative recommendations on how to allow the repo to be set programmatically?

For some context, Skaffold has a similar 'default repo' concept, which can be set as a flag, envvar, or in global config: https://skaffold.dev/docs/environment/image-registries/

In the meantime, I'll extract a separate PR for this change.

New PR for programmatically setting the destination image repo: #351

imjasonh · 2021-04-28T13:22:22Z

pkg/commands/publisher.go

@@ -41,6 +41,11 @@ func qualifyLocalImport(importpath string) (string, error) {
 	return pkgs[0].PkgPath, nil
 }

+// PublishImages publishes images
+func PublishImages(ctx context.Context, importpaths []string, pub publish.Interface, b build.Interface) (map[string]name.Reference, error) {


Is there a reason to prefer exporting this new wrapper method, versus exporting publishImages directly?

I think it makes the diff smaller, which is nice. I kind of like it :P

I'd be okay with it as a specific diff-shrinking measure that we intend to clean up with a followup PR.

Yes, it was just to keep the diff small.

imjasonh · 2021-04-28T13:22:51Z

pkg/commands/resolver.go

@@ -127,11 +138,19 @@ func makeBuilder(ctx context.Context, bo *options.BuildOptions) (*build.Caching,
 	return build.NewCaching(innerBuilder)
 }

+// MakePublisher creates a ko publisher
+func MakePublisher(po *options.PublishOptions) (publish.Interface, error) {
+	return makePublisher(po)


Same comment about makePublisher vs MakePublisher, makeBuilder, etc. -- if we're going to export them, I'd prefer to just export them.

As above. I was thinking of an expand-and-contract style of approach (https://www.martinfowler.com/bliki/ParallelChange.html).

imjasonh · 2021-04-28T13:23:43Z

pkg/commands/resolver.go

@@ -204,6 +223,7 @@ type nopPublisher struct {
 }

 func (n nopPublisher) Publish(_ context.Context, br build.Result, s string) (name.Reference, error) {
+	s = strings.TrimPrefix(s, build.StrictScheme)


Why is this change necessary?

Ah, that was unrelated - the nopPublisher failed in a unit test because the scheme prefix wasn't trimmed.

This enables programmatically setting the destination image repository when embedding ko's `publish` functionality in other tools. See ko-build#348

* Add flag and PublishOption for destination repo This enables programmatically setting the destination image repository when embedding ko's `publish` functionality in other tools. See #348 * Set DockerRepo PublishOption from KO_DOCKER_REPO This enables programmatically setting the destination image repository and avoids exposing a flag. * Update comment on DockerRepo option * Fix readme and copyright headers

pkg/commands/config.go

pkg/commands/publisher.go

pkg/commands/resolver_test.go

jonjohnsonjr · 2021-05-14T15:52:20Z

pkg/commands/config.go

@@ -39,6 +41,16 @@ var (
 	baseImageOverrides map[string]name.Reference
 )

+// SetDefaultBaseImage enables programmatically overriding the base image
+func SetDefaultBaseImage(baseImage string) error {


I don't love this being a package global. Can we add this to the BuildOptions and and plumb it down through to https://github.com/google/ko/blob/1b023d30207f385020a5829deec7c147719a1ff9/pkg/commands/resolver.go#L82?

It's a private function, so we can just add another parameter to getBaseImage and pass in the defaultBaseImage

Good point. I removed the exported function and added BaseDir to BuildOptions.

I also found that we could set the variable without modifying the getBaseImage function signature, but please let me know if you'd rather add a parameter.

On a related note, I was thinking to make UserAgent an overrideable option, but it's used by both Builder (to fetch the base image) and by Publisher (to push to the registry). We could have two different user-agent options, or introduce some shared build/publish option.

please let me know if you'd rather add a parameter

I think I'd prefer as a parameter, because this is still mutating package-global state. If you call MakeBuilder twice from the same program, they would interfere with each other.

On that note, we might want to rename MakeBuilder to NewBuilder? I think that's more in line with idiomatic constructor names in go?

We could have two different user-agent options, or introduce some shared build/publish option.

I'm fine with whatever's easiest -- if I were implementing this I'd probably just add a second publisher option, but I'm curious about your opinion as a consumer. I know it's kind of gross to have to specify that twice.

Great point, I've changed it to a parameter.

I've also added UserAgent fields to both BuildOptions and PublishOptions. Having to specify them both programmatically didn't seem like a big deal.

halvards · 2021-05-21T08:51:13Z

Was there a problem with GitHub Actions? A log of the checks failed, but all I see is "This check failed", and no output.

jonjohnsonjr · 2021-05-21T19:40:23Z

Something strange is happening, you might need to rebase

jonjohnsonjr · 2021-05-24T17:01:56Z

pkg/commands/resolver.go

+	if bo.BaseImage != "" {
+		baseImageRef, err := name.ParseReference(bo.BaseImage)
+		if err != nil {
+			return nil, fmt.Errorf("'gobuildOptions': error parsing %q as image reference: %v", bo.BaseImage, err)
+		}
+		defaultBaseImage = baseImageRef
+	}


Suggested change

if bo.BaseImage != "" {

baseImageRef, err := name.ParseReference(bo.BaseImage)

if err != nil {

return nil, fmt.Errorf("'gobuildOptions': error parsing %q as image reference: %v", bo.BaseImage, err)

}

defaultBaseImage = baseImageRef

}

Fixed. Apologies, missed this in the rebase.

- Export functions and a variable to enable embedding of ko's `publish` functionality to be embedded in other tools. See GoogleContainerTools/skaffold#5611 - Remove DockerRepo PublishOption and flag. This removes the `DockerRepo` config option and `--docker-repo` flag from the PR. New PR with the extracted config option: ko-build#351 - Fix copyright headers for boilerplate check. - Use DockerRepo PublishOption instead of env var. - Override defaultBaseImage using BuildOptions. Remove exported package global SetDefaultBaseImage and instead allow programmatic override of the default base image using the field `BaseImage` in `options.BuildOptions`. Also fix copyright header years. - Add BuildOptions parameter to getBaseImage This enables access to BaseImage for programmatically overriding the default base image from `.ko.yaml`. - Add UserAgent to BuildOptions and PublishOptions This enables programmatically overriding the `User-Agent` HTTP request header for both pulling the base image and pushing the built image. - Rename MakeBuilder to NewBuilder and MakePublisher to NewPublisher. For more idiomatic constructor function names.

jonjohnsonjr

lgtm

@imjasonh any final objections?

halvards · 2021-05-26T09:43:27Z

Thanks for all the help with this!

Please let me know if you'd like me to remove the indirection of the NewBuilder()/makeBuilder(), NewPublisher()/makePublisher() and PublishImages()/publishImages() functions.

google-cla bot added the cla: yes label Apr 28, 2021

imjasonh reviewed Apr 28, 2021

View reviewed changes

halvards added a commit to halvards/ko that referenced this pull request Apr 29, 2021

Add flag and PublishOption for destination repo

258ac25

This enables programmatically setting the destination image repository when embedding ko's `publish` functionality in other tools. See ko-build#348

halvards added a commit to halvards/ko that referenced this pull request Apr 29, 2021

Add flag and PublishOption for destination repo

02a1511

This enables programmatically setting the destination image repository when embedding ko's `publish` functionality in other tools. See ko-build#348

halvards mentioned this pull request Apr 29, 2021

Add flag and PublishOption for destination repo #351

Merged

jonjohnsonjr reviewed May 14, 2021

View reviewed changes

halvards force-pushed the export-publish-funcs branch from b5536ec to b0bdca1 Compare May 24, 2021 08:43

jonjohnsonjr reviewed May 24, 2021

View reviewed changes

halvards force-pushed the export-publish-funcs branch from b0bdca1 to a27762f Compare May 24, 2021 22:36

jonjohnsonjr approved these changes May 25, 2021

View reviewed changes

imjasonh approved these changes May 25, 2021

View reviewed changes

jonjohnsonjr merged commit d6b3a3c into ko-build:main May 25, 2021

halvards deleted the export-publish-funcs branch May 26, 2021 09:41

halvards mentioned this pull request Jun 18, 2021

Add ko builder GoogleContainerTools/skaffold#6041

Closed

imjasonh mentioned this pull request Oct 5, 2021

[Feature Request]: add google/ko as brand new builder engine for building and pushing container images goreleaser/goreleaser#2556

Closed

imjasonh mentioned this pull request Dec 17, 2021

Document support/expectations for consuming ko as a library #554

Closed

halvards mentioned this pull request Dec 22, 2021

Introduce log levels #560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable embedding of ko publish #348

Enable embedding of ko publish #348

halvards commented Apr 28, 2021

codecov-commenter commented Apr 28, 2021 •

edited

Loading

imjasonh left a comment

imjasonh Apr 28, 2021

jonjohnsonjr Apr 28, 2021

halvards Apr 29, 2021

halvards Apr 29, 2021

imjasonh Apr 28, 2021

jonjohnsonjr Apr 28, 2021

imjasonh Apr 28, 2021

halvards Apr 28, 2021

imjasonh Apr 28, 2021

halvards Apr 29, 2021

imjasonh Apr 28, 2021

halvards Apr 28, 2021

jonjohnsonjr May 14, 2021

halvards May 18, 2021

jonjohnsonjr May 18, 2021

halvards May 19, 2021

halvards commented May 21, 2021

jonjohnsonjr commented May 21, 2021

jonjohnsonjr May 24, 2021

halvards May 24, 2021

jonjohnsonjr left a comment

halvards commented May 26, 2021

Enable embedding of ko publish #348

Enable embedding of ko publish #348

Conversation

halvards commented Apr 28, 2021

codecov-commenter commented Apr 28, 2021 • edited Loading

Codecov Report

imjasonh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halvards commented May 21, 2021

jonjohnsonjr commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjohnsonjr left a comment

Choose a reason for hiding this comment

halvards commented May 26, 2021

codecov-commenter commented Apr 28, 2021 •

edited

Loading