-
Notifications
You must be signed in to change notification settings - Fork 39
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
CLI-287: add ignore
parameter toImage. add_local_dir
and Image. copy_local_dir
methods
#2636
base: main
Are you sure you want to change the base?
CLI-287: add ignore
parameter toImage. add_local_dir
and Image. copy_local_dir
methods
#2636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a drive-by; saw it's still WIP so apologies if you're thinking about these already.
b7062be
to
f87f793
Compare
…rom/cli-287-add-include_files-parameter-to-image-dir-copy-methods
@@ -23,11 +23,12 @@ | |||
from ._utils.async_utils import aclosing, async_map, synchronize_api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to make any changes to Mount
if we're planning to deprecate it as public API? I could see the argument that it would be confusing to have an inversion between the public interface / internal implementation I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the inversion is annoying but also since the ignore
callable works on Path
objects and the condition
we have for mount methods only allows str
you would basically have to wrap every ignore
to work on strings instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per @freider's comment below I opted to add a private method to the Mount
object for the image methods to use instead and leave the signature of the Mount
copy & add methods as is
…rom/cli-287-add-include_files-parameter-to-image-dir-copy-methods
a9ebd36
to
697fbf2
Compare
…rom/cli-287-add-include_files-parameter-to-image-dir-copy-methods
…r-to-image-dir-copy-methods
ignore
parameter toImage. add_local_dir
and Image. copy_local_dir
methods
from .pattern_matcher import PatternMatcher | ||
|
||
|
||
class LocalFileFilter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be called "IgnorePatterns" or something, and make it clear in that it excludes on True
values. The word "Filter" and some of these docstrings make it sound like its the other way around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this class in itself doesn't exclude or include specifically though, I guess that's more how it's used in the Image
methods, but I'm open to renaming it, but maybe something like FilePathPatternMatcher
or something to show that it's related to our PatternMatcher
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a little confused about why we need to be wrapping our own PatternMatcher
object here
ignore
parameter to ourImage
add_local_dir
andcopy_local_dir
methods. It is similar to thecondition
method onMount
methods but instead operates on aPath
object. It takes either a list of string patterns to ignore which follows thedockerignore
syntax implemented in ourPatternMatcher
class using a newLocalFileFilter
class which wraps around it, or you can pass in a callable which allows for more flexible selection of files.Usage:
which will add the
./local-dir
directory to the image but ignore all files except.txt
files