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 pack builder suggest panic #1255

Merged
merged 1 commit into from
Aug 12, 2021
Merged

Fix pack builder suggest panic #1255

merged 1 commit into from
Aug 12, 2021

Conversation

yedamao
Copy link
Contributor

@yedamao yedamao commented Aug 12, 2021

  • check inspector.InspectBuilder returned info is not nil

Summary

Fix pack builder suggest panic

Output

Before

go test -v --run=Test_getBuilderDescription
=== RUN   Test_getBuilderDescription
--- FAIL: Test_getBuilderDescription (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x171447c]

goroutine 6 [running]:
testing.tRunner.func1.2(0x185ed40, 0x1ecf140)
        /usr/local/Cellar/go/1.16.6/libexec/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc00039a600)
        /usr/local/Cellar/go/1.16.6/libexec/src/testing/testing.go:1146 +0x4b6
panic(0x185ed40, 0x1ecf140)
        /usr/local/Cellar/go/1.16.6/libexec/src/runtime/panic.go:965 +0x1b9
github.com/buildpacks/pack/internal/commands.getBuilderDescription(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1a5d340, 0x1f15870, 0x3b20f68, 0x167da3c04692c)
        /Users/damao/workspace/contribute/pack/internal/commands/suggest_builders.go:121 +0x9c
github.com/buildpacks/pack/internal/commands.Test_getBuilderDescription(0xc00039a600)
        /Users/damao/workspace/contribute/pack/internal/commands/stack_suggest_test.go:77 +0x4b
testing.tRunner(0xc00039a600, 0x19a5658)
        /usr/local/Cellar/go/1.16.6/libexec/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
        /usr/local/Cellar/go/1.16.6/libexec/src/testing/testing.go:1238 +0x2b3
exit status 2
FAIL    github.com/buildpacks/pack/internal/commands    0.569s

After

go test -v --run=Test_getBuilderDescription
=== RUN   Test_getBuilderDescription
--- PASS: Test_getBuilderDescription (0.00s)
PASS
ok      github.com/buildpacks/pack/internal/commands    1.001s

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #1250

@yedamao yedamao requested a review from a team as a code owner August 12, 2021 05:10
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Aug 12, 2021
@github-actions github-actions bot added this to the 0.21.0 milestone Aug 12, 2021
- check `inspector.InspectBuilder` returned info is not nil

Signed-off-by: yedamo <[email protected]>
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #1255 (4d2d145) into main (f962404) will decrease coverage by 1.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
- Coverage   81.23%   80.09%   -1.14%     
==========================================
  Files         140      140              
  Lines        8581     8581              
==========================================
- Hits         6970     6872      -98     
- Misses       1176     1274      +98     
  Partials      435      435              
Flag Coverage Δ
os_linux 79.81% <100.00%> (-0.05%) ⬇️
os_macos 77.16% <100.00%> (ø)
os_windows 79.95% <100.00%> (-1.17%) ⬇️
unit 80.09% <100.00%> (-1.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jromero
Copy link
Member

jromero commented Aug 12, 2021

My initial inclination was that there's something more hidden behind the covers here. After looking at the implementation it does look like both err and info may be nil in the "Not Found" scenario. This patch will fix the presented error but I do wonder if a better approach would be to return our own error of "ErrNotFound" like most other common apis. 🥫 for thought.

image

@jromero jromero added type/chore Issue that requests non-user facing changes. and removed type/enhancement Issue that requests a new feature or improvement. labels Aug 12, 2021
Copy link
Member

@jromero jromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @yedamao for the contribution! Much appreaciated!

@jromero jromero merged commit 9352ef6 into buildpacks:main Aug 12, 2021
@jromero jromero added type/bug Issue that reports an unexpected behaviour. and removed type/chore Issue that requests non-user facing changes. labels Aug 12, 2021
@yedamao
Copy link
Contributor Author

yedamao commented Aug 12, 2021

I noticed the image.ErrNotFound error, but not sure that should be returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Issue that reports an unexpected behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pack builder suggest should not panic when using registry mirror
2 participants