skaffold icon indicating copy to clipboard operation
skaffold copied to clipboard

fix(helm): don't panic if reference parsing fails

Open jonas-jonas opened this issue 2 years ago • 2 comments

Description

Something in my setup broke and skaffold started panicking in the envVarForImage function:

WARN[0014] unable to extract values for IMAGE_REPO, IMAGE_TAG and IMAGE_DIGEST from image k3d-localhost:5111/***:v0.0.1-1004-g4cd4ad1a8@1ff316f0e620a4907dd028061654680b7c4b00f48d43dbdb906fec77bc3d96e5 due to error:
invalid reference format  subtask=-1 task=DevLoop
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x10 pc=0x105e7d664]

goroutine 1 [running]:
github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/helm.envVarForImage({0x14000b20bb0, 0xa}, {0x14000d9ae00, 0x75})
	/tmpfs/src/github/skaffold/pkg/skaffold/helm/args.go:149 +0x494
github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/helm.ConstructOverrideArgs(0x14000fbdb48, {0x14000d68300, 0x6, 0x14001270f40?}, {0x1400016cb00, 0x9, 0x10}, 0x14000d9af00?)
	/tmpfs/src/github/skaffold/pkg/skaffold/helm/args.go:62 +0xa88
// omitted for brevity

I am not sure how to reproduce this reliably, but I figured panicking is never desirable here, and the fix seemed easy. LMK what you think.

Not sure how to test this properly either, if you have any pointers for this, LMK.

jonas-jonas avatar May 17 '23 10:05 jonas-jonas

Codecov Report

Merging #8796 (c6378e5) into main (290280e) will decrease coverage by 6.54%. The diff coverage is 50.00%.

:exclamation: Current head c6378e5 differs from pull request most recent head 7e490d3. Consider uploading reports for the commit 7e490d3 to get more accurate results

@@            Coverage Diff             @@
##             main    #8796      +/-   ##
==========================================
- Coverage   70.48%   63.94%   -6.54%     
==========================================
  Files         515      620     +105     
  Lines       23150    31457    +8307     
==========================================
+ Hits        16317    20116    +3799     
- Misses       5776     9835    +4059     
- Partials     1057     1506     +449     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) :arrow_down:
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) :arrow_down:
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 40 more

... and 409 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar May 22 '23 17:05 codecov[bot]

@alphanota @plumpy @mattsanta Would you review this pull request?

mepi262 avatar Jan 22 '25 13:01 mepi262