Skip to content

Conversation

brutasse
Copy link
Contributor

@brutasse brutasse commented Jul 9, 2014

Fixes #265.

Implementation is a bit more elaborate than docker's implementation and matches with the one proposed in moby/moby#6869 to handle permission issues more nicely.

@shin-
Copy link
Contributor

shin- commented Jul 9, 2014

Awesome, thanks! Do you think you could add some unit tests?

@brutasse
Copy link
Contributor Author

brutasse commented Jul 9, 2014

@shin- I just added an integration test

@brutasse
Copy link
Contributor Author

Is there anything else that needs to be done to help getting this merged?

@shin-
Copy link
Contributor

shin- commented Jul 21, 2014

Hey, sorry for the late response, I was MIA for a week and didn't get a chance to check back.

I'd be interested in a couple unit tests for the utils.tar function specifically with different exclude values, and checking that the resulting excludes the expected files (i.e. by listing the tarfile contents). Do you think that's something you could add? That way if we introduce a bug in the future it would be caught by the CI immediately (whereas integration tests are only run manually).

Thanks a ton for the work you've done so far!

Fixes #265.

Implementation is a bit more elaborate than docker's implementation and
matches with the one proposed in moby/moby#6869 to handle permission
issues more nicely.
@brutasse
Copy link
Contributor Author

@shin- done. I've added a unit test for utils.tar that validates a couple of scenarios. In addition:

  • Some tests were leaking temp dirs, I added a couple of addCleanup calls to properly delete them
  • Python 2.6 doesn't have addCleanup() so I added an implementation for 2.6 that's loosely similar to what unittest does in Python >= 2.7

Let me know if anything else is needed.

@shin-
Copy link
Contributor

shin- commented Jul 23, 2014

Great, thanks a lot! Merging =)

shin- added a commit that referenced this pull request Jul 23, 2014
@shin- shin- merged commit 8e45264 into docker:master Jul 23, 2014
@hamiltont
Copy link

Thanks a lot @brutasse and @shin- !

@brutasse
Copy link
Contributor Author

✨ 🍰 🤘

@brutasse brutasse deleted the 265-dockerignore branch July 23, 2014 18:31
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 for .dockerignore

3 participants