Skip to content

Addresses #268 : local builds should respect .gitignore #269

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

Closed
wants to merge 4 commits into from
Closed

Addresses #268 : local builds should respect .gitignore #269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc4d148
Addresses #268 : local builds should respect .gitignore
spMohanty Mar 18, 2018
7d96e95
Refactors #268 : Use content managers for filehandling
spMohanty Mar 19, 2018
86a1f8e
Addresses #268 : Use relative filepaths for pathspec matchings
spMohanty Mar 19, 2018
a9a917f
Addresses #268: Implments tests
spMohanty Mar 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion repo2docker/buildpacks/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import io
import os
import re
import pathspec
import logging
import docker

Expand Down Expand Up @@ -349,7 +350,21 @@ def _filter_tar(tar):
src_path = os.path.join(os.path.dirname(__file__), *src_parts)
tar.add(src_path, src, filter=_filter_tar)

tar.add('.', 'src/', filter=_filter_tar)
_exclude_tar = None
if os.path.exists(".gitignore"):
gitignore_fh = open(".gitignore")
Copy link
Member

Choose a reason for hiding this comment

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

can you use a context manager here so that the file gets closed?

Copy link
Member

Choose a reason for hiding this comment

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

Typical docker builds use a separate .dockerignore and ignore .gitignore. Should we follow that pattern? I'd rather be as close to exact for what docker build would normally do as we can.

Copy link
Author

Choose a reason for hiding this comment

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

@betatim : sure, I will push in another commit with a bit of refactoring, and also the tests.

@minrk : Thats more of a usability question. If repo2docker wants to be transparent to the user then, adding a separate .dockerignore might add to the confusion. You could add conditionals to check if both .dockerignore and .gitignore exist, and prioritize the .dockerignore, but if the repository just has a .gitignore then you use that instead.

ignorespec = pathspec.PathSpec.from_lines('gitignore', gitignore_fh)

def _exclude_tar(filepath):
# Conditionally exclude files based on the pathspecs mentioned
# in the `.gitignore` file.
# Note that, the behaviour of this function is not **exactly**
# same to the way `git` excludes files based on `.gitignore`.
#
# https://github.com/cpburnz/python-path-specification/issues/19
return ignorespec.match_file(filepath)
Copy link
Member

Choose a reason for hiding this comment

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

It concerns me a bit that this isn't exact. Is there any possibility that we could end up excluding files that shouldn't be excluded, or does the bug you link exclusively mean that we will include some files we maybe shouldn't?

Copy link
Author

@spMohanty spMohanty Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@minrk : pathspecs was the only pure python implementation I could find. Also, I think it does implement the pathspecs mostly correctly, but the difference comes from how git is a bit more liberal. For example, git is okay with referencing D/* to refer to all the subdirectories of D, whereas pathspecs expect you to write just D/.

In the issue I reported, and in most of the use cases, this will only mean we will forget to exclude some files which were meant to be excluded.

The only use case when a file that is not meant to be excluded is excluded, is when .gitignore specifically ignores a particular file, but the user had force-pushed the file to the repository.

But in the both these use cases, the .gitignore should ideally be modified to include the exceptions. For example, when you force-push say a *.csv file which was ignored by .gitignore, then you are expected to also remember to add a !<path_to_file> to .gitignore.


tar.add('.', 'src/', exclude=_exclude_tar, filter=_filter_tar)

tar.close()
tarf.seek(0)
Expand Down
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
'escapism',
'jinja2',
'ruamel.yaml>=0.15',
'pathspec>=0.5.5',
],
python_requires='>=3.4',
author='Yuvi Panda',
Expand Down