Add a new --ignore-file flag to docker build by vdemeester · Pull Request #29405 · moby/moby

vdemeester · 2016-12-14T13:18:18Z

This makes it possible to use something else than .dockerignore as a ignore file. Should fix #12886.

$ docker build -t foo/bar --ignore-file mydockerignore .
# […]

Questions/todos :

~~support multiple files ? (--ignorefile f1 --ignorefile f2 and merge the content naively)~~
update documentation

/cc @thaJeztah @duglin @tiborvass

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

duglin · 2016-12-14T14:01:30Z

Given how many people have asked for this, if we can do it nicely, then I'm in favor of it.

I don't think we need to support multiple files. Once we have a single file done we can explore adding it if people scream for it, but I'm not sure I see a lot of value yet. Should be easy to add though.

thaJeztah · 2016-12-14T14:17:23Z

I agree with @duglin; baby steps, and just having this is already a good start :-)

thaJeztah · 2016-12-14T14:18:16Z

api/swagger.yaml

should be "Path within the build context to the dockerignore file." (I think)

i.e., missing file at the end

thaJeztah · 2016-12-14T14:35:51Z

Design looks good to me

vdemeester · 2016-12-14T16:26:09Z

oh I guess it's breaking the cache & co… hum… I missed something somewhere 😂

tianon · 2016-12-15T19:57:50Z

Any particular reason we can't have both --ignore and --ignore-file? 😇

vdemeester · 2016-12-15T20:00:20Z

@tianon what do you mean ?

tianon · 2016-12-15T20:01:34Z

So I could just docker build --ignore .git rather than writing a whole new file 😇

tianon · 2016-12-15T20:05:58Z

Also, can this be specified more than once?

Right now, if I wanted to just "build this thing, but also ignore .git", I need to copy the entire .dockerignore verbatim to add a single line (no concept of includes there either), and then keep it in sync over time (for example, I've cloned someone else's repo and want to ignore extra stuff I've got locally that upstream doesn't care about -- the types of things we tell people to put in .git/info/excludes rather than .gitignore as concrete examples).

vdemeester · 2016-12-15T21:43:10Z

@tianon I hear you 👼 I think we should go step by step, and I would follow up this one with proposal on a --ignore and why not support of multiple --ignore-file too.

thaJeztah · 2016-12-15T21:48:37Z

@vdemeester agreed; was discussing this with @tianon already; this is a good start, and I'm open to future improvements

vdemeester · 2016-12-15T21:55:18Z

yup, and I really really like the idea of --ignore .git for example, that could help a lot of workflow quite a bit 👼

justincormack · 2016-12-15T23:00:56Z

@tianon you don't really want multiple ignores we want to be able to specify multiple bits of context, additive not subtractive. I will do a PR at some point.

justincormack · 2016-12-15T23:01:27Z

api/swagger.yaml

should be .dockerignore I think

justincormack · 2016-12-15T23:02:49Z

builder/dockerignore.go

Can we call this ignoreFile, its very confusing to have to work out what fileName refers to.

vdemeester · 2016-12-25T19:30:56Z

Rebase and @justincormack comments taken into account 👼

duglin · 2016-12-25T20:07:59Z

builder/dockerignore.go

I'm not sure this comment/logic is true in the case of someone being explicit about it. I would prefer if we threw an error if we were told to use a file and that file didn't exist. I think it should only silently ignore the "not found" error in the default case.

This would mean making the server side know what the default is (i.e. .dockerignore), I think it make sense, even just for retro-compatibility API-wise 👼. Hum actually I'm not sure anymore…

Good one; yes (without looking at how to accomplish it), I think it someone specified a .dockerignore file, and it doesn't exist, that should be an error. And if no file was specified, it should not be an error.

But, now sure if that can be achieved 😊

One way to know what's going on is to use an empty string to mean "not specified" - then in Process() if its "" you know you can default to .dockerignore and ignore the error if the file isn't there.

duglin · 2016-12-25T20:11:49Z

client/image_build.go

Related to my previous comment, if we don't send this query parameter at all when the --ignore-file wasn't specified then the server should be able to ignore the "not found" error because it's defaulting.

@duglin so docker build --ignore-file .dockerignore would behave differently than docker build ?

