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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonas-jonas
Copy link

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.

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

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

❗ 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%) ⬇️
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%) ⬇️
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

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

@jonas-jonas jonas-jonas force-pushed the jonas-jonas/fixPanicInEnvVarForImage branch from c6378e5 to 7e490d3 Compare June 26, 2023 18:09
@mepi262
Copy link

mepi262 commented Jan 22, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants