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

dockerComposeUp task #186

Merged
merged 6 commits into from Jun 28, 2018
Merged

Conversation

samrogerson
Copy link
Contributor

No description provided.

@samrogerson samrogerson requested a review from a team as a code owner June 27, 2018 23:10
import org.gradle.api.tasks.TaskAction

@Slf4j
class DockerComposeUp extends DefaultTask {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extend from an Exec task so that people can configure/override more stuff?

Copy link
Member

Choose a reason for hiding this comment

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

Is there best practices regarding defining a task in a separate groovy class vs directly in the plugin class? We do the latter in PalantirDockerPlugin although a separate class makes things cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate class is generally the way to go as it's much easier to add clear annotations for incremental behaviour, description etc etc.

It's also much easier to convert the separate classes from Groovy -> Java and gradle recommend writing plugins in Java over groovy https://docs.gradle.org/current/userguide/custom_plugins.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best practice is to not extend Exec.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK fair enough, this will probably be fine as an MVP anyway - can always add more configurability if people really need it later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I would say composition with project.exec provides way more flexibility than directly extending Exec since you're bound to execs TaskAction method.

when:
with('dockerComposeUp').build()
then:
file("foobarbaz").exists()
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah thanks for this - was infinitely better than what I'd done first.

Copy link
Member

@pkoenig10 pkoenig10 left a comment

Choose a reason for hiding this comment

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

Can you also update the README with information about this task?

@@ -137,4 +137,41 @@ class DockerComposePluginTests extends AbstractPluginTest {
then:
buildResult.output.contains("Could not find specified template file")
}

def 'docker-compose is executed and fails on invalid file'() {
Copy link
Member

Choose a reason for hiding this comment

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

Add a test specifiying a different docker-compose file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

hello:
container_name: "helloworld"
image: "python:3.4-alpine"
command: touch /test/foobarbaz
Copy link
Member

Choose a reason for hiding this comment

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

This can probably just be the alpine image (no python needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@uschi2000
Copy link
Contributor

Directionally, I'd prefer to move this plugin back to towards its original intent: to create Docker images. Docker CLI wrapping seems like a bottomless endeavor, see some of the other issues on this repo.

@samrogerson
Copy link
Contributor Author

In agreement with Rob. Longer term should break out the build orchestration utility tasks somewhere else.

@pkoenig10
Copy link
Member

Also in agreement about avoiding Docker CLI wrapping. The dockerRun plugin is the source of most of these feature creep issues.

Covnersely, I think adding Docker Compose tasks is valuable and less prone to feature creep. The dockerCompose plugin pushes most of the complexity and configurability into the provided template file. Adding some narrowly scoped tasks for managing Compose services allows us to keep the plugin simple while also supporting a wide range of workflows.

@@ -152,6 +152,10 @@ replaces all occurrences of version variables of the form `{{group:name}}` in
the `docker-compose.yml.template` file by the resolved versions and writes the
resulting file as `docker-compose.yml`.

The `docker-compose` plugin also provides a `dockerComposeUp` task that starts
the docker images specified in the `dockerComposeFile` in detached mode.
Copy link
Member

Choose a reason for hiding this comment

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

Also add the dockerComposeUp task to the list of tasks at the bottom of the README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

None yet

4 participants