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

Support .cortexignore file to exclude files/directories from cortex project zip #800

Merged
merged 12 commits into from Feb 12, 2020

Conversation

wingkwong
Copy link
Contributor

closes #723


checklist:

  • run make test and make lint
  • test manually (i.e. build/push all images, restart operator, and re-deploy APIs)

@claassistantio
Copy link

claassistantio commented Feb 9, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@deliahu deliahu left a comment

Choose a reason for hiding this comment

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

@wingkwong thank you for your contribution, this is a great feature!

I made a few comments on the pull request, but nothing major, and most of them are up to you if you want to make the changes I mentioned. I also pushed a few very small changes to your branch.

I had two questions regarding the behavior of .cortexignore:

  1. As I was running the code, it seemed to me that it didn't work quite as I expected. I was in the examples/tensorflow/iris-classifier directory, I created .cortexignore with *.md, but README.md was still included in the project files (I tested this by printing out projectPaths in cli/cmd/deploy.go).

  2. I wasn't able to determine by reading the code where a leading ! is handled (I didn't see mention of it in filepath.Match).

Please let me know your thoughts!

pkg/lib/files/files.go Outdated Show resolved Hide resolved
pkg/lib/files/files.go Outdated Show resolved Hide resolved
pkg/lib/files/files_test.go Outdated Show resolved Hide resolved
pkg/lib/files/files_test.go Outdated Show resolved Hide resolved
pkg/lib/zip/zip.go Outdated Show resolved Hide resolved
@wingkwong
Copy link
Contributor Author

@wingkwong thank you for your contribution, this is a great feature!

I made a few comments on the pull request, but nothing major, and most of them are up to you if you want to make the changes I mentioned. I also pushed a few very small changes to your branch.

I had two questions regarding the behavior of .cortexignore:

  1. As I was running the code, it seemed to me that it didn't work quite as I expected. I was in the examples/tensorflow/iris-classifier directory, I created .cortexignore with *.md, but README.md was still included in the project files (I tested this by printing out projectPaths in cli/cmd/deploy.go).
  2. I wasn't able to determine by reading the code where a leading ! is handled (I didn't see mention of it in filepath.Match).

Please let me know your thoughts!

  1. If *.md is in cortexignore.md
    https://github.com/wingkwong/cortex/blob/ebfd26301609042f9b3b6c52e2cc3ee00d66813e/pkg/lib/files/files_test.go#L188

and I expect there is a README.md. The test case will be failed
image

so it should be able to ignore it.

  1. the logic comes from
    https://github.com/wingkwong/cortex/blob/b0546616882bc9d17380c551b106a515ca91896e/pkg/lib/files/files.go#L615

@deliahu
Copy link
Member

deliahu commented Feb 10, 2020

Thanks for addressing the comments!

For 1, I realized the root of the issue was that were were matching against the relative paths, but when ListDirRecursive() was called with relative set to false (as it is in cli/cmd/deploy.go), then it was not working as expected.

For 2, I did see this logic, however it seems like this is just for parsing the patterns, but the ! was not being interpreted by the matcher.

It seemed like even according to Docker's documentation, they had to do some workarounds on top of Go's filepath.Match(). So I searched for a replacement to the matcher and found this one: https://godoc.org/github.com/denormal/go-gitignore. I switch to it in to your PR, and also tried using the files.IgnoreFn() pattern, what do you think of it?

@wingkwong
Copy link
Contributor Author

Thanks for addressing the comments!

For 1, I realized the root of the issue was that were were matching against the relative paths, but when ListDirRecursive() was called with relative set to false (as it is in cli/cmd/deploy.go), then it was not working as expected.

For 2, I did see this logic, however it seems like this is just for parsing the patterns, but the ! was not being interpreted by the matcher.

It seemed like even according to Docker's documentation, they had to do some workarounds on top of Go's filepath.Match(). So I searched for a replacement to the matcher and found this one: https://godoc.org/github.com/denormal/go-gitignore. I switch to it in to your PR, and also tried using the files.IgnoreFn() pattern, what do you think of it?

I like this approach. It looks more cleaner!

@deliahu
Copy link
Member

deliahu commented Feb 11, 2020

Great! I'll send it to @vishalbollu for review, and then we'll merge it in

cli/cmd/deploy.go Show resolved Hide resolved
pkg/lib/files/files.go Show resolved Hide resolved
@deliahu
Copy link
Member

deliahu commented Feb 12, 2020

@wingkwong I will merge it now, thank you for your contribution!

@deliahu deliahu merged commit 2240b96 into cortexlabs:master Feb 12, 2020
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.

Support .cortexignore file to exclude files/directories from cortex project zip
4 participants