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.

Sign up for GitHub

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

Jump to bottom

Support multi-dir wildcards in .dockerignore #17090

Merged
merged 1 commit into from
Nov 13, 2015
Merged

Support multi-dir wildcards in .dockerignore #17090

merged 1 commit into from
Nov 13, 2015

Conversation

duglin
Copy link
Contributor

@duglin duglin commented Oct 15, 2015

Closes #13113

This adds support for wildcarding multiple levels of dirs in .dockerignore files.
** (or **/) means zero or more dirs.
So **/*.go will exclude all go files from the build context.

Signed-off-by: Doug Davis [email protected]

@duglin duglin force-pushed the dockerignoreWildcards branch 2 times, most recently from 97939f2 to ffdabb8 Compare October 15, 2015 21:58
@duglin duglin mentioned this pull request Oct 15, 2015
Closed
@thaJeztah
Copy link
Member

Tests are failing :/

@duglin duglin force-pushed the dockerignoreWildcards branch 4 times, most recently from f90b822 to 192d307 Compare October 16, 2015 02:41
@duglin
Copy link
Contributor Author

duglin commented Oct 16, 2015

windows failure is real so please don't merge yet.... need debugging help from @jhowardmsft

@duglin duglin force-pushed the dockerignoreWildcards branch 2 times, most recently from eb176c5 to 8990eb9 Compare October 16, 2015 16:22
@duglin
Copy link
Contributor Author

duglin commented Oct 16, 2015

@jhowardmsft I think I got it working but I don't have a lot of confidence that its very stable. Please look over the escaping bits to see if any of it makes sense.

@tiborvass
Copy link
Contributor

I'm +1 for this to align with gitignore.

@vdemeester
Copy link
Member

I'm 😻 for it too 😉

@duglin
Copy link
Contributor Author

duglin commented Oct 17, 2015

https://git-scm.com/docs/gitignore
hmmm, close but not quite what I implemented - will look at the missing /** case.
So please don't merge quite yet.

@duglin
Copy link
Contributor Author

duglin commented Oct 20, 2015

While its not quite the exact same as .gitignore, its pretty close. Please take a look when you get a chance.

ping @tiborvass @vdemeester @jhowardmsft

@thaJeztah
Copy link
Member

would be cool if we could test this against the tests in git itself, e.g. https://github.com/git/git/blob/8d530c4d64ffcc853889f7b385f554d53db375ed/t/t0008-ignores.sh

@duglin
Copy link
Contributor Author

duglin commented Oct 20, 2015

@thaJeztah its not 100% compatible with git's stuff so I wouldn't be surprised if it failed. This is mainly because I believe git's logic takes into account whether we're talking about a file or a dir, and we don't have that information at this spot in the code. We might be able to redesign things so that we do, but that's a much bigger change since it leaves the boundaries of the Match() function and would impact things like the tar/archive logic. I can explore that option if people want....

@thaJeztah
Copy link
Member

@duglin nah, it's cool. Any change that makes .dockerignore act more like .gitignore is good, so I'm happy with this PR, we can evolve later if we want to.

@vdemeester
Copy link
Member

Repeating myself but looks good, and the tests too 😻. Agree with @thaJeztah too, so :
Design LGTM for me.

@duglin
Copy link
Contributor Author

duglin commented Oct 27, 2015

ok - added more integration tests so once Janky does its stuff we should be good to go.
I'll squash it once janky runs too.

@duglin duglin force-pushed the dockerignoreWildcards branch 4 times, most recently from 77976e0 to dd4983f Compare October 29, 2015 01:23
@duglin
Copy link
Contributor Author

duglin commented Oct 29, 2015

Failures on exp. & userns are unrelated.
ping @jfrazelle @tiborvass @vdemeester for review.
ping @jhowardmsft to make sure there isn't anything Windows specific I need to think about.

@vincentbernat
Copy link
Contributor

I suppose you meant @vdemeester instead of me.

@duglin
Copy link
Contributor Author

duglin commented Oct 29, 2015

@vincentbernat LOL oops, sorry. yes @vdemeester

@duglin
Copy link
Contributor Author

duglin commented Nov 8, 2015

ping @jfrazelle @tiborvass @vdemeester

@vdemeester
Copy link
Member

I'll take the +1 of @tiborvass and the "nah, it's cool. Any change that makes .dockerignore act more like .gitignore is good" from @thaJeztah as Design LGTMs and moving this to code review 😉

LK4D4
LK4D4 reviewed Nov 13, 2015
View reviewed changes
pkg/fileutils/fileutils.go

res, err := regexp.MatchString(regStr, path)

// fmt.Printf("(%s->%q,%q) -> (%v,%v)\n", pattern, regStr, path, res, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

forgotten debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL yes and no. I left it there because I kept having to uncomment it as I was testing windows.
If all reviews are ok then I'll remove this.

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 13, 2015

LGTM apart from suspicious comment

@duglin
Copy link
Contributor Author

duglin commented Nov 13, 2015

remove extra comment - per @LK4D4's comment

LK4D4 added a commit that referenced this pull request Nov 13, 2015
@LK4D4
Merge pull request #17090 from duglin/dockerignoreWildcards
f5e318a 
Support multi-dir wildcards in .dockerignore
@LK4D4 LK4D4 merged commit f5e318a into moby:master Nov 13, 2015
@thaJeztah thaJeztah added this to the 1.10 milestone Nov 14, 2015
@duglin duglin deleted the dockerignoreWildcards branch February 19, 2016 19:29
@sroze sroze mentioned this pull request Mar 5, 2016
Merged
@kutyel kutyel mentioned this pull request Jul 18, 2016
Merged
@jmarver jmarver mentioned this pull request Jan 6, 2017
Closed
@Lothiraldan Lothiraldan mentioned this pull request Jan 31, 2017
Closed
@camilamacedo86 camilamacedo86 mentioned this pull request Mar 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
impact/changelog status/2-code-review
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.
7 participants
@duglin @thaJeztah @tiborvass @vdemeester @vincentbernat @LK4D4 @GordonTheTurtle