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

Support ko image references #4952

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Support ko image references #4952

merged 1 commit into from
Nov 19, 2020

Conversation

halvards
Copy link
Contributor

Description
ko supports replacing image manifest fields with the image names
and digests of the built images. For this to work, the image field
must contain the import path of the Go binary, prefixed by ko://.
The prefix was optional prior to ko v0.6.0. Starting from v0.6.0,
the prefix is mandatory.

This change allows Skaffold to use manifests containing ko image
references, including those that use the prefix. Skaffold replaces the
image name as expected when using ko via a custom buildCommand.

It also handles Go import paths containing uppercase characters, e.g.
github.com/GoogleContainerTools/skaffold/cmd/skaffold, by lowercasing them.

This means ko users can start using Skaffold without making changes
to their manifests, and it allows these users to use both ko via
Skaffold custom builder, and ko standalone (without Skaffold),
side-by-side.

Related: #1958, #4098

User facing changes
Updates the custom builder ko example to v0.6.0, and to use a ko image reference.

Follow-up Work
First-class support for ko would be great 😄

@halvards halvards requested a review from a team as a code owner October 27, 2020 11:18
@halvards halvards requested a review from gsquared94 October 27, 2020 11:18
@google-cla google-cla bot added the cla: yes label Oct 27, 2020
@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #4952 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4952      +/-   ##
==========================================
- Coverage   72.36%   72.31%   -0.05%     
==========================================
  Files         360      360              
  Lines       12527    12537      +10     
==========================================
+ Hits         9065     9066       +1     
- Misses       2800     2809       +9     
  Partials      662      662              
Impacted Files Coverage Δ
pkg/skaffold/docker/default_repo.go 100.00% <100.00%> (ø)
pkg/skaffold/docker/reference.go 100.00% <100.00%> (ø)
pkg/skaffold/kubernetes/manifest/images.go 100.00% <100.00%> (ø)
pkg/skaffold/deploy/kpt/kpt.go 79.51% <0.00%> (-4.24%) ⬇️

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 b866a05...83a4d47. Read the comment docs.

`ko` supports replacing `image` manifest fields with the image names
and digests of the built images. For this to work, the `image` field
must contain the import path of the Go binary, prefixed by `ko://`.
The prefix was optional prior to ko v0.6.0. Starting from v0.6.0,
[the prefix is mandatory](https://github.com/google/ko/releases/tag/v0.6.0).

This change allows Skaffold to use manifests containing ko image
references, including those that use the prefix. Skaffold replaces the
image name as expected when using ko via a custom `buildCommand`.

It also handles Go import paths containing uppercase characters, e.g.
`github.com/GoogleContainerTools/skaffold`, by lowercasing them.

This means `ko` users can start using Skaffold without making changes
to their manifests, and it allows these users to use both ko via
Skaffold custom builder, and ko standalone (without Skaffold),
side-by-side.
@MarlonGamez
Copy link
Contributor

MarlonGamez commented Oct 27, 2020

Hi @halvards , thanks for opening this PR! These changes look good to me :) Thanks for updating the example to match the changes. I'll run kokoro for this and if everything is good I think this should be good to go in.

Do you have ideas for what first class support for ko in skaffold would look like?

@briandealwis
Copy link
Member

@MarlonGamez I think we should hold off until @nkubala is back. It seems odd to embed specific tool strings that we don't directly support.

@MarlonGamez
Copy link
Contributor

@briandealwis yeah, that makes sense. I'll label it as do-not-merge for now then

@tejal29
Copy link
Contributor

tejal29 commented Nov 9, 2020

@nkubala would you be able to take a look at this.

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

sorry for the late reply on this one, talked with @jonjohnsonjr just to understand more of what's going on here and this seems fine. obviously it's not ideal to be hardcoding strings in like this, but because of the way we parse image references in skaffold our hands are sort of tied. ko treats github paths as image references, and even though they're technically not, they parse correctly with docker.ParseReference. the ko:// prefix unfortunately causes this to choke, so short of reworking our image reference parsing this is the best option.

because of the way ko's watcher works, first class support in skaffold is a whole different problem entirely :) it's an awesome way to build go apps though, so even though we don't "natively" support it in skaffold i think it's fine to merge this fix in the interim.

@nkubala nkubala merged commit 45585e0 into GoogleContainerTools:master Nov 19, 2020
@halvards halvards deleted the support-ko-prefix branch November 19, 2020 20:22
@halvards
Copy link
Contributor Author

Thanks all, and yes sorry for not providing more context up front on the import path/image reference parsing issue!

On the topic of watching, I was thinking that any further integration of ko into skaffold could use skaffold's watch implementation, rather than the one in ko? Maybe a discussion for another thread.

@nkubala
Copy link
Contributor

nkubala commented Dec 2, 2020

@halvards definitely a discussion worth having, but this thread is probably too low visibility to get it any traction :) feel free to open another issue here or on the ko repo to further discuss

halvards added a commit to halvards/skaffold that referenced this pull request Oct 5, 2021
This change ensures that image names from `skaffold.yaml` are sanitized
even when the Skaffold default repo is not set.

This is relevant for ko images with names that consist of the `ko://`
scheme prefixed, followed by a Go import path that may contain uppercase
characters, e.g.,
`ko://github.com/GoogleContainerTools/skaffold/cmd/skaffold`.

Fixes: GoogleContainerTools#6675
Tracking: GoogleContainerTools#6041
Related: GoogleContainerTools#4952
tejal29 pushed a commit that referenced this pull request Oct 5, 2021
This change ensures that image names from `skaffold.yaml` are sanitized
even when the Skaffold default repo is not set.

This is relevant for ko images with names that consist of the `ko://`
scheme prefixed, followed by a Go import path that may contain uppercase
characters, e.g.,
`ko://github.com/GoogleContainerTools/skaffold/cmd/skaffold`.

Fixes: #6675
Tracking: #6041
Related: #4952
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants