Skip to content

feat: improves ValidateContextDirectory performance#1577

Merged
thaJeztah merged 2 commits intodocker:masterfrom
orisano:1576-improve-validate-context-directory
Apr 1, 2019
Merged

feat: improves ValidateContextDirectory performance#1577
thaJeztah merged 2 commits intodocker:masterfrom
orisano:1576-improve-validate-context-directory

Conversation

@orisano
Copy link
Copy Markdown
Contributor

@orisano orisano commented Dec 14, 2018

for #1576
reduces PatternMatcher construction.

@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "1576-improve-validate-context-directory" git@github.com:orisano/cli.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 14, 2018

Codecov Report

Merging #1577 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1577      +/-   ##
==========================================
+ Coverage   56.26%   56.26%   +<.01%     
==========================================
  Files         308      308              
  Lines       21293    21301       +8     
==========================================
+ Hits        11980    11986       +6     
- Misses       8439     8440       +1     
- Partials      874      875       +1

matches := func(file string) (bool, error) {
file = filepath.Clean(file)
if file == "." {
// Don't let them exclude everything, kind of silly.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure we can ignore this case; I know there's repositories using this (i.e. have .dockerignore with . in it, to not send anything)

Copy link
Copy Markdown
Contributor Author

@orisano orisano Dec 17, 2018

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm... interesting. I definitely have seen people using this in their .dockerignore. Wondering if it worked at all for them (haven't tried it)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should add a test case here, or at least copy the tests from https://github.com/moby/moby/blob/master/pkg/fileutils/fileutils_test.go#L221 (as the code was copied too), and maybe it will need to extract this inlined function.

Copy link
Copy Markdown
Contributor Author

@orisano orisano Dec 23, 2018

Choose a reason for hiding this comment

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

i added test cases

@orisano orisano force-pushed the 1576-improve-validate-context-directory branch from fea0322 to b9d6b5d Compare December 23, 2018 12:11
Copy link
Copy Markdown
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Thank you ! LGTM

@orisano
Copy link
Copy Markdown
Contributor Author

orisano commented Jan 8, 2019

what is the current status of this pr?

@silvin-lubecki
Copy link
Copy Markdown
Contributor

@thaJeztah PTAL 🦁

orisano added 2 commits March 28, 2019 01:20
Signed-off-by: Nao YONASHIRO <owan.orisano@gmail.com>
Signed-off-by: Nao YONASHIRO <owan.orisano@gmail.com>
@orisano orisano force-pushed the 1576-improve-validate-context-directory branch from b9d6b5d to 446762d Compare March 27, 2019 16:22
@orisano
Copy link
Copy Markdown
Contributor Author

orisano commented Mar 28, 2019

sorry, what is the current status of this pr?

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

sorry, this one dropped of my list 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants