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

zkutils: auto remove the ZK docker container #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jieyu
Copy link
Contributor

@jieyu jieyu commented Jul 13, 2018

zkutils: auto remove the ZK docker container

Checklist

  • [] ~80% unit test coverage?
  • [] Updated README.md?
  • [] Completed sections of this PR template?
  • [] Edited all sections [IN BRACKETS]

Overview of Change

This is for the case where the test process is killed, leaving the ZK
container running. It'll be great if docker stop can also remove the
container as well.

Changelog [optional]

  • [] [optional list]

Affected Usage

ZK container will be auto removed if stopped.

This is for the case where the test process is killed, leaving the ZK
container running. It'll be great if `docker stop` can also remove the
container as well.
@jieyu
Copy link
Contributor Author

jieyu commented Jul 13, 2018

The windows CI failure looks unrelated to this change.

@@ -54,7 +54,9 @@ func StartZookeeper(opts ...func(*ZKConfig)) (*ZkControl, error) {

// the container IP is not routable on Darwin, thus needs port
// mapping for the container.
hostConfig := &container.HostConfig{}
hostConfig := &container.HostConfig{
AutoRemove: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of hard-coding this, maybe make it a field in ZKConfig and then you can configure it using a functional option. I'd like to avoid hard-coding new things here.

@janisz
Copy link

janisz commented Jun 2, 2020

@jieyu What's the status of this?

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