Skaffold render solely perform the manifests hydration #4193

ChrisGe4 · 2020-05-15T19:02:38Z

Description
Proposal: go/skaffold-render-hydration-only

Currently skaffold render performs all image builds then outputs hydrated Kubernetes manifests. This PR modifies skaffold render to solely perform the hydration of Kubernetes manifests with the option to build artifacts.

In this PR, a new option --skip-build will be added to the render command. skaffold render --skip-build only hydrated the kubernetes manifests without building the image.

pull from upstream

merge upstream

Merge upstream changes.

codecov · 2020-05-15T19:35:50Z

Codecov Report

Merging #4193 into master will decrease coverage by 0.76%.
The diff coverage is 57.57%.

Impacted Files	Coverage Δ
pkg/skaffold/config/options.go	`100.00% <ø> (ø)`
pkg/skaffold/runner/render.go	`0.00% <0.00%> (ø)`
pkg/skaffold/runner/runner.go	`0.00% <ø> (ø)`
pkg/skaffold/deploy/util.go	`63.88% <50.00%> (-1.74%)`	⬇️
pkg/skaffold/deploy/kubectl.go	`67.64% <81.81%> (+1.24%)`	⬆️
cmd/skaffold/app/cmd/render.go	`47.61% <100.00%> (+5.51%)`	⬆️
pkg/skaffold/runner/build_deploy.go	`71.91% <100.00%> (+3.09%)`	⬆️
pkg/skaffold/deploy/resource/deployment.go	`76.84% <0.00%> (-16.15%)`	⬇️
pkg/skaffold/deploy/resource/status.go	`90.00% <0.00%> (-10.00%)`	⬇️
pkg/skaffold/server/server.go	`48.88% <0.00%> (-4.77%)`	⬇️
... and 19 more

pkg/skaffold/runner/build_deploy.go

balopat

Added my comments - I don't think we should infer the image tags and assume that that's what the user wants.

merge upstream

brandonnelson3 · 2020-06-04T18:04:13Z

pkg/skaffold/runner/build_deploy.go

@@ -39,8 +39,8 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 		return nil, err
 	}

