feat: option to have File store preserve permissions when extracting #891
Conversation
sammy-da
force-pushed
the
file-store-preserve-perms
branch
3 times, most recently
from
9f93f59 to
f9f1baa
Compare
sammy-da
requested review from
sajayantony,
shizhMSFT,
SteveLasker and
Wwwsylvia
as code owners
sammy-da
force-pushed
the
file-store-preserve-perms
branch
3 times, most recently
from
454df67 to
830c0a9
Compare
Signed-off-by: Sammy Abed <sammy.abed@digitalasset.com>
sammy-da
force-pushed
the
file-store-preserve-perms
branch
from
830c0a9 to
0b55f28
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (72.72%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #891 +/- ##
==========================================
- Coverage 80.20% 80.19% -0.02%
==========================================
Files 63 63
Lines 6043 6053 +10
==========================================
+ Hits 4847 4854 +7
- Misses 858 860 +2
- Partials 338 339 +1 ☔ View full report in Codecov by Sentry. |
sammy-da
force-pushed
the
file-store-preserve-perms
branch
from
3a904ee to
bafe923
Compare
sammy-da
force-pushed
the
file-store-preserve-perms
branch
from
bafe923 to
5560c4d
Compare
content/file/errors.go
Outdated
| @@ -23,6 +23,7 @@ var ( | |||
| ErrPathTraversalDisallowed = errors.New("path traversal disallowed") | |||
| ErrOverwriteDisallowed = errors.New("overwrite disallowed") | |||
| ErrStoreClosed = errors.New("store already closed") | |||
| ErrPreservePermissions = errors.New("failed to restore permission during unpacking") | |||
There was a problem hiding this comment.
Since this error is file store specific, it should be put the file package.
BTW, should it be "restore permissions" in the error message?
There was a problem hiding this comment.
this is the file package. (?)
BTW, should it be "restore permissions" in the error message?
yep, typo
There was a problem hiding this comment.
btw, not sure how to increase the code coverage for this.
To do that, i think i would need to test the chmod error case. It isn't obvious to me how to trigger such failure, other than by concurrently trying to delete a file before it gets chmod'ed, i.e. trigger a race, but that would be flaky test
There was a problem hiding this comment.
this is the file package. (?)
Oh sorry I was blind😂
Yeah it would be very tricky to test the chmod error case... I think we can bypass it. /cc @shizhMSFT
sammy-da
force-pushed
the
file-store-preserve-perms
branch
3 times, most recently
from
28c3052 to
26443ab
Compare
Co-authored-by: Lixia (Sylvia) Lei <lixlei@microsoft.com> Signed-off-by: sammy-da <sammy.abed@digitalasset.com> Signed-off-by: Sammy Abed <sammy.abed@digitalasset.com>
sammy-da
force-pushed
the
file-store-preserve-perms
branch
from
26443ab to
9bf6f31
Compare
Wwwsylvia
changed the title
| @@ -476,3 +476,39 @@ func TestStore_Dir_OverwriteSymlink_RemovalFailed(t *testing.T) { | |||
| t.Fatal("error calling Chmod(), error =", err) | |||
| } | |||
| } | |||
|
|
|||
| func Test_extractTarDirectory_PreservePermissions(t *testing.T) { | |||
There was a problem hiding this comment.
Forgot to mention, since this function is defined in utils.go, it's better to test it in utils_unix_test.go instead of file_unix_test.go.
A side note: we need to include the oras copyright header at the beginning of each file.
| // Restore full mode bits | ||
| if preservePermissions && (header.Typeflag == tar.TypeReg || header.Typeflag == tar.TypeDir) { | ||
| if err := os.Chmod(filePath, os.FileMode(header.Mode)); err != nil { | ||
| return fmt.Errorf("%w: %w", ErrPreservePermissions, err) |
There was a problem hiding this comment.
nit: I don't think we need ErrPreservePermissions. It should be fine just returning err. BTW, double %w won't work.
|
@sammy-da DCO checking is failing. Could you fix it? |
This PR takes care of #886, or rather part of it.
It adds an option to the File store that's similar to tar's
--preserve-permissions.closes #886