copier: add RemoveOptions.AllowWildcard#6827
Conversation
Add AllowWildcard as a boolean field to copier.RemoveOptions. When set, the path is expanded as a glob pattern and each match is removed. Signed-off-by: Fatih Akca <fatihakca5@gmail.com>
akca
force-pushed
the
opfile-allowwildcard
branch
from
d683b1c to
c2719f8
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
dosubot
Bot
added
the
size:M
|
I have restarted the failed check, which didn't appear related to these changes. |
|
LGTM |
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
Looks about right, though I'm not clear on whether the cases where the wildcard doesn't match anything should expect to get an error back. Do we know what the expected behavior there is?
| { | ||
| name: "wildcard-literal-missing", | ||
| remove: "subdir-a/file-nonexistent", | ||
| allowWildcard: true, |
There was a problem hiding this comment.
Without allowNotFound and all set (i.e., both false), should an error be returned, causing this test to fail because fail wasn't set?
| { | ||
| name: "wildcard-no-match", | ||
| remove: "subdir-a/nonexistent-*", | ||
| allowWildcard: true, |
There was a problem hiding this comment.
Without allowNotFound and all set (i.e., both false), should an error be returned, causing this test to fail because fail wasn't set?
| targets, err = extendedGlob(req.Directory) | ||
| if err != nil { | ||
| return errorResponse("copier: remove: glob %q: %v", req.Directory, err) | ||
| } |
There was a problem hiding this comment.
I think we need to be catching cases where the returned set of targets has len() == 0 when AllowNotFound isn't set, and returning an error, as Glob() will only return an error for an invalid pattern.
AllowWildcard doesn't always mean that there's a wildcard being used, and I don't know that we want to effectively ignore not-found errors when it's set.
Checked BuildKit's implementation of the same flag (here). When Test cases here expecting success on zero matches follow that same behaviour. @nalind what do you think? |
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
Looks about right, though I'm not clear on whether the cases where the wildcard doesn't match anything should expect to get an error back. Do we know what the expected behavior there is?
Checked BuildKit's implementation of the same flag (here). When
AllowWildcardis set and the pattern matches nothing, it silently succeeds.AllowNotFounddoesn't affect that; it only controls whether individually resolved paths that don't exist are an error.
I think that's a wrinkle that would be great to note in the godoc, if only for future-me's benefit. Not a blocker, though.
Test cases here expecting success on zero matches follow that same behaviour. @nalind what do you think?
That's the use case we want to be compatible with, so the tests are good as-is.
LGTM
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds
AllowWildcardas a boolean field tocopier.RemoveOptions. When set, the path is expanded as a glob pattern and each match is removed. This aligns buildah's copier with BuildKit'sFileActionRm.AllowWildcardbehavior.How to verify it
New test cases cover:
*,?,[ab]patterns, zero matches, interaction withAllandAllowNotFoundflags.Which issue(s) this PR fixes:
Fixes #6806
Special notes for your reviewer:
Does this PR introduce a user-facing change?