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

Allow testcontainers to have custom labels #725

Merged
merged 6 commits into from
Jun 4, 2018

Conversation

mgansler
Copy link
Contributor

This may be useful for tools such as traefik which use docker labels for configuration

@kiview
Copy link
Member

kiview commented May 31, 2018

Created as part of the Gr8ConfEU 2018 Hackergarten 🙂

The Codacy error can be ignore.

* @param key label key
* @param value label value
*/
void addLabel(String key, String value);
Copy link
Member

Choose a reason for hiding this comment

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

should we avoid having non-fluent version of such methods? Or is there any value it adds?
/cc @rnorth @kiview

Copy link
Member

Choose a reason for hiding this comment

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

I would be okay with deprecating the old ones and not adding new ones.

@@ -473,6 +476,7 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
Map<String, String> labels = createCommand.getLabels();
labels = new HashMap<>(labels != null ? labels : Collections.emptyMap());
labels.putAll(DockerClientFactory.DEFAULT_LABELS);
labels.putAll(this.labels);
Copy link
Member

Choose a reason for hiding this comment

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

I would do it before DockerClientFactory.DEFAULT_LABELS (they must be set to the values we provide), and also before createContainerCmdModifiers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I a developer really wants to overwrite the default labels he or she should be able to do so.
I tried overwriting the org.testcontainers label with false and left the session_id untouched - the containers still were removed.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, IMO this is too dangerous.
Also, why would anyone override internal labels of Testcontainers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - should we silently ignore org.testcontainers.* labels or throw an exception?

Copy link
Member

Choose a reason for hiding this comment

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

an exception, plus an order change ( createContainerCmdModifiers are supposed to be low level and should be able to override any pre-set values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the university is closing down - I'll change this tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

Sure! Thanks for your contribution :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsideup done :) Please review all the changes, I will squash the commits when you give your okay ;)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! No need to squash, GitHub allows us to do it automatically 🎉

@ClassRule
public static GenericContainer alpine =
new GenericContainer("alpine:3.2")
.withExposedPorts(80)
.withEnv("MAGIC_NUMBER", "42")
.withLabel("our.custom", "label")
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 this use case (labels) is way too specific and should be documented separately (this examples shows common usage of the GenericContainer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, fair enough

* Create a container with a custom label for testing.
*/
@ClassRule
public static GenericContainer alpineCustomLabel = new GenericContainer("alpine:3.2")
Copy link
Member

@bsideup bsideup May 31, 2018

Choose a reason for hiding this comment

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

please use try-with-resources, otherwise this container will be started for any test of this class

Copy link
Member

Choose a reason for hiding this comment

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

Also, the container for this test can be as simple as:

new GenericContainer()
    .withLabel("our.custom", "label")
    .withCommand("ps")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it behaves like every other container in this test class ;)

Copy link
Member

Choose a reason for hiding this comment

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

not every, rather legacy ;)

Copy link
Member

Choose a reason for hiding this comment

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

See createContainerCmdHookTest, copyToContainerTest and others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see - much better :)

Map<String, String> combinedLabels = new HashMap<>();
combinedLabels.putAll(labels);
combinedLabels.putAll(DockerClientFactory.DEFAULT_LABELS);
combinedLabels.putAll(createCommandLabels);
Copy link
Member

Choose a reason for hiding this comment

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

DEFAULT_LABELS must always override :)

Copy link
Member

Choose a reason for hiding this comment

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

Also, I would change to:

if (createCommand.getLabels() != null) {
    combinedLabels.putAll(createCommand.getLabels());
}

to avoid a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the order should be user defined labels < createCommandLabels < DEFAULT_LABELS?

Copy link
Member

Choose a reason for hiding this comment

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

Yes :) Sorry if I confused you about it before!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries ;)

1. user defined labels
2. create command labels
3. DEFAULT_LABELS
@mgansler
Copy link
Contributor Author

mgansler commented Jun 1, 2018

Not sure why it failed on Travis - can you trigger it again? @bsideup

@bsideup
Copy link
Member

bsideup commented Jun 1, 2018

@mgansler triggered! There is a flaky test, sorry :)

@kiview
Copy link
Member

kiview commented Jun 3, 2018

I restarted again...

@mgansler
Copy link
Contributor Author

mgansler commented Jun 3, 2018

And they passed - finally :D

@kiview kiview merged commit a2c0981 into testcontainers:master Jun 4, 2018
kiview added a commit that referenced this pull request Jun 4, 2018
@bsideup bsideup added this to the next milestone Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants