ensure files from the tar stream are created with proper permissions #787
Conversation
|
@jim-minter ptal, in reference to https://issues.jboss.org/browse/CLOUD-1982?_sscc=t |
bparees
force-pushed
the
tar_permissions
branch
from
dabaf32
to
8995843
Compare
| @@ -338,6 +338,7 @@ func (t *stiTar) ExtractTarStreamFromTarReader(dir string, tarReader Reader, log | |||
| glog.Errorf("Error creating dir %q: %v", dirPath, err) | |||
| return err | |||
| } | |||
| t.Chmod(dirPath, header.FileInfo().Mode()) | |||
There was a problem hiding this comment.
note that i did this instead of changing the 0700 above because i only wanted to affect the permissions of files/dirs explicitly defined in the tar file, not parent dirs being indirectly created.
|
@rcernich fyi. after this merges we'll have to updated openshift w/ the change, it'll be part of 3.6.1 at this point. In the meantime the workaround would be to perform permission fixing in your assemble script. |
|
Awesome. Thanks @bparees! |
|
Oops :( Fix lgtm, please update the unit test. |
bparees
force-pushed
the
tar_permissions
branch
from
8995843
to
d6aa569
Compare
|
@jim-minter unit tests updated. |
bparees
force-pushed
the
tar_permissions
branch
from
d6aa569
to
9aa63b6
Compare
pkg/tar/tar_test.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func isDir(mode os.FileMode) bool { |
There was a problem hiding this comment.
There was a problem hiding this comment.
fixed. interesting that doesn't exist for symlinks.
|
hopefully it'll pass on Windows too? [test] |
bparees
force-pushed
the
tar_permissions
branch
3 times, most recently
from
60013ec
to
48860b4
Compare
pkg/tar/tar_test.go
Outdated
| if files[i].mode&0100 == 0100 { | ||
| // if the file is executable, make it executable for everyone | ||
| files[i].mode |= 0111 | ||
| } |
There was a problem hiding this comment.
@jim-minter please sanity check the above permission logic.
bparees
force-pushed
the
tar_permissions
branch
2 times, most recently
from
e3b27f1
to
a7574d4
Compare
|
assuming it passes the tests, lgtm |
bparees
force-pushed
the
tar_permissions
branch
from
a7574d4
to
6cc2f41
Compare
pkg/tar/tar_test.go
Outdated
| } | ||
| if files[i].mode.IsDir() { | ||
| // if the file is a directory, make it executable for everyone | ||
| files[i].mode |= 0111 |
There was a problem hiding this comment.
@jim-minter changed this logic to only apply to directories (not files+symlinks) based on the latest failure.
bparees
force-pushed
the
tar_permissions
branch
from
6cc2f41
to
b283c1e
Compare
|
Evaluated for source to image test up to b283c1e |
|
Source To Image Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_s2i/608/) (Base Commit: 0d91192) (PR Branch Commit: b283c1e) |
|
finally. @jim-minter please give a final review, i had to tweak the permission operations in the unit test a bit to make windows happy. |
|
lgtm, please merge |
|
[merge] let the backporting begin. |
|
Source To Image Merge Results: Waiting: You are in the build queue at position: 1 |
|
Evaluated for source to image merge up to b283c1e |
|
backport to 3.6 branch: https://github.com/openshift/source-to-image/pull/788/files |
also bug https://bugzilla.redhat.com/show_bug.cgi?id=1479130