bug:Merge custom labels with default#2637
bug:Merge custom labels with default#2637bearrito wants to merge 2 commits intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Shouldn't we move the line calling user function bit lower in the function so Build args are not overwritten. |
|
Just to double check, the bug #2632 (and hence this PR) only affects the building of images, merging of labels already works as expected for creating containers? |
| for customLabelKey := range customLabels { | ||
| _, present := defaultLabels[customLabelKey] | ||
| if present || strings.HasPrefix(customLabelKey, LabelBase) { | ||
| return fmt.Errorf("custom labels cannot begin with %s or already be present", LabelBase) |
There was a problem hiding this comment.
Do we want to return an error here, or simply ignore those keys?
There was a problem hiding this comment.
I don't have an opinion. I did the same thing for python, so this keeps parity https://github.com/testcontainers/testcontainers-python/blame/0ce4fecb2872620fd4cb96313abcba4353442cfd/core/testcontainers/core/labels.py#L19
What does Java do?
There was a problem hiding this comment.
I think it's more user-friendly not returning an error here and possibly inform the user in the logs: "you did pass a wrong prefixed key? I can continue without them" Vs "you did pass a wrong prefixed key? Please stop and fix that". I know the latter is closer to what the Go compiler would say, right? So probably changing my mind while typing. Wdyt?
| buildOptions.Labels = core.DefaultLabels(core.SessionID()) | ||
| err := core.MergeCustomLabels(buildOptions.Labels, core.DefaultLabels(core.SessionID())) | ||
| if err != nil { | ||
| panic(fmt.Errorf("Unable to merge custom labels")) |
There was a problem hiding this comment.
We probably don't want to panic here, but pass the error to the caller. In any case, see my comment in https://github.com/testcontainers/testcontainers-go/pull/2637/files#r1673820440
| _, present := defaultLabels[customLabelKey] | ||
| if present || strings.HasPrefix(customLabelKey, LabelBase) { | ||
| return fmt.Errorf("custom labels cannot begin with %s or already be present", LabelBase) | ||
| } |
There was a problem hiding this comment.
I would return the error, but I would split and reorder so the caller knows exactly what happened:
| _, present := defaultLabels[customLabelKey] | |
| if present || strings.HasPrefix(customLabelKey, LabelBase) { | |
| return fmt.Errorf("custom labels cannot begin with %s or already be present", LabelBase) | |
| } | |
| if strings.HasPrefix(customLabelKey, LabelBase) { | |
| return fmt.Errorf("custom labels cannot begin with %q", LabelBase) | |
| } | |
| if _, present := defaultLabels[customLabelKey]; present { | |
| return fmt.Errorf("label %q already present", customLabelKey) | |
| } |
|
|
||
| func TestLabelBuilding(t *testing.T) { | ||
| labels := core.DefaultLabels("session-id") | ||
| assert.NotNil(t, labels) |
There was a problem hiding this comment.
nit: redundant check, Len will take care of it.
| assert.NotNil(t, labels) |
| } | ||
| err := core.MergeCustomLabels(goodCustomLabels, labels) | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, goodCustomLabels) |
There was a problem hiding this comment.
nit: redundant check Contains will take care of it.
| assert.NotNil(t, goodCustomLabels) |
| assert.Contains(t, goodCustomLabels, core.LabelLang) | ||
| assert.Len(t, goodCustomLabels, 6) | ||
|
|
||
| badCustomLabels := map[string]string{ |
There was a problem hiding this comment.
nit: I would use a subtest for good and bad, so one can fail and the other can pass, but that's a bit of personal preference.
|
|
||
| func TestLabelBuilding(t *testing.T) { | ||
| labels := core.DefaultLabels("session-id") | ||
| assert.NotNil(t, labels) |
There was a problem hiding this comment.
nit: I would recommend using require exclusively to avoid noise when one condition fails.
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
issue: defer the container terminate here, otherwise it won't get done on a test failure.
| require.NoError(t, err) | |
| require.NoError(t, err) | |
| t.Cleanup(func() { | |
| require.NoError(t, container.Terminate(context.Background())) | |
| }) |
|
|
||
| defer func() { | ||
| err := container.Terminate(ctx) | ||
| if err != nil { | ||
| log.Fatalf("could not terminate container: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
This is too late see comment above.
| defer func() { | |
| err := container.Terminate(ctx) | |
| if err != nil { | |
| log.Fatalf("could not terminate container: %v", err) | |
| } | |
| }() |
|
@bearrito we'd like to include this fix in the upcoming release. Could you take a look at Steven's comments? 🙏 Thanks! |
|
I think #2775 resolved the same behaviour. Closing. @bearrito thanks for this PR even though we merged the other one. It probably served as inspiration for the other author. From my side, I have to apologise if the review of this PR took that long, so please forgive me about that. I'm really happy with your recent contributions, so please keep sending them if you see anything is not behaving as you expect 🙏 |
5 participants
What does this PR do?
Fixes custom label merging with the default labels.
Why is it important?
If users set custom labels via container customizes, they will expect those labels are present in the container inspection
Related issues
How to test this PR
Unit tests have been added.