yes. I was thinking that docker build --ignore-file .dockerignore would do a couple of things:
1 - check for .dockerignore so that we can fail fast if its not there. Note, it would only do this check if the user specified the --ignore-file flag.
2 - send the appropriate ignore-file query param to the daemon - but only when the user specified a value. Meaning, if they didn't specify the --ignore-file flag then there would be no query param.
3 - the daemon would detect whether or not the query param was specified. If specified, regardless of its value, if the file isn't there then it would generate an error. If not there then we use the default value but do not generate an error if the file is missing. This is similar to how we act on the client - no error for a missing file when we're defaulting, but generate an error when the user was explicit.

ok, so this should be what is implemented now 👼

duglin · 2016-12-25T20:12:27Z

integration-cli/docker_cli_build_test.go

funky formatting

duglin · 2016-12-25T20:13:20Z

integration-cli/docker_cli_build_test.go

we should add a test for when the file isn't found - regardless of whether you change things based on my previous comments.

Thanks for adding the cli test for this. I think we may need one at the "api" level too - meaning one where we call the REST API with a missing dockerignore file since this just tests the client side, not the server's side.

duglin · 2016-12-25T20:13:59Z

we'll need to update the docs for this.

vdemeester · 2017-01-02T16:02:55Z

@duglin updated to take your comment into account 👼, added a tests case on that too

This makes it possible to use something else than .dockerignore as a ignore file. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

duglin · 2017-01-02T20:45:32Z

builder/dockerfile/internals.go

+		err = dockerIgnore.Process(ignoreFile, []string{b.options.Dockerfile})
+		if !ignoreErr && err != nil {
+			return err
+		}


Is there any reason this "ignoreErr" logic isn't in Process()? I worry about putting it here because it assumes that the only possible error from Process() is that .dockerignore doesn't exist - which right now is true. But, if at some point in the future another error is returned then the next person to touch this code might do something really hacky like start checking for certain errors - or worse, forget to even check at all and introduce a bug.

Its not a big thing, but I would prefer if all of this new logic were moved into Process() so that we can add the ignoreErr logic right at the os.IsNotExist(err) line of code and avoid this potential headache later on. I think the code will be smaller/easier too.

duglin · 2017-01-02T20:46:31Z

