Add ko builder design proposal draft #5611

halvards · 2021-03-29T12:25:23Z

No description provided.

codecov · 2021-03-29T12:33:42Z

Codecov Report

Merging #5611 (1e46d57) into master (e947844) will decrease coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5611      +/-   ##
==========================================
- Coverage   70.64%   70.52%   -0.13%     
==========================================
  Files         411      413       +2     
  Lines       15860    15982     +122     
==========================================
+ Hits        11205    11271      +66     
- Misses       3827     3880      +53     
- Partials      828      831       +3

Impacted Files	Coverage Δ
cmd/skaffold/app/flags/image.go	`72.97% <0.00%> (-11.03%)`	⬇️
pkg/skaffold/util/tar.go	`50.66% <0.00%> (-5.34%)`	⬇️
pkg/skaffold/build/scheduler.go	`92.98% <0.00%> (-1.36%)`	⬇️
cmd/skaffold/app/cmd/flags.go	`89.02% <0.00%> (ø)`
pkg/skaffold/config/options.go	`100.00% <0.00%> (ø)`
pkg/skaffold/schema/versions.go	`82.41% <0.00%> (ø)`
pkg/skaffold/schema/latest/config.go	`58.82% <0.00%> (ø)`
pkg/skaffold/schema/v2beta14/config.go	`20.58% <0.00%> (ø)`
pkg/skaffold/schema/v2beta14/upgrade.go	`100.00% <0.00%> (ø)`
pkg/skaffold/runner/new.go	`62.77% <0.00%> (+0.20%)`	⬆️
... and 3 more

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 e947844...1e46d57. Read the comment docs.

briandealwis

Some quick comments

briandealwis · 2021-03-29T15:38:44Z

docs/design_proposals/ko-builder.md

+- _share with other developers_: no additional tools are required to
+  `skaffold run` with the ko builder, not even Docker.


Maybe add a placeholder here to note that it may depend on ko if we don't embed ko.

briandealwis · 2021-03-29T15:40:03Z

docs/design_proposals/ko-builder.md

