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

bug:Merge custom labels with default #2637

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion container.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,10 @@ func (c *ContainerRequest) BuildOptions() (types.ImageBuildOptions, error) {
}

if !c.ShouldKeepBuiltImage() {
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"))
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

// Do this as late as possible to ensure we don't leak the context on error/panic.
Expand Down
48 changes: 48 additions & 0 deletions container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/stretchr/testify/require"

"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/internal/core"
"github.com/testcontainers/testcontainers-go/wait"
)

Expand Down Expand Up @@ -489,6 +490,53 @@ func TestShouldStartContainersInParallel(t *testing.T) {
}
}

func TestLabelBuilding(t *testing.T) {
labels := core.DefaultLabels("session-id")
assert.NotNil(t, labels)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: redundant check, Len will take care of it.

Suggested change
assert.NotNil(t, labels)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I would recommend using require exclusively to avoid noise when one condition fails.

assert.Len(t, labels, 4)
goodCustomLabels := map[string]string{
"org.custom.label1": "foo",
"org.custom.label2": "bar",
}
err := core.MergeCustomLabels(goodCustomLabels, labels)
require.NoError(t, err)
assert.NotNil(t, goodCustomLabels)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: redundant check Contains will take care of it.

Suggested change
assert.NotNil(t, goodCustomLabels)

assert.Contains(t, goodCustomLabels, core.LabelLang)
assert.Len(t, goodCustomLabels, 6)

badCustomLabels := map[string]string{
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

core.LabelLang: "python",
core.LabelSessionID: "invalid",
}
err = core.MergeCustomLabels(badCustomLabels, labels)
require.Error(t, err)
}

func TestLabelsForContainer(t *testing.T) {
ctx := context.Background()

customLabel := "org.company.environment"
container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
ContainerRequest: testcontainers.ContainerRequest{
Image: "alpine:latest",
Labels: map[string]string{customLabel: "test"},
},
})
require.NoError(t, err)

Comment on lines +525 to +526
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: defer the container terminate here, otherwise it won't get done on a test failure.

Suggested change
require.NoError(t, err)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, container.Terminate(context.Background()))
})

containerJson, err := container.Inspect(ctx)
require.NoError(t, err)
assert.Contains(t, containerJson.Config.Labels, core.LabelLang)
assert.Contains(t, containerJson.Config.Labels, customLabel)

defer func() {
err := container.Terminate(ctx)
if err != nil {
log.Fatalf("could not terminate container: %v", err)
}
}()
Comment on lines +531 to +537
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is too late see comment above.

Suggested change
defer func() {
err := container.Terminate(ctx)
if err != nil {
log.Fatalf("could not terminate container: %v", err)
}
}()

}

func ExampleGenericContainer_withSubstitutors() {
ctx := context.Background()

Expand Down
21 changes: 21 additions & 0 deletions internal/core/labels.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package core

import (
"fmt"
"strings"

"github.com/testcontainers/testcontainers-go/internal"
)

Expand All @@ -13,6 +16,7 @@ const (
LabelVersion = LabelBase + ".version"
)

// DefaultLabels returns the default map of Labels for a container
func DefaultLabels(sessionID string) map[string]string {
return map[string]string{
LabelBase: "true",
Expand All @@ -21,3 +25,20 @@ func DefaultLabels(sessionID string) map[string]string {
LabelVersion: internal.Version,
}
}

// MergeCustomLabels merges default labels in-place to the custom labels.
//
// It is an error to use "org.testcontainers" as a prefix to custom labels.
func MergeCustomLabels(customLabels map[string]string, defaultLabels map[string]string) error {
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)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to return an error here, or simply ignore those keys?

Copy link
Contributor Author

@bearrito bearrito Jul 11, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

}
Comment on lines +34 to +37
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would return the error, but I would split and reorder so the caller knows exactly what happened:

Suggested change
_, 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)
}

}
for defaultLabel, defaultValue := range defaultLabels {
customLabels[defaultLabel] = defaultValue
}

return nil
}
Loading