-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
feat: use MkdirAll by OS(#19301) #19323
Conversation
|
||
// MkdirAllProvider is an interface for creating directories. | ||
type MkdirAllProvider interface { | ||
MkdirAll(path string, perm os.FileMode) error |
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 think the interface we need is func MkdirAll(root, unsafePath string, mode int) error
. The Linux implementation will just pass through the args to securejoin.MkdirAll
. The non-Linux implementation will run securejoin.SecureJoin(root, unsafePath)
and then run os.MkdirAll
.
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.
Is it correct to change the part mentioned in the issue as follows??
before
fullHydratePath, err = securejoin.SecureJoin(h.dirPath, hydratePath)
if err != nil {
return fmt.Errorf("failed to construct hydrate path: %w", err)
}
// TODO: consider switching to securejoin.MkdirAll: https://github.com/cyphar/filepath-securejoin?tab=readme-ov-file#mkdirall
err = os.MkdirAll(fullHydratePath, os.ModePerm)
after
err = MkdirAll(rootPath, hydratePath, os.ModePerm)
b56ec74
to
51a9e40
Compare
@crenshaw-dev As you said, I modified the implementation. |
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 looks nearly perfect! A few requests:
- Can we eliminate the structs and just have a plain function called
SecureMkdirAll
? I tried locally, looks like the struct isn't necessary. - Can you add super basic unit tests for both implementations?
Great! |
Signed-off-by: thisishwan2 <[email protected]>
Signed-off-by: thisishwan2 <[email protected]>
Signed-off-by: thisishwan2 <[email protected]>
Signed-off-by: thisishwan2 <[email protected]>
Signed-off-by: thisishwan2 <[email protected]>
Signed-off-by: thisishwan2 <[email protected]>
Signed-off-by: thisishwan2 <[email protected]>
59ceb85
to
54dcc1d
Compare
Signed-off-by: Michael Crenshaw <[email protected]>
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.
Thank you!!
Hello. This is a PR for issue #19301
I created an implementation to run MkdirAll by os. And I abstracted it. In hydratorhelper, I modified it to use an implementation that fits os.
Please let me know if there is anything wrong!
Checklist: