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

Add support for Docker compose withOptions(...) #2827

Merged
merged 7 commits into from
Sep 14, 2020

Conversation

Gapmeister66
Copy link
Contributor

@Gapmeister66 Gapmeister66 commented Jun 2, 2020

Adds support for docker compose options using a 'with' fluent style syntax.

This enables a client to pass for example, the '--compatibility' option to the docker-compose command.

Parameterized Junit Test has been added.

Modified code in line with review @kiview

Changed options method to accept vargs @rnorth

I had some issues re-basing (squashing) my branch commits. The operation seemed to conflict with new changes in the master branch. I managed to commit in the end, but without squashed commits from my branch. Apologies if the git history is messy. If anybody can give me some advise in this area, please do :)

kiview
kiview previously requested changes Jun 2, 2020
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Thanks a lot for submitting the PR, I assume this comes after our conversation in Slack? The approach looks good in general, just some comments with regards to implementation.

@@ -78,6 +78,7 @@
private boolean localCompose;
private boolean pull = true;
private boolean build = false;
private String options = StringUtils.EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a Set<String> to which to add the options. Or in case order would be important (it probably is not?) a List.

*
* @return this instance, for chaining
*/
public SELF withOptions(String options) {
Copy link
Member

Choose a reason for hiding this comment

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

Just adding to the Set here would be enough.
String for runWithCompose can be constructed using Stream and Collectors.joining.

Copy link
Member

Choose a reason for hiding this comment

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

Building on @kiview's suggestion to use a set, this also would mean that withOptions could take a varargs String, and just add all to the set. This would be quite nice semantically.

@Gapmeister66
Copy link
Contributor Author

No problem, I will make the changes as soon as possible.

Do I reuse the same branch then rebase to squash the commits together? What is the procedure you guys use?

@rnorth
Copy link
Member

rnorth commented Jun 6, 2020

@Gapmeister66 we use 'Squash and Merge' when we merge, so there's no need for you to manually squash or rewrite your commits before then (unless you want to).

@Gapmeister66
Copy link
Contributor Author

@rnorth

Hello Richard

How often do you guys run CI scripts over my branch ? I'm wondering whether CI has run over all of my commits thus far? The reason I ask is that my build failed at:

'Build and test with Gradle (:docs:examples:junit4:generic:check)'.

It seems to have happened since my last commit (varargs), but that just might be a coincidence if this is my first CI build. There is no error logged as far as I can tell.

I'm not a Gradle expert (I use Maven in my day job), does anybody have an idea of why my build failed?

-- and I though this would be easy :)

@stale
Copy link

stale bot commented Sep 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Sep 5, 2020
@rnorth rnorth added this to the next milestone Sep 14, 2020
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.

LGTM. Sorry for the extreme amount of time it's taken to review again, @Gapmeister66

@rnorth rnorth changed the title Docker compose options. Add support for Docker compose withOptions(...) Sep 14, 2020
@rnorth
Copy link
Member

rnorth commented Sep 14, 2020

Fixed merge conflicts.

@rnorth rnorth dismissed kiview’s stale review September 14, 2020 19:11

Comments have been reflected

@rnorth rnorth merged commit 77251a1 into testcontainers:master Sep 14, 2020
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

3 participants