-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
build.go: initialize CustomLabels map if nil #9579
Conversation
It's my first PR so any feedback is welcome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, and thanks for the PR!
CI failed because your commit didn't include a "sign off", you can read the details including how to fix it in the DCO action log - once you've done that and re-pushed, I'll get this merged 🙂
Done, thank you @milas :) |
Hey @paroque28 |
@glours The error was this:
I think it's a symptom since I am loading my project using the I also notices that Here is my code: https://github.com/paroque28/container-chief/blob/master/pkg/manager/compose.go#L128 |
@paroque28 |
What do you mean in compose-go? In my opinion people that import your library (like me) are going to hit this error and will have to do the reverse engineering as well . So, I would rather merge this PR but also add the custom labels to the loader. Idk, there are a couple ways of fixing this... Adding the labels, asserting ,etc... What do you think? |
@paroque28 when you load the project with |
@glours yes, that's what I did in my code but it's not enough. The loader doesn't initialize the custom labels. That's why I created this PR |
@paroque28 that's exactly my point, we should fix the behavior of |
Oh yeah go it! Sure, let's close this one |
Hey @paroque28 |
Yes! That sounds perfect :) |
@glours done, sorry for the delay. |
@paroque28 you need to "sign off" your latest commit to make the CI pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Accesing the map directly instead of the copy value, otherwise the label doesn't get set. Signed-off-by: Pablo Rodriguez <pablo.aarch64@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the commit message is correct, this might not work. If CustomLabels
is nil
the code has no effect.
My bad, should have looked into the current code, it's already fixed |
This change fixes the
panic: assignment to entry in nil map
error.I also accessed the map directly instead of the copy value, otherwise the label doesn't get set.
What I did
Related issue
Error that got me here:
(not mandatory) A picture of a cute animal, if possible in relation with what you did