-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
tarutil: allow file names to be specified by regexp #749
Conversation
Some features do not exist in set locations, but can be anywhere in the layer. This allows those featurefmt to specify the filenames to be scanned by regexp, as opposed to purely by path prefix. If any current users of this express paths which use regexp special characters this could be a breaking change for them (with the exception of . which will continue to work as it matches against the literal character .), however none of the code in this repo does that. fixes #456
49bc84e
to
afd7fe2
Compare
pkg/tarutil/tarutil.go
Outdated
@@ -78,7 +80,7 @@ func ExtractFiles(r io.Reader, filenames []string) (FilesMap, error) { | |||
// Determine if we should extract the element | |||
toBeExtracted := false | |||
for _, s := range filenames { | |||
if strings.HasPrefix(filename, s) { | |||
if match, err := regexp.MatchString("^"+s, filename); err == nil && match { |
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.
This leading ^
is implicit behavior and seems unnecessary.
If we have to change other files, I don't mind.
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.
The ^
does change the semantics (https://play.golang.org/p/yZl7bJLzr22) - I included it to keep the behaviour in line with the previous behaviour of only checking for the prefix.
I don't think there are any changes needed to the other files as the other file definitions were simple prefixes and didn't contain any special characters, it was more a headsup in case there were other subtle implications.
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.
Since we're moving to use the regex, I think it makes sense to enforce all the implementations with RequiredFile
implementation to use regex.
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.
okay, I've removed the ^
and updated all users to have a regexp instead in order to maintain previous behaviour
2abb1e5
to
497fa2b
Compare
This removes the previous behaviour from tarutil to do simple prefix matching. All places where the previous prefix-based matches were specified have been updated to use a regexp instead, maintaining previous behaviour.
497fa2b
to
a3a3707
Compare
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.
LGTM! Thanks for this change.
Some features do not exist in set locations, but can be anywhere in the layer.
This allows those featurefmt to specify the filenames to be scanned by
regexp, as opposed to purely by path prefix.
If any current users of this express paths which use regexp special characters
this could be a breaking change for them (with the exception of . which will
continue to work as it matches against the literal character .), however
none of the code in this repo does that.