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

First pass at updating the "create" command to be idempotent #82

Closed
wants to merge 0 commits into from
Closed

Conversation

tnine
Copy link

@tnine tnine commented Jan 29, 2015

Refactored endpoint and version to be maven properties. This allows users of boot2docker to override in their maven profile for testing. This PR will address issues #79 and #81.

@tnine
Copy link
Author

tnine commented Jan 29, 2015

This isn't ready to ack yet. Rather, a work in progress where we can comment on the progress of the the issue.

@lordofthejars
Copy link
Member

@aslakknutsen we have a first approach of reusing an existing container if it is already started. Now I think we must discuss which is the best strategy to follow. Discard to use an already started container automatically, by using a configuration parameter, ...

It is true that reusing a container it should not be an automatic operation because it can be there something from previous execution. Also let suppose that the already started container have a link to another container, and that second container has just stopped correctly, then we have a problem because the container that is being reused is pointing out to a not created container.

It seems that reusing a container is more complicated that we though (or at least me) initially.

@tnine
Copy link
Author

tnine commented Jan 30, 2015

I set up an Ubuntu image with docker natively installed to check my changes, unfortunately they seem to have caused a problem with the native installation. I'll keep working on these changes and verify that they work in both environments.

I'm having issues getting the existing tests in CubeControllerTest.should_enrich_test* to pass. There's no comments, and it's not immediately obvious to me what they are testing. Can you give me a rundown of what they should be testing so I can start looking at what has been broken with the config changes?

@lordofthejars
Copy link
Member

I have just run the tests and they run and pass.

@@ -62,7 +65,7 @@

<container qualifier="tomcat_dockerfile">
<configuration>
<property name="host">localhost</property>
<property name="host">${docker.tomcat.host}</property>
Copy link
Member

Choose a reason for hiding this comment

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

host isn't really needed here. It will be auto updated by Cube anyway.

@tnine tnine closed this Jan 30, 2015
@tnine
Copy link
Author

tnine commented Jan 30, 2015

Changing this into 2 pull requests. You can find them here.

@tnine
Copy link
Author

tnine commented Jan 30, 2015

See #84 for fixing the docker ip issue

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