builder/dockerignore/dockerignore.go

 	}
 	if err := scanner.Err(); err != nil {
-		return nil, fmt.Errorf("Error reading .dockerignore: %v", err)
+		return nil, fmt.Errorf("Error reading dockerignore file: %v", err)


Can we add the name of the dockerignore file to this error message to help to user when they "fat finger" it?

duglin · 2017-01-02T20:57:08Z

cli/command/image/build.go

 		if err == nil {
 			excludes, err = dockerignore.ReadAll(f)
-			if err != nil {
+			if !ignoreErr && err != nil {


Is this the right spot for this? if we get an os.IsNotExist() above then won't f be nil and then the ReadAll() will return immediately with nil, nil? It seems as though ignoreErr might not be needed at all, or you'll want to add it to the if err == nil { line (2 lines above) to skip the ReadAll() in that case. But, it actually seems like that "if" might be better written as if f != nil {

edited comment, was missing a "not"

justincormack · 2017-01-06T18:35:20Z

I do think that specifying the files you don't want is totally wrong by the way. Subtractive specification is hard. I write all my docker builds as tar cf - Dockerfile files I want | docker build - so I can additively specify files. If docker build accepted more than one file for the context life would be much easier and no one would mess around with .dockerignore at all, they would just list the files they wanted...

thaJeztah · 2017-01-07T16:28:36Z

Well, it's modeled after .gitignore. Also, you'd still be able to do this, by ignoring everything, then add files you do want as not ignored

justincormack · 2017-01-07T20:19:31Z

@thaJeztah the use case quoted in the issue is multiple Dockerfiles in one directory. I use these a lot and ignore is absolutely the wrong way to deal with them.

for example, taking one of my things:
Dockerfile.1 uses file.1, directory.1, file.shared, directory.shared
Dockerfile.2 uses file.2, directory.2, file.shared, directory.shared

So I do

tar cf - Dockerfile.1 file.1 directory.1 file.shared directory.shared | docker build -
tar cf - Dockerfile.2 file.2 directory.2 file.shared directory.shared | docker build -

and thats great. I want to do, in the future

docker build -f Dockerfile.1 file.1 directory.1 file.shared directory.shared
docker build -f Dockerfile.2 file.2 directory.2 file.shared directory.shared

After this PR I would have to create two more files:

ignore.1 < Dockerfile.2 file.2 directory.2
ignore.2 <  Dockerfile.1 file.1 directory.1

and then do

docker build -f Dockerfile.1 --ignore ignore.1 .
docker build -f Dockerfile.2 --ignore ignore.2 .

This is a terrible way to do things, the ignore file for the first build is listing the files for the second build because it is subtractive not additive. If the second build needs more files, I need to add them to the ignore file for the first one. The more builds I have the more I have to list in each file.

In addition if you use make, with the additive way the files for the context are the make dependencies, so I tend to just write:

build.1: Makefile.1 file.1 file.shared directory.shared
       tar cf - $^ | docker build -

which could be the simpler docker build $^ with multi file contexts.

duglin · 2017-01-08T14:33:26Z

@justincormack what's interesting about your use case is that its basically asking for a new Dockerfile command to list the files to be included in the build context. Then you wouldn't need to jump thru the tar steps you do - it would make the Dockerfile more self-describing.

justincormack · 2017-01-08T23:47:18Z

I guess you could put it in the Dockerfile, hadn't thought of that.

…

On 8 Jan 2017 14:33, "Doug Davis" ***@***.***> wrote: @justincormack <https://github.com/justincormack> what's interesting about your use case is that its basically asking for a new Dockerfile command to list the files to be included in the build context. Then you wouldn't need to jump thru the tar steps you do - it would make the Dockerfile more self-describing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29405 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdcPFJZVPZR4uDW3sWmTpb3MSzXTc5Zks5rQPO9gaJpZM4LM6R8> .

thaJeztah · 2017-01-09T09:09:23Z

This is a terrible way to do things, the ignore file for the first build is listing the files for the second build because it is subtractive not additive. If the second build needs more files, I need to add them to the ignore file for the first one. The more builds I have the more I have to list in each file.

It really depends on the use case; if the only difference between Dockerfile1 and Dockerfile2 is to have a single directory excluded, having to specify everything else is terrible.

I think allowing multiple paths to be set for the build-context is slightly orthogonal to this PR; this PR just brings having a second Dockerfile "on parity" with the "main" Dockerfile.

I don't think both options are mutually exclusive though

what's interesting about your use case is that its basically asking for a new Dockerfile command to list the files to be included in the build context. Then you wouldn't need to jump thru the tar steps you do - it would make the Dockerfile more self-describing.

Yes, I think we've discussed such options before, i.e., have the client only send whats actually needed to the daemon. Previous discussions were more about doing so automatically, but having a way to explicitly set what's needed is probably an easier solution.

I do think something like that would make sense, and makes it easier to use, as having to manually specify all paths to include during build makes it quite hard to use ("to build this Dockerfile, specify the following options during docker build ...."). Ideally build should be just docker build -t foo . in most cases.

justincormack · 2017-01-09T10:52:05Z

@thaJeztah no, because when you make changes, eg you refactor your code, you have to refactor a negative, which basically means unrelated code. As you see from my example, the ignore file for build 1 only refers to files that are used in build 2, so if you change build 2, you have to change ignore file 1. This means when you try to split your code, eg split these into different directories, the commit history is horribly entangled.

The model for docker build is not git, it is go build. go build takes a directory or a list of files. It does not have an --exclude flag, nor does it have an exclude file. Sure this might make your command line shorter in extremely rare situations, but you will not get that added to go as the design is wrong: build processes specify what they are built from, not what they are not built from. This is the whole basis of dependency management and analysis.

This change is not actively harmful, it is just adding bloat by taking one existing design decision to a really complex extreme, instead of changing a much simpler part of the design which would make this totally unnecessary.

justincormack · 2017-01-17T15:49:36Z

I opened #30101 to discuss alternatives

LK4D4 · 2017-01-27T18:33:10Z

I see that this is in code-review, but discuss looks like design-review.
@vdemeester Should we proceed with review?

vdemeester · 2017-01-27T20:45:44Z

@LK4D4 maybe it should go back to design-review if there is no consensus 😅

LK4D4 · 2017-01-27T21:26:33Z

@vdemeester still @justincormack's proposal was proven to be undoable. Maybe we should discuss on maintainers meeting.

thaJeztah · 2017-02-02T20:03:34Z

We discussed this in the maintainers meeting, and think that this is not the right approach; having the builder selectively upload content that is used, will probably be a better solution, and something that is actively looked in to

vdemeester · 2017-02-02T20:27:28Z

ok :)

thaJeztah · 2017-02-02T23:41:04Z

sorry 😢 😄

dvalentiate · 2018-01-02T19:35:22Z

Has anything come along to replace the proposed functionality of this PR?

Not having --ignore-file is making life difficult for the teams I'm working with.

thaJeztah · 2018-01-02T20:17:28Z

@dvalentiate the issue associated with this PR is #12886, feel free to provide input/suggestions there

GordonTheTurtle added the status/0-triage label Dec 14, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Dec 14, 2016

vdemeester force-pushed the build-ignore-file-flag branch from 9917d3b to 63c976b Compare December 14, 2016 14:18

thaJeztah reviewed Dec 14, 2016

View reviewed changes

vdemeester force-pushed the build-ignore-file-flag branch from 63c976b to 928ce93 Compare December 14, 2016 14:19

thaJeztah added status/2-code-review and removed status/1-design-review labels Dec 14, 2016

vdemeester force-pushed the build-ignore-file-flag branch 2 times, most recently from 7ab916b to 20ffb77 Compare December 15, 2016 13:42

justincormack suggested changes Dec 15, 2016

View reviewed changes

vdemeester force-pushed the build-ignore-file-flag branch from 20ffb77 to 594bd8e Compare December 25, 2016 19:30

duglin reviewed Dec 25, 2016

View reviewed changes

integration-cli/docker_cli_build_test.go Outdated

Copy link
Copy Markdown

Contributor

duglin Dec 25, 2016

Choose a reason for hiding this comment

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

funky formatting

duglin reviewed Dec 25, 2016

View reviewed changes

vdemeester force-pushed the build-ignore-file-flag branch from 21f7c7d to 170f251 Compare January 2, 2017 16:02

Add a new --ignore-file flag to docker build

568d669

This makes it possible to use something else than .dockerignore as a ignore file. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the build-ignore-file-flag branch from 170f251 to 568d669 Compare January 2, 2017 17:31

duglin reviewed Jan 2, 2017

View reviewed changes

justincormack mentioned this pull request Jan 12, 2017

additive build context #30101

Closed

LK4D4 added status/1-design-review and removed status/2-code-review labels Jan 27, 2017

thaJeztah mentioned this pull request Feb 2, 2017

Speedup build by skipping what's bind-mounted #29298

Closed

thaJeztah closed this Feb 2, 2017

vdemeester deleted the build-ignore-file-flag branch February 2, 2017 20:27

piotrekkr mentioned this pull request Jun 29, 2022

Add support for specifying .dockerignore file with -i/--ignore #12886

Closed

Conversation

vdemeester commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duglin commented Dec 14, 2016

Uh oh!

thaJeztah commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 14, 2016

Uh oh!

vdemeester commented Dec 14, 2016

Uh oh!

tianon commented Dec 15, 2016

Uh oh!

vdemeester commented Dec 15, 2016

Uh oh!

tianon commented Dec 15, 2016

Uh oh!

tianon commented Dec 15, 2016

Uh oh!

vdemeester commented Dec 15, 2016

Uh oh!

thaJeztah commented Dec 15, 2016

Uh oh!

vdemeester commented Dec 15, 2016

Uh oh!

justincormack commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Dec 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Dec 25, 2016

Uh oh!

vdemeester commented Jan 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justincormack commented Jan 6, 2017

Uh oh!

thaJeztah commented Jan 7, 2017

Uh oh!

vdemeester commented Dec 14, 2016 •

edited

Loading

vdemeester Jan 2, 2017 •

edited

Loading

duglin Jan 2, 2017 •

edited

Loading