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

Feature/startup certain services #1528

Merged

Conversation

HaMatthias
Copy link
Contributor

Added the feature of setting certain services, defined in the docker-compose.yml to be started.

@HaMatthias
Copy link
Contributor Author

Solves #1497.

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

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

Only some trivial suggestions from me, otherwise looks good!


private void createServices() {
// Run the docker-compose container, which starts up the services
runWithCompose("up -d");
runWithCompose("up -d " + String.join(" ", this.services));
Copy link
Member

@rnorth rnorth Jun 6, 2019

Choose a reason for hiding this comment

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

In the case of services being an empty list, the run command will be "up -d ". I know that for Docker Compose this is functionally identical and starts all services.

However, just to be explicit in our code, could we perhaps do an if/else check around this, and only concatenate the services list onto "up -d" if necessary?

I'm fine to have the up string duplicated in the code.

You might also notice that I'm being a hypocrite about explicitness of code, when the original -d here is itself not very clear 😁. If you wouldn't mind changing -d to --detach while you're editing the code, that would be another improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
runWithCompose("up -d " + String.join(" ", this.services));
if(services.empty()) {
runWithCompose("up -d");
} else {
runWithCompose("up -d " + String.join(" ", services));
}

Would that be fine to you? Clearer separation is always better!

Note: The older versions of docker-compose do not support --detach (e.g. my version of 1.17.1). So that is why I would let it like that? Thanks for your suggestion :)

Copy link
Member

Choose a reason for hiding this comment

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

That looks good to me - thanks.

.gitignore Outdated
@@ -63,3 +63,4 @@ src/mkdocs-codeinclude-plugin
src/pip-delete-this-directory.txt

.DS_Store
.directory
Copy link
Member

Choose a reason for hiding this comment

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

I'm not against adding things to .gitignore, but it looks like this might have been added accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was not any accident. My OS generates this file, when the directory should show dot-files in the UI ;).

Copy link
Member

Choose a reason for hiding this comment

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

I've never seen .directory being generated before. What's your OS? 😄

If this is something that's fairly unique to your development environment, perhaps this belongs in your global gitignore rather than in individual repos.

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 am working with Dolphin on Ubuntu 18.04.2 LTS.
I will remove it, due to making global settings of it. Did some research.

@HaMatthias
Copy link
Contributor Author

Would be really nice to have this feature in master.

I really need to work with it and I am struggling to create my own .jar with all these dependencies (shadow Jar - especially the namespace org.rnorth.ducttape.ratelimits).

@HaMatthias
Copy link
Contributor Author

Requested changes are done.

@bsideup bsideup requested a review from rnorth June 25, 2019 11:52
@bsideup bsideup added this to the next milestone Jun 25, 2019
@bsideup
Copy link
Member

bsideup commented Jul 1, 2019

@rnorth bump

@flowrider3000
Copy link

How is the status of that? I would also need this :)

@rnorth rnorth merged commit 7fae384 into testcontainers:master Jul 6, 2019
@rnorth
Copy link
Member

rnorth commented Jul 7, 2019

This was released in 1.11.4 🎉

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants