Implement file action remove for wildcards #1233
Conversation
|
Please sign your commits following these rules: $ git clone -b "rm-wildcard" [email protected]:hinshun/buildkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
|
solver/llbsolver/file/backend.go
Outdated
| } | ||
|
|
||
| for _, s := range m { | ||
| p, err := fs.RootPath(d, filepath.Join(filepath.Join("/", s))) |
There was a problem hiding this comment.
Maybe create an extra function for this to avoid repetition with non-wildcard path.
|
@hinshun Add a capability for this as well https://github.com/moby/buildkit/blob/master/solver/pb/caps.go so support for it can be detected. We could also error out if the capability does not exist like https://github.com/moby/buildkit/blob/master/client/llb/exec.go#L150 but probably not that important as this shouldn't be widely used atm. |
solver/llbsolver/file/backend.go
Outdated
| @@ -204,6 +225,10 @@ func cleanPath(s string) string { | |||
| return s2 | |||
| } | |||
|
|
|||
| func rootPath(root, src string) (string, error) { | |||
There was a problem hiding this comment.
I thought a function that also calls os.RemoveAll and does the action.AllowNotFound check.
There was a problem hiding this comment.
When AllowWildcard is true and is removing each of the matched paths, I don't think AllowNotFound makes sense there?
There was a problem hiding this comment.
It should be a no-op for that case, assuming that directory is immutable. If it is not behavior can be undefined.
There was a problem hiding this comment.
If you want to keep the different AllowWildcard behavior, feel free to revert this as a single line helper function doesn't really improve this code, just makes is more complicated to read through.
There was a problem hiding this comment.
I refactored it as suggested 👍
|
Looking into the caps change. |
hinshun
force-pushed
the
rm-wildcard
branch
2 times, most recently
from
dc64d2a to
b8829fd
Compare
Signed-off-by: Edgar Lee <[email protected]>
Fixes #1232 .
Where should tests go? In
client/llb/fileop_test.go?