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

make docker build -rm=true default #4292

Merged
merged 1 commit into from Feb 26, 2014

Conversation

cpuguy83
Copy link
Member

Fixes part of #4291

Docker-DCO-1.1-Signed-off-by: Brian Goff cpuguy83@gmail.com (github: cpuguy83)

@tianon
Copy link
Member

tianon commented Feb 23, 2014

+9001 yesplz

@jamtur01
Copy link
Contributor

Definitely needs a docs update to make this very clear. Otherwise +1.

@cpuguy83
Copy link
Member Author

Wasn't sure if there was a doc for this or not.

@jamtur01
Copy link
Contributor

The cli.rst doc for sure and perhaps the Docker build docs.

@jamtur01
Copy link
Contributor

@jamtur01
Copy link
Contributor

@cpuguy83
Copy link
Member Author

When -rm is passed in, if the default is true does it set it to false or would it have to be docker build -rm=false ?

@cpuguy83
Copy link
Member Author

Updated docs.

@jamtur01
Copy link
Contributor

Docs LGTM /cc @metalivedev @SvenDowideit

@@ -208,7 +208,7 @@ Examples:

.. code-block:: bash

$ sudo docker build --rm .
$ sudo docker build --rm=true .
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if "true" becomes the default, shouldn't we instead just remove --rm from the invocation, so our examples get even simpler?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that...
But the doc is showing off what -rm does, not that it's default.

But either way is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh ok, that makes sense. :)

@tianon
Copy link
Member

tianon commented Feb 23, 2014

@cpuguy83 would you be willing to add the following patch to this, too? :)

diff --git a/Makefile b/Makefile
index e124d1d..e15ed35 100644
--- a/Makefile
+++ b/Makefile
@@ -32,10 +32,10 @@ shell: build
        $(DOCKER_RUN_DOCKER) bash

 build: bundles
-       docker build -rm -t "$(DOCKER_IMAGE)" .
+       docker build -t "$(DOCKER_IMAGE)" .

 docs-build:
-       docker build -rm -t "$(DOCKER_DOCS_IMAGE)" docs
+       docker build -t "$(DOCKER_DOCS_IMAGE)" docs

 bundles:
        mkdir bundles

(ie, remove -rm from our Makefile)

@SvenDowideit
Copy link
Contributor

LGTM - tho I think this needs to be in the Changelog to try to alert the old hands that life is different now

@tianon
Copy link
Member

tianon commented Feb 24, 2014

Uh, the "run" lines in the Makefile still need -rm :)

@cpuguy83
Copy link
Member Author

Whoops! See, you should have done a PR :)

@cpuguy83
Copy link
Member Author

Fixed.

@tianon
Copy link
Member

tianon commented Feb 24, 2014

:) LGTM

@cpuguy83
Copy link
Member Author

@SvenDowideit Is this something I need to do or is there someone that maintains this?

@tianon
Copy link
Member

tianon commented Feb 24, 2014

That's up to the release maintainer. :)

@creack
Copy link
Contributor

creack commented Feb 24, 2014

LGTM /cc @vieux @unclejack

@crosbymichael
Copy link
Contributor

LGTM

@crosbymichael
Copy link
Contributor

ping @shykes @vieux

This is an external change for the user, does it LG to you?

@vieux
Copy link
Contributor

vieux commented Feb 24, 2014

It's ok for me

On Mon, Feb 24, 2014 at 11:26 AM, Michael Crosby
notifications@github.comwrote:

ping @shykes https://github.com/shykes @vieux https://github.com/vieux

This is an external change for the user, does it LG to you?

Reply to this email directly or view it on GitHubhttps://github.com//pull/4292#issuecomment-35924722
.

Victor VIEUX
http://vvieux.com

@shykes
Copy link
Contributor

shykes commented Feb 24, 2014

If we're going to make it the default, I think we should hide it from any docs and examples other than the most advanced. Reduce the cognitive load.

Otherwise I agree with the change.

@SvenDowideit
Copy link
Contributor

@cpuguy83 I'd be thrilled if you updated the docs :) but if you don't want to, I'm willing to do it in a separate PR

@cpuguy83
Copy link
Member Author

I made the doc change, let me know if this is sufficient.

layers that were used to create each image layer. Doing so has no impact on
the image build cache.
If you wish to keep the intermediate containers after the build is complete,
you must use ``--rm=false``. This has no affect on the build cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be effect here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh... grammar... I still want to say this should be affect.
If -rm affected the build the result would be an effect ?

Or perhaps "This has no impact on the build cache"

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be something like This doesn't affect the build cache. if you want to stick to using affect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it's updated.

Thanks.

Docker-DCO-1.1-Signed-off-by: Brian Goff <cpuguy83@gmail.com> (github: cpuguy83)
@unclejack
Copy link
Contributor

LGTM

unclejack added a commit that referenced this pull request Feb 26, 2014
@unclejack unclejack merged commit 31ddc16 into moby:master Feb 26, 2014
@cpuguy83 cpuguy83 deleted the 4291_make_rm_default_for_build branch February 26, 2014 17:05
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

9 participants