+    ```go
+    type KoArtifact struct {
+    	// Flags are additional build flags passed to the builder.
+    	// For example: `["--platform=linux/amd64,linux/arm64"]`.


Could you choose a different example? Although users will need this for now, we'll handle platforms separately in the future.

Typically we've been exposing key config flags as explicit keys on the artifact. The docker artifact, for example, exposes cacheFrom (--cache-from), network (--network). From a quick glance, it seems to me that we'd want to support specifying an alternative .ko.yaml (KO_CONFIG_PATH), GOLDFLAGS?
And possibly SOURCE_DATE_EPOCH?

Good point. I've modified this struct to include the configuration options.

A config option I left out is disable-optimizations. This is useful when debugging, and the implementation can set this to true when the user runs skaffold debug. I don't know yet if there are scenarios where users would want to set this to true even if not in debug mode.

briandealwis · 2021-03-29T15:42:45Z

docs/design_proposals/ko-builder.md

+    	// BaseImage overrides the default ko base image.
+    	// Corresponds to, and overrides, the `defaultBaseImage` in `.ko.yaml`.
+    	BaseImage string `yaml:"fromImage,omitempty"`


Exposing this as a separate field is great 👍

briandealwis · 2021-03-29T15:44:53Z

docs/design_proposals/ko-builder.md

+    - It would allow Skaffold to support a range of ko versions. On the other
+      hand, these versions would need to be tracked and documented.


Would we need to provide a version field to allow a user to choose between them?

If Skaffold doesn't embed ko, then the ko version used would be whichever is first in the user's PATH I think?

briandealwis · 2021-03-29T15:50:04Z

docs/design_proposals/ko-builder.md

+    Embedding ko would require some stability guarantees for the most important
+    interfaces that Skaffold would use:


I don't think API stability matters as much as documented behavioural stability.

We embed Cloud Native Buildpacks' pack as a library, and they occasionally make big changes, but provide backward and forward compatibility for at least a couple of versions. So embedding will not break users using a buildpacks builder that has updated to the newer version of pack before we've updated our embedding.

This seems to be less of a concern with ko since there does not appear to be any independently-released dependencies.

Good point. When I think of interface stability, I include the behavior and not just the parameters. I've added your comment on behavioral stability to make it clearer.

@briandealwis

Updates based on feedback from @briandealwis

nkubala · 2021-03-30T17:25:38Z

docs/design_proposals/ko-builder.md

+    	// Specify as the number of seconds since January 1st 1970, 00:00 UTC.
+    	// You can override this value by setting the `SOURCE_DATE_EPOCH`
+    	// environment variable.
+    	SourceDateEpoch unit64 `yaml:"sourceDateEpoch,omitempty"`


would most users want this to be time.Now?

I'd expect most would leave it at 0, for reproducible builds. That's the ko default.

The proposal doesn't actually touch on reproducible builds yet, but we could elaborate on this. ko makes reproducible builds possible, for users who care about that. It requires a reproducible build environment though (consistent paths, GOROOT, etc) - until we add the ability to pass the go build flags required to remove paths from the binary.

Just a nit: we can drop this field from config and rely on plumbing through SOURCE_DATE_EPOCH env variable for reproducible builds to the build process.
We can set it to time.Now when the env variable is not set.

I'd suggest leaving the default to 0. This is the ko default, and I see both Knative and Tekton uses this. It'd be confusing if ko behaved differently when used via Skaffold.

Your point of plumbing through envvars is good. Is there an established pattern in Skaffold for whether config in skaffold.yaml takes precedence over external builder config (e.g., Jib config in pom.xml/build.gradle)?

sounds good.

nkubala · 2021-03-30T17:31:34Z

docs/design_proposals/ko-builder.md

+
+1.  Should Skaffold embed ko (as a Go module), or shell out?
+
+    Benefits of embedding:


one more benefit: it doesn't require users to install/know anything about ko to adopt it. this makes it easier for users to try it out simply by changing around some config in their skaffold.yaml, rather than installing a new tool entirely.

It would be great to do it under the hood by prompting the user without actually them changing the artifact type. We can easily do it for IDE users. For CLI users, we can show this as a tip in skaffold diagnose

@tejal29 do you mean as part of skaffold init?

docs/design_proposals/ko-builder.md

tejal29 · 2021-03-31T05:17:54Z

docs/design_proposals/ko-builder.md

+    Embedding ko would require some level of documented behavioural stability
+    guarantees for the most ko interfaces that Skaffold would use, such as
+    [`build.Interface`](https://github.com/google/ko/blob/82cabb40bae577ce3bc016e5939fd85889538e8b/pkg/build/build.go#L24)
+    and
+    [`publish.Interface`](https://github.com/google/ko/blob/82cabb40bae577ce3bc016e5939fd85889538e8b/pkg/publish/publish.go#L24),
+    or others?


Wouldn't there be some code duplication across skaffold and ko repos to create the publisher and builder object. All of this https://github.com/google/ko/blob/82cabb40bae577ce3bc016e5939fd85889538e8b/pkg/commands/publish.go#L62

Agree, there would be. These seemed like the most appropriate existing interfaces. We should look into defining something at a slightly higher level in ko to avoid the duplication.

Would you take up that work too ?

Yes, I'll coordinate with the ko team and create the necessary PRs to do that.

tejal29 · 2021-03-31T05:25:04Z

docs/design_proposals/ko-builder.md

+    	// Specify as the number of seconds since January 1st 1970, 00:00 UTC.
+    	// You can override this value by setting the `SOURCE_DATE_EPOCH`
+    	// environment variable.
+    	SourceDateEpoch unit64 `yaml:"sourceDateEpoch,omitempty"`


Just a nit: we can drop this field from config and rely on plumbing through SOURCE_DATE_EPOCH env variable for reproducible builds to the build process.
We can set it to time.Now when the env variable is not set.

docs/design_proposals/ko-builder.md

tejal29 · 2021-03-31T05:31:58Z

docs/design_proposals/ko-builder.md

+
+Please describe what new test cases you are going to consider.
+
+1.  Unit and integration tests for ko builder, similar to other builders.


cross project integration tests to make sure changes to ko publish or any used Interface does not break skaffold

docs/design_proposals/ko-builder.md

tejal29 · 2021-03-31T05:54:03Z

docs/design_proposals/ko-builder.md

+
+1.  Should Skaffold embed ko (as a Go module), or shell out?
+
+    Benefits of embedding:


It would be great to do it under the hood by prompting the user without actually them changing the artifact type. We can easily do it for IDE users. For CLI users, we can show this as a tip in skaffold diagnose

docs/design_proposals/ko-builder.md

tejal29

LGTM after closing all open issues.

briandealwis · 2021-04-21T22:17:13Z

docs/design_proposals/ko-builder.md

+[default repo](https://skaffold.dev/docs/environment/image-registries/)
+maps directly to this value.
+
+### Open questions


How can skaffold debug identify Ko images as being Go? Is there an image label or environment variable set?

Can skaffold debug look at the artifact type in skaffold.yaml?

If not, I'd suggest looking for the KO_DATA_PATH environment variable, which is always set.

Other options:

look at the entrypoint, it's always /ko-app/ko

check if PATH contains /ko-app

look at the author field of the image layer at the top of the stack (in the image configuration), the value will be ko.

Export functions and a variable to enable embedding of ko's `publish` functionality to be embedded in other tools. See GoogleContainerTools/skaffold#5611

tejal29 · 2021-05-10T23:21:08Z

@halvards Please merge this when you get a chance.

halvards · 2021-05-10T23:48:52Z

I don't have write access to the repo, so someone else will have to merge for me.

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

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

Add ko builder design proposal first draft

d7e2c4b

halvards requested a review from a team as a code owner March 29, 2021 12:25

pull-request-size bot added the size/L label Mar 29, 2021

google-cla bot added the cla: yes label Mar 29, 2021

briandealwis reviewed Mar 29, 2021

View reviewed changes

Update ko builder proposal based on feedback

ca60ac6

Updates based on feedback from @briandealwis

nkubala reviewed Mar 30, 2021

View reviewed changes

tejal29 reviewed Mar 31, 2021

View reviewed changes

halvards added 6 commits April 9, 2021 19:26

Add more comprehensive ko builder config example

7c138b6

Add implementation plan details

ade91ee

Merge remote-tracking branch 'upstream/master'

3403963

Add initializer rank to ko builder proposal

958b971

Ko builder proposal: add open questions

1eed482

Ko builder proposal: Clarify proposed init behavior

1e46d57

tejal29 approved these changes Apr 21, 2021

View reviewed changes

briandealwis reviewed Apr 21, 2021

View reviewed changes

MarlonGamez mentioned this pull request Apr 26, 2021

Investigate why SanitizeImageName isn't being called #5505

Closed

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

Enable embedding of ko publish

339183b

Export functions and a variable to enable embedding of ko's `publish` functionality to be embedded in other tools. See GoogleContainerTools/skaffold#5611

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

Enable embedding of ko publish

22bc056

Export functions and a variable to enable embedding of ko's `publish` functionality to be embedded in other tools. See GoogleContainerTools/skaffold#5611

halvards mentioned this pull request Apr 28, 2021

Enable embedding of ko publish ko-build/ko#348

Merged

tejal29 enabled auto-merge (squash) May 11, 2021 06:14

tejal29 disabled auto-merge May 11, 2021 06:14

tejal29 merged commit 8d05e94 into GoogleContainerTools:master May 11, 2021

halvards mentioned this pull request Jun 18, 2021

Add ko builder #6041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ko builder design proposal draft #5611

Add ko builder design proposal draft #5611

halvards commented Mar 29, 2021

codecov bot commented Mar 29, 2021 •

edited

Loading

briandealwis left a comment

briandealwis Mar 29, 2021

halvards Mar 30, 2021

briandealwis Mar 29, 2021

halvards Mar 30, 2021

briandealwis Mar 29, 2021

briandealwis Mar 29, 2021

halvards Mar 30, 2021

briandealwis Mar 29, 2021

halvards Mar 30, 2021

nkubala Mar 30, 2021

halvards Mar 31, 2021

tejal29 Mar 31, 2021

halvards Apr 9, 2021 •

edited

Loading

tejal29 Apr 21, 2021

nkubala Mar 30, 2021

tejal29 Mar 31, 2021

halvards Apr 21, 2021

tejal29 Mar 31, 2021

halvards Apr 9, 2021

tejal29 Apr 21, 2021

halvards Apr 21, 2021

tejal29 Mar 31, 2021

tejal29 Mar 31, 2021

tejal29 Mar 31, 2021

tejal29 left a comment

briandealwis Apr 21, 2021

halvards Apr 21, 2021

tejal29 commented May 10, 2021

halvards commented May 10, 2021

		- _share with other developers_: no additional tools are required to
		`skaffold run` with the ko builder, not even Docker.

		- It would allow Skaffold to support a range of ko versions. On the other
		hand, these versions would need to be tracked and documented.

		Embedding ko would require some stability guarantees for the most important
		interfaces that Skaffold would use:


		1. Should Skaffold embed ko (as a Go module), or shell out?

		Benefits of embedding:


		Please describe what new test cases you are going to consider.

		1. Unit and integration tests for ko builder, similar to other builders.

Add ko builder design proposal draft #5611

Add ko builder design proposal draft #5611

Conversation

halvards commented Mar 29, 2021

codecov bot commented Mar 29, 2021 • edited Loading

Codecov Report

briandealwis 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

halvards Apr 9, 2021 • edited Loading

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

tejal29 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejal29 commented May 10, 2021

halvards commented May 10, 2021

codecov bot commented Mar 29, 2021 •

edited

Loading

halvards Apr 9, 2021 •

edited

Loading