build.go: initialize CustomLabels map if nil by paroque28 · Pull Request #9579 · docker/compose

paroque28 · 2022-06-21T05:01:14Z

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:

panic: assignment to entry in nil map

goroutine 16 [running]:
github.com/docker/compose/v2/pkg/compose.(*composeService).getLocalImagesDigests(0x1c0006d4318?, {0x1579128?, 0xc00069f710?}, 0xc000419220)
        /home/prodrigu/go/pkg/mod/github.com/docker/compose/[email protected]/pkg/compose/build.go:195 +0x305
github.com/docker/compose/v2/pkg/compose.(*composeService).ensureImagesExists(0x0?, {0x1579128, 0xc00069f710}, 0xc000419220, 0x0)
        /home/prodrigu/go/pkg/mod/github.com/docker/compose/[email protected]/pkg/compose/build.go:105 +0x7b
github.com/docker/compose/v2/pkg/compose.(*composeService).create(0xc0003efc30?, {0x1579128, 0xc00069f710}, 0xc000419220, {{0xc0004284e0, 0x2, 0x2}, 0x1, 0x0, {0x0, ...}, ...})
        /home/prodrigu/go/pkg/mod/github.com/docker/compose/[email protected]/pkg/compose/create.go:66 +0x173
github.com/docker/compose/v2/pkg/compose.(*composeService).Up.func1({0x1579128, 0xc00069f710})
        /home/prodrigu/go/pkg/mod/github.com/docker/compose/[email protected]/pkg/compose/up.go:36 +0x8e
github.com/docker/compose/v2/pkg/progress.Run.func1({0x1579128?, 0xc00069f710?})

(not mandatory) A picture of a cute animal, if possible in relation with what you did

paroque28 · 2022-06-21T05:03:29Z

It's my first PR so any feedback is welcome

milas

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 🙂

pkg/compose/build.go

paroque28 · 2022-06-24T20:45:30Z

Done, thank you @milas :)

glours · 2022-06-24T20:50:30Z

Hey @paroque28
Thanks for your contribution.
Is this PR link to an existing issue? Is it possible to have more context on why this fix? Just want to be sure we address the root cause and not an symptom of the issue

paroque28 · 2022-06-24T21:49:01Z

@glours
There is no linked issue, but I can create one if you need it.

The error was this:

panic: assignment to entry in nil map

I think it's a symptom since I am loading my project using the loader.Load() and the result contains an uninitialized CustomLabels map

I also notices that "com.docker.compose.project" was not set, and this is required for doing docker compose up

Here is my code: https://github.com/paroque28/container-chief/blob/master/pkg/manager/compose.go#L128
It's a small function

glours · 2022-07-04T12:43:37Z

@paroque28
We think that this issue should be fix directly in compose-go, so if that's fine for you we'll close this PR

paroque28 · 2022-07-04T13:48:55Z

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?

glours · 2022-07-04T14:23:41Z

@paroque28 when you load the project with loader.Load, you're using a function imported from "github.com/compose-spec/compose-go/loader", the project returned by this function should take care of initializing slices and maps of the internal structure

paroque28 · 2022-07-04T19:39:17Z

@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

glours · 2022-07-04T19:49:31Z

@paroque28 that's exactly my point, we should fix the behavior of loader.Load, I'll open a PR in compose-go

paroque28 · 2022-07-04T19:50:06Z

Oh yeah go it!

Sure, let's close this one

glours · 2022-07-07T15:55:10Z

Hey @paroque28
I was starting working on compose-go and reading the code I figured out that the existing code isn't using the safer way to add a custom label
So if you want to make your first contribution to Docker Compose, we can re-open your PR and I'll let you know how to fix properly the issue directly inside Compose code base!
WDYT?

paroque28 · 2022-07-07T16:06:52Z

Yes! That sounds perfect :)
How can I help you solve this issue?

pkg/compose/build.go

paroque28 · 2022-07-10T19:23:43Z

@glours done, sorry for the delay.

glours · 2022-07-11T10:12:02Z

@paroque28 you need to "sign off" your latest commit to make the CI pass

glours

LGTM

Accesing the map directly instead of the copy value, otherwise the label doesn't get set. Signed-off-by: Pablo Rodriguez <[email protected]>

hmb

If the commit message is correct, this might not work. If CustomLabels is nil the code has no effect.

hmb · 2024-06-08T17:17:53Z

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

milas suggested changes Jun 24, 2022

View reviewed changes

pkg/compose/build.go Outdated Show resolved Hide resolved

paroque28 force-pushed the patch-1 branch from 534ef87 to 1c2b2f0 Compare June 24, 2022 20:44

paroque28 requested a review from milas June 24, 2022 20:45

glours closed this Jul 4, 2022

glours reopened this Jul 7, 2022

glours requested changes Jul 7, 2022

View reviewed changes

pkg/compose/build.go Outdated Show resolved Hide resolved

pkg/compose/build.go Outdated Show resolved Hide resolved

glours mentioned this pull request Jul 8, 2022

Using Docker Compose as a library docker/roadmap#387

Open

paroque28 requested a review from glours July 10, 2022 19:23

glours approved these changes Jul 11, 2022

View reviewed changes

paroque28 force-pushed the patch-1 branch from 50c6220 to 3d9bfd2 Compare July 11, 2022 14:46

build.go: Access customLabels directly instead of by reference

eb06e1c

Accesing the map directly instead of the copy value, otherwise the label doesn't get set. Signed-off-by: Pablo Rodriguez <[email protected]>

paroque28 force-pushed the patch-1 branch from 3d9bfd2 to eb06e1c Compare July 11, 2022 14:52

paroque28 requested a review from glours July 11, 2022 14:53

glours enabled auto-merge July 12, 2022 10:44

glours disabled auto-merge July 12, 2022 10:44

glours merged commit 6c8ff02 into docker:v2 Jul 12, 2022

paroque28 mentioned this pull request Sep 5, 2022

Reading a Compose file results in undefined Labels #9808

Closed

RiskyFeryansyahP mentioned this pull request Sep 6, 2022

patch: build.go access custom labels directly cause panic #9810

Merged

hmb reviewed Jun 8, 2024

View reviewed changes

Conversation

paroque28 commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paroque28 commented Jun 21, 2022

Uh oh!

milas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paroque28 commented Jun 24, 2022

Uh oh!

glours commented Jun 24, 2022

Uh oh!

paroque28 commented Jun 24, 2022

Uh oh!

glours commented Jul 4, 2022

Uh oh!

paroque28 commented Jul 4, 2022

Uh oh!

glours commented Jul 4, 2022

Uh oh!

paroque28 commented Jul 4, 2022

Uh oh!

glours commented Jul 4, 2022

Uh oh!

paroque28 commented Jul 4, 2022

Uh oh!

glours commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paroque28 commented Jul 7, 2022

Uh oh!

Uh oh!

Uh oh!

paroque28 commented Jul 10, 2022

Uh oh!

glours commented Jul 11, 2022

Uh oh!

glours left a comment

Choose a reason for hiding this comment

Uh oh!

hmb left a comment

Choose a reason for hiding this comment

Uh oh!

hmb commented Jun 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paroque28 commented Jun 21, 2022 •

edited

Loading

glours commented Jul 7, 2022 •

edited

Loading