-	// In dry-run mode, we don't build anything, just return the tag for each artifact.
-	if r.runCtx.Opts.DryRun {
+	// In dry-run mode or --digest-source  set to 'remote' or 'none', we don't build anything, just return the tag for each artifact.


Nit: two spaces after --digest-source

nkubala

I made a few wording suggestions, specifically related to saying that when --digest-source=none, "image tags are used from the skaffold.yaml", which isn't actually true - they're used from the Kubernetes manifests, which is an important distinction.

I also ran into a bug with --digest-source=none:

use the getting-started example in our repo (with skaffold-example as the image in the skaffold.yaml and k8s-pod.yaml
append a :latest to the image in the k8s manifest
set your default-repo to something in skaffold (skaffold config set default-repo gcr.io/my-project)
skaffold render --digest-source=none should give me a manifest with something like

apiVersion: v1
kind: Pod
...
spec:
  containers:
  - image: gcr.io/my-project/skaffold-example:latest
    name: getting-started

but for the image name, I actually get gcr.io/my-project/skaffold-example:v1.10.0-82-gb616768d3-dirty, so it generated me a tag. appending the repository name to the image in the k8s manifest, however, gives me the correct result.

I'm not sure how deep this bug goes, so idk if it should be fixed in this PR or a follow-up. but --digest-source=none will be broken for anyone using the default-repo functionality in skaffold until it's fixed (which I suspect is a sizable number of users).

nkubala · 2020-06-09T20:01:36Z

pkg/skaffold/runner/build_deploy.go

@@ -39,8 +39,8 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 		return nil, err
 	}

-	// In dry-run mode, we don't build anything, just return the tag for each artifact.
-	if r.runCtx.Opts.DryRun {
+	// In dry-run mode or --digest-source  set to 'remote' or 'none', we don't build anything, just return the tag for each artifact.


Suggested change

// In dry-run mode or --digest-source set to 'remote' or 'none', we don't build anything, just return the tag for each artifact.

// In dry-run mode or with --digest-source set to 'remote' or 'none', we don't build anything, just return the tag for each artifact.

nkubala · 2020-06-09T20:01:44Z

cmd/skaffold/app/cmd/render.go

@@ -38,10 +38,12 @@ var (
 func NewCmdRender() *cobra.Command {
 	return NewCmd("render").
 		WithDescription("[alpha] Perform all image builds, and output rendered Kubernetes manifests").
+		WithExample("Hydrate Kubernetes manifests without building the images ", "render --digest-source=remote").


Suggested change

WithExample("Hydrate Kubernetes manifests without building the images ", "render --digest-source=remote").

WithExample("Hydrate Kubernetes manifests without building the images, using digest resolved from tag in remote registry", "render --digest-source=remote").

nkubala · 2020-06-09T20:04:16Z

cmd/skaffold/app/cmd/render.go

 		WithCommonFlags().
 		WithFlags(func(f *pflag.FlagSet) {
 			f.BoolVar(&showBuild, "loud", false, "Show the build logs and output")
 			f.StringVar(&renderOutputPath, "output", "", "file to write rendered manifests to")
+			f.StringVar(&opts.DigestSource, "digest-source", "local", "Set to 'local' to build and tag images, and output templated Kubernetes manifests; Set to 'remote' to resolve the digest of the image by tag from the container registry and output templated Kubernetes manifests; Set to 'none' to output templated Kubernetes manifests with images listed in skaffold.yaml")


Suggested change

f.StringVar(&opts.DigestSource, "digest-source", "local", "Set to 'local' to build and tag images, and output templated Kubernetes manifests; Set to 'remote' to resolve the digest of the image by tag from the container registry and output templated Kubernetes manifests; Set to 'none' to output templated Kubernetes manifests with images listed in skaffold.yaml")

f.StringVar(&opts.DigestSource, "digest-source", "local", "Set to 'local' to build images locally and use digests from built images; Set to 'remote' to resolve the digest of images by tag from the remote registry; Set to 'none' to use tags directly from the Kubernetes manifests")

nkubala · 2020-06-09T20:06:50Z

pkg/skaffold/runner/render.go

+		}
+	}
+	if r.runCtx.Opts.DigestSource == noneDigestSource {
+		color.Default.Fprintln(out, "--digest-source set to 'none', tags listed in skaffold.yaml will be used for render")


Suggested change

color.Default.Fprintln(out, "--digest-source set to 'none', tags listed in skaffold.yaml will be used for render")

color.Default.Fprintln(out, "--digest-source set to 'none', tags listed in Kubernetes manifests will be used for render")

ChrisGe4 · 2020-06-09T22:01:21Z

I made a few wording suggestions, specifically related to saying that when --digest-source=none, "image tags are used from the skaffold.yaml", which isn't actually true - they're used from the Kubernetes manifests, which is an important distinction.

I also ran into a bug with --digest-source=none:

use the getting-started example in our repo (with skaffold-example as the image in the skaffold.yaml and k8s-pod.yaml

append a :latest to the image in the k8s manifest

set your default-repo to something in skaffold (skaffold config set default-repo gcr.io/my-project)

skaffold render --digest-source=none should give me a manifest with something like
apiVersion: v1
kind: Pod
...
spec:
  containers:
  - image: gcr.io/my-project/skaffold-example:latest
    name: getting-started
but for the image name, I actually get gcr.io/my-project/skaffold-example:v1.10.0-82-gb616768d3-dirty, so it generated me a tag. appending the repository name to the image in the k8s manifest, however, gives me the correct result.

I'm not sure how deep this bug goes, so idk if it should be fixed in this PR or a follow-up. but --digest-source=none will be broken for anyone using the default-repo functionality in skaffold until it's fixed (which I suspect is a sizable number of users).

Thank you for reviewing this PR. I misunderstood the use case before and I have got the issue fixed. However, there is one question regarding the example you put in the comment. The image used in k8s-pod.yaml is skaffold-example, should I leave it as it be or add latest tag as it in your example? I think we should just use skaffold-example, right?

nkubala · 2020-06-09T23:20:07Z

ok just tried it out with --digest-source=none and it's just leaving the image alone now which is what I'd expect. one thing I noticed though is that it doesn't actually respect the default-repo value now - so even if I have gcr.io/my-project set as a default repo, the image in the rendered manifest is still skaffold-example when using --digest-source=none. for remote and local digests it's working as intended.

I think we do still want to respect the default repo in this case: this flag is indicating that the tag will not be touched by skaffold, but the default repo functionality should still be applied here.

should I leave it as it be or add latest tag as it in your example?

I actually wouldn't mind adding latest to the "rendered" image name in the manifest - in container image land, the absence of a tag usually implies latest anyway, so I think it's ok to just add it on there since it's probably what the user was trying to do. it's not a huge deal either way though, since functionally they should be equivalent.

addressed

ChrisGe4 and others added 12 commits April 19, 2020 17:52

render --output takes gcs path

45b0582

fix linter errors

f00567d

Merge remote-tracking branch 'upstream/master'

30c8462

pull from upstream

Code review changes.

1ba46ea

Update pkg/skaffold/deploy/util.go

f8b0f0f

More code review changes.

785effd

Merge remote-tracking branch 'upstream/master'

6e31845

merge upstream

Merge remote-tracking branch 'origin/master'

e1f32c9

More code review changes.

9c2cb1d

Merge remote-tracking branch 'upstream/master'

ea65111

merge upstream

Skaffold render hydrate manifests only

ec6ca63

Merge remote-tracking branch 'upstream/master'

ec2bfeb

Merge upstream changes.

ChrisGe4 requested review from balopat, briandealwis, dgageot, nkubala and tejal29 as code owners May 15, 2020 19:02

pull-request-size bot added the size/M label May 15, 2020

googlebot added the cla: yes label May 15, 2020

Fix linter errors

1364219

Fix maligned linter errors

3077a9e

balopat reviewed May 16, 2020

View reviewed changes

pkg/skaffold/runner/build_deploy.go Outdated Show resolved Hide resolved

balopat previously requested changes May 16, 2020

View reviewed changes

balopat self-assigned this May 18, 2020

balopat added the kokoro:run runs the kokoro jobs on a PR label May 20, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 20, 2020

ChrisGe4 added 3 commits June 1, 2020 14:49

Merge remote-tracking branch 'upstream/master'

e5aae71

merge upstream

Merge remote-tracking branch 'upstream/master'

e91978b

merge upstream

Change the flag to --digest-source.

86c3f80

ChrisGe4 requested a review from tstromberg as a code owner June 2, 2020 02:54

Fix linter error

b616768

ChrisGe4 requested a review from balopat June 2, 2020 04:01

brandonnelson3 reviewed Jun 4, 2020

View reviewed changes

brandonnelson3 approved these changes Jun 4, 2020

View reviewed changes

nkubala suggested changes Jun 9, 2020

View reviewed changes

ChrisGe4 added 2 commits June 9, 2020 18:14

--digest-source=none will use tags from Kubernetes manifests

bb76ece

fix linter error

15e8157

respect the default-repo value while using --digest-source=none

dd56955

pull-request-size bot added size/L and removed size/M labels Jun 10, 2020

nkubala approved these changes Jun 11, 2020

View reviewed changes

nkubala merged commit 256e79a into GoogleContainerTools:master Jun 11, 2020

fmotrifork mentioned this pull request Jun 12, 2020

skaffold 1.11.0 fishworks/fish-food#783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skaffold render solely perform the manifests hydration #4193

Skaffold render solely perform the manifests hydration #4193

ChrisGe4 commented May 15, 2020

codecov bot commented May 15, 2020 •

edited

Loading

balopat left a comment

brandonnelson3 Jun 4, 2020

nkubala left a comment •

edited

Loading

nkubala Jun 9, 2020

nkubala Jun 9, 2020

nkubala Jun 9, 2020

nkubala Jun 9, 2020

ChrisGe4 commented Jun 9, 2020

nkubala commented Jun 9, 2020

	// In dry-run mode or --digest-source set to 'remote' or 'none', we don't build anything, just return the tag for each artifact.
	// In dry-run mode or with --digest-source set to 'remote' or 'none', we don't build anything, just return the tag for each artifact.

	WithExample("Hydrate Kubernetes manifests without building the images ", "render --digest-source=remote").
	WithExample("Hydrate Kubernetes manifests without building the images, using digest resolved from tag in remote registry", "render --digest-source=remote").

	f.StringVar(&opts.DigestSource, "digest-source", "local", "Set to 'local' to build and tag images, and output templated Kubernetes manifests; Set to 'remote' to resolve the digest of the image by tag from the container registry and output templated Kubernetes manifests; Set to 'none' to output templated Kubernetes manifests with images listed in skaffold.yaml")
	f.StringVar(&opts.DigestSource, "digest-source", "local", "Set to 'local' to build images locally and use digests from built images; Set to 'remote' to resolve the digest of images by tag from the remote registry; Set to 'none' to use tags directly from the Kubernetes manifests")

	color.Default.Fprintln(out, "--digest-source set to 'none', tags listed in skaffold.yaml will be used for render")
	color.Default.Fprintln(out, "--digest-source set to 'none', tags listed in Kubernetes manifests will be used for render")

Skaffold render solely perform the manifests hydration #4193

Skaffold render solely perform the manifests hydration #4193

Conversation

ChrisGe4 commented May 15, 2020

codecov bot commented May 15, 2020 • edited Loading

Codecov Report

balopat left a comment

Choose a reason for hiding this comment

brandonnelson3 Jun 4, 2020

Choose a reason for hiding this comment

nkubala left a comment • edited Loading

Choose a reason for hiding this comment

nkubala Jun 9, 2020

Choose a reason for hiding this comment

nkubala Jun 9, 2020

Choose a reason for hiding this comment

nkubala Jun 9, 2020

Choose a reason for hiding this comment

nkubala Jun 9, 2020

Choose a reason for hiding this comment

ChrisGe4 commented Jun 9, 2020

nkubala commented Jun 9, 2020

codecov bot commented May 15, 2020 •

edited

Loading

nkubala left a comment •

edited

Loading