Add option to render manifest from previous build result

[AndiDog]

Relates to #1187, #922, #1937

Should merge before : n/a

Should merge after : n/a

Description

Add option to render manifest from previous build result.

This is helpful e.g. in scenarios where build should be run just once, but manifest generation multiple times (for instance, once for dev/staging/prod).

User facing changes

Backwards-compatible: skaffold render first builds, unless the new parameter --build-artifacts is given – then it reads the images from the existing build output file.

Next PRs

Only slightly related: potentially, an option --no-build would make sense to render the manifest as specified in deploy: section, but without building and replacing image tags. That could be used in CI to fail early if the manifest / Helm chart isn't valid – before running the build which could take several minutes and incur network/compute resources.

Hints for reviewer

Didn't see a good place to add tests, since my implementation is at cmd level. Please advise and I'll happily add a test.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes unit tests
• Mentions any output changes.
• Adds documentation as needed: user docs, YAML reference, CLI reference.
• Adds integration tests if needed.

Reviewer Notes

• The code flow looks good.
• Unit test added.
• User facing changes look good.

Release Notes

`skaffold render` now supports a parameter `--build-artifacts` to take image tags from a previous file (as created by `skaffold build --file-output=...`). Without this parameter, the default behavior is to build the images first.

[AndiDog]

Changed parameter name to -a/--build-artifacts for consistency with skaffold deploy.

[balopat]

Thanks @AndiDog, can you please run ./hack/generate-man.sh and commit the results?

[balopat]

Added a nit. I think this looks good but can you think about adding a test somehow to this? If you're stuck with it, I'm happy to help out there.

[AndiDog]

@balopat do you have a hint where a test could be placed? My implementation is at cmd level, so integration/render_test.go doesn't really fit since the builds: []build.Artifact{ ... } is already filled there, instead of taken from a file. Ideally, there would be a test that runs the skaffold executable directly instead of running a Go test function. I could add a test for flags.BuildOutputFileFlag :: func BuildArtifacts which is what the command implementation uses?!

[balopat]

@balopat do you have a hint where a test could be placed? My implementation is at cmd level, so integration/render_test.go doesn't really fit since the builds: []build.Artifact{ ... } is already filled there, instead of taken from a file. Ideally, there would be a test that runs the skaffold executable directly instead of running a Go test function. I could add a test for flags.BuildOutputFileFlag :: func BuildArtifacts which is what the command implementation uses?!

I think adding an integration test is a good incremental step. You can create a new function TestKubectlRenderFromBuildOutput in integration/render_test.go that is similar to TestKubectlRender, just takes a build output file as well as an input.

[AndiDog]

I modified the command code to make it testable – please have a look. This also makes testing other deployers much easier because the test works almost exactly like running skaffold render directly.

Plus new commit "Ignore local executables for integration test (architecture/OS might mismatch)". Makes the kind-based integration test work on macOS for me.

Please have a look if this way of testing looks good.

[balopat]

I modified the command code to make it testable – please have a look. This also makes testing other deployers much easier because the test works almost exactly like running skaffold render directly.

Thanks for doing this work! The test logic itself looks good.
However - I don't see the benefit yet in the refactoring part: integration tests are supposed to call the built skaffold binary with flags. The three things you pass in here and the buildoutput flag should all be available as flags to the skaffold command.

[balopat]

Plus new commit "Ignore local executables for integration test (architecture/OS might mismatch)". Makes the kind-based integration test work on macOS for me.

Thanks - this is cool. I don't know when I ran kind integration tests on my mac the last time :) probably that's why you ran into this!

[balopat]

We have another motivation to remove the BUILDKIT env var, our kokoro test is failing due to this here:

++ gcloud auth configure-docker -q
Docker configuration file updated.
++ '[' -e /tmpfs/src/gfile/bazel-cache-gcs ']'
+ pushd /tmpfs/src/github/skaffold
+ GCP_ONLY=true
+ make integration-in-docker
DOCKER_BUILDKIT=1 time docker build \
		-f deploy/skaffold/Dockerfile \
		-t gcr.io/k8s-skaffold/skaffold-builder \
		.
buildkit not supported by daemon
Command exited with non-zero status 1

[AndiDog]

Working on the changes.

I noted that skaffold render connects to the Kubernetes cluster in the renderManifests function. That shouldn't be the case, or it's totally unusable for CI use cases. skaffold render should only render, i.e. do text replacement in an existing manifest, nothing else. That should be offline. Have to fix that as well before I can push the changes for your review.

[nkubala]

@AndiDog a few small nits but this is basically ready in my eyes. there are definitely a few more things to review than we'd like in this PR but in interest of not blocking you I think it's ok to let them in together.

[nkubala]

@AndiDog rename those two vars and rebase to get CI green and I'll merge this!

[AndiDog]

@nkubala ready!

addressed comments.