-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add Windows file ownership code #138
Add Windows file ownership code #138
Conversation
b378366
to
1ed2012
Compare
Sorry about the previous omission. Tests should pass now. |
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.
Not a windows guy, but SGTM
copy/mkdir_windows.go
Outdated
} | ||
var userSID *windows.SID | ||
if err := windows.ConvertStringSidToSid(sidPtr, &userSID); err != nil { | ||
return fmt.Errorf("converting to windows SID: %w", err) |
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.
Nit: use errors.Errorf
so the stacktrace does not get lost.
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.
@gabriel-samfira Could you fix this small issue and rebase. I think we can merge this then.
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.
Yes. Doing it ASAP.
Side note: will also have a new PR soon with fixes for windows across the rest of the repo and a workflow to test on windows.
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.
Done.
@gabriel-samfira Could you also look into setting up actual windows CI workflow for this repo? |
@tonistiigi Sure! Will have a look this week. |
@tonistiigi Added an issue to track the Windows CI workflow here #142 |
1ed2012
to
27d81c4
Compare
This change adds the ability to copy Windows file ownership and security information, as well as implement proper Chown(). Signed-off-by: Gabriel Adrian Samfira <[email protected]>
27d81c4
to
b277694
Compare
This change adds the ability to copy Windows file ownership and security information, as well as implement proper Chown() for Windows.
Signed-off-by: Gabriel Adrian Samfira [email protected]