Seeing as this is my first time working on a non-trivial library (and also my first time contributing Go code), I would really appreciate any feedback, specifically about design, code quality, testing and documentation. I'm just another student trying to learn from the pros.

Thanks! :)

Seeing as this is my first time working on a non-trivial library (and also my first time contributing Go code), I would really appreciate any feedback, specifically about design, code quality, testing and documentation. I'm just another student trying to learn from the pros.

No worries, thanks so much for giving it a go 😄

let me ping @duglin, who's done a lot of work in this area, so may be a good person do an initial review

Will try to look it over deeper over the next day or so but my initial take on this is that I'm concerned about the performance of it. We already know that dockerignore processing is too slow, which is why #20128 was created and I still need to review it. I believe this will add another traversal over the entire build context - which can't be good for our timing.

I would prefer if we looked to merge the dockerignore processing into the tar'ing process so that we only walk the tree once. @vbatts would this totally break things because it introduces docker specific stuff into that process? I kind of think we've crossed that bridge already but may not have admitted it.

windowsTP4 timed out after 2 hours, restarting https://jenkins.dockerproject.org/job/Docker-PRs-WoW-TP4/2320/console

I just took a look at the TarWithOptions, and had a few thoughts.

Firstly, I took a look at #20128, and noticed that pwaller mentioned most of the time was spent creating the regular expressions from the patterns. If these were to be cached (perhaps in a map?), would it speed up the process? (At the cost of extra space. However, even with 100 rules, each at a filepath length of 100 4-byte utf-8 characters, this map would only take up 80kb).

Secondly, are we allowing a subdirectory's .dockerignore file state that a file in it's parent folder should be ignored?

So, if our directory structure is:

root
>subdir1
>>.dockerignore
>file1

Should root/subdir1/.dockerignore be able to list a rule ../file1?

If so, wouldn't that require two traversals, the first to parse all the rules, then the second to actually create the archive?

@thaJeztah, @duglin I'm also taking a look at pre-generating all the regular expressions, so that in cases where there are massive numbers of files, they aren't being regenerated hundreds or thousands of times over. That might be able to improve the performance. I think i have a replacement for the clean/build/match cycle that fileutils goes through, and am running tests. I'm looking to have that done before the end of the weekend.

I don't think ../file1 should be supported. We should try to match .gitignore, which treats the location of the .gitignore file itself as the "root" of the exclusion, i.e. https://git-scm.com/docs/gitignore.

Supporting ../...... also doesn't make sense if a .dockerignore is put at the root of the build-context, because Docker doesn't use files outside of the build-context (parent directories)

I just pushed a new commit which should address @duglin's performance concerns. Based on pwaller's (why can't i mention/ping him? :s) comments in #20128, i have added a new package, implementing a form of pre-compiled regular expressions. Unlike in #20128, the gobwas/glob library is not used, as it appears that it would require some adapting from the current regular expression set that is currently supported to those that glob supports.

Based on this, a back-of-the envelope estimate that I got by running all DockerSuite.TestBuild* tests are as follows:

Docker master branch: ~ 917.926s
Previous commit (Added recursive .dockerignore capabilities): ~900.838s
Current commit (Added Pre-compiled Regular Expressions): ~889.742s

As always, comments would be much appreciated! :)

ping @thaJeztah @duglin

Just wanted to check back in on this PR. If there's any feedback, or anything that needs to be changed, I'd love to know.

Hi @ObsessiveOrange you can ping me, but I'm not a docker contributor so I don't show up in auto-complete (unless I participate in this PR). Feel free to ping me.

The PR looks good to me, although pkg/precompiledregexp looks a bit weird as a name. Only a minor comment.

I'll try and find a moment to take performance measurements with/without your changes from my side. No promises though.

Hi @pwaller! Thanks for your comments! The whole non auto-complete thing makes a lot more sense now.

If you could get some performance data, that'd be great! I don't actually have anything to test it on, except building the standard docker container.

Cheers!

Hi @pwaller, any chance that you would have had time to take a look at the performance of these patches?

Also, @duglin, is there any chance that you would you have time to look through some of this code?

@ObsessiveOrange we just discussed this PR in the maintainers review session and we are worried about breaking backward compatibility for a "nice to have". For example; we know many users have a directory structure containing multiple Dockerfiles and associated .dockerignore files (e.g. for different builds from the same source). Currently, those .dockerignore files are "ignored", but with this change, they will actually do something, and break existing setups.

We don't want to break backward compatibility for a "nice to have", so decided to not proceed with this PR.

We really appreciate you working on this, and taking the time to contribute, so, sorry for closing this PR, but I hope you understand 👍

@thaJeztah Thanks for keeping me informed! I completely understand your (and the other maintainer's) decision.

If i may take a little more of your time, I just have one last thought regarding a change that might not break backward compatibility, while still allowing for this new feature.

I'm thinking that we could add a comment at the top of each .dockerIgnore file that should be recursive, something like the #!/usr/bin/env bash that bash scripts have. Maybe something along the lines of: #!recursive-root: SampleName for the root ignore file, and #! recursive-child: SampleName for all children .dockerignore files. This would then be required to be added to any subsequent .dockerignore file for it to be parsed. The recursive-child annotation would allow us to tell which root it was meant for. In this way, we could even have multiple sets of recursive .dockerignore trees, a few .dockerignore files that are entirely non-recursive, all without conflicting with each other.

Root .dockerignore:

#! recursive-root: Sample
build/*
runner.sh

Child .dockerignore:

#! recursive-child: Sample
src/test/resources/*
bin/*
!bin/compiled.exe

Further, by making the .dockerignore files only recursive when the comments are present, older .dockerignore entries are not affected unless explicitly set.

If this would work, i'd love to work on it, but if not, I completely understand :)

@ObsessiveOrange we briefly discussed other options, but adding such special "meta" information in the ignore files is adding too much complexity (both from coding, and from a UX perspective). For now, we decided to not add this as a feature.

Yep. sounds good! Thanks for letting me know! :) 👍