Repository navigation
Conversation
…uilt images When a Helm chart doesn't wire a built image into its values, skaffold only prints a warning. In `dev`/`run` that warning is quickly scrolled off screen by the container logs that follow, so users never notice their image wasn't deployed. Add a `--fail-on-unused-images` flag (dev, run, debug, deploy) that turns the warning into an actionable error which aborts the deployment. Default behavior is unchanged. Fixes GoogleContainerTools#3769 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WtfcVvV8XXi4Tn9s2bE3mm
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #3769
Description
Today, when Skaffold builds an image that the deployed Helm manifests never reference, the Helm deployer prints a warning and carries on:
This almost always means the chart's
values.yamlisn't wired up to the tag Skaffold produced, so the deployment silently runs a different image than the one that was just built. Inskaffold dev/skaffold runthe warning is immediately scrolled off screen by the container logs that follow, and users have to scroll back up to find it — which is exactly what #3769 reports. The result is a confusing debugging session where code changes appear to have no effect.This PR adds an opt-in
--fail-on-unused-imagesflag that turns that warning into an actionable error and aborts the deployment, so the problem surfaces immediately instead of scrolling away.Implementation
cmd/skaffold/app/cmd/flags.go— new--fail-on-unused-imagesbool flag (defaultfalse), defined ondev,run,debug, anddeploy. As with every registered flag it is also settable viaSKAFFOLD_FAIL_ON_UNUSED_IMAGES.pkg/skaffold/config/options.go— newSkaffoldOptions.FailOnUnusedImagesfield.pkg/skaffold/runner/runcontext/context.go—RunContext.FailOnUnusedImages()accessor, alongside the existingForceDeploy().pkg/skaffold/deploy/helm/helm.go— the HelmConfiginterface gainsFailOnUnusedImages() bool, and the deployer stores it.warnAboutUnusedImagesbecomescheckUnusedImages, which now collects all unused tags first and then either warns (default, byte-for-byte the same messages as before) or returns an error.Deploypropagates that error. The existing multi-config carve-out is preserved unchanged: multi-config projects can legitimately have artifacts that no single deployer consumes, so they are still skipped entirely.pkg/skaffold/helm/errors.go— newUnusedImagesErr, built withsErrors.NewErrorWithStatusCodelike the other Helm errors, so the failure flows through Skaffold's structured error reporting rather than being an opaquefmt.Errorf. It carriesStatusCode_DEPLOY_HELM_USER_ERRand aFIX_SKAFFOLD_CONFIG_HELM_ARTIFACT_OVERRIDESsuggestion pointing at the Helm image-configuration docs.The default code path is unchanged: without the flag, users get exactly the same warnings they get today.
User facing changes
New flag on
dev,run,debug, anddeploy:Before (and still the default, with no flag):
After, with the flag:
Because it is opt-in and defaults to
false, there is no behavior change for existing users; CI pipelines that want the stricter contract can turn it on (or exportSKAFFOLD_FAIL_ON_UNUSED_IMAGES=true).The generated CLI reference (
docs-v2/content/en/docs/references/cli/_index.md) is regenerated via./hack/generate-man.shand included in this PR.Testing
Three cases added to
TestHelmDeployinpkg/skaffold/deploy/helm/helm_test.go, driven by a newtestBuildsUnusedartifact thatvalidDeployYamldeliberately does not reference:unused image only warns by default— asserts the exact warning strings are still emitted and the deploy succeeds, pinning the unchanged default behavior.unused image fails the deployment with --fail-on-unused-images— asserts the deploy errors out.used image does not fail the deployment with --fail-on-unused-images— guards against false positives when every built image is referenced.The test's
helmConfigfake implements the newFailOnUnusedImages()method.Follow-up Work
kubectl/kustomizepaths, the plumbing (option → run context → deployerConfig) is already in place and the equivalent check can be added there in a follow-up.skaffold.yaml(e.g. underdeploy.helm) rather than — or in addition to — a CLI flag, happy to move it; the flag was chosen to match how--forceand the other deploy-behavior toggles are exposed.🤖 Generated with Claude Code
https://claude.ai/code/session_017RHHxazDH38gSMesDJHg9C