-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix permissions on initially created named volumes #10531
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@mheon PTAL |
0db10db
to
4a48ed1
Compare
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.
Just nits, code LGTM.
Does it really fully fix #10188? It looks like there's something in Buildah as well.
libpod/container_internal.go
Outdated
@@ -1734,6 +1734,15 @@ func (c *Container) chownVolume(volumeName string) error { | |||
if err := os.Lchown(mountPoint, uid, gid); err != nil { | |||
return err | |||
} | |||
|
|||
st, err := os.Lstat(filepath.Join(c.state.Mountpoint, v.Dest)) |
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.
Could you add a comment here? Maybe also mention #10188 for historical reasons.
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.
Sure,
@@ -904,6 +904,18 @@ USER bin`, BB) | |||
Expect(session.ExitCode()).To(Equal(100)) | |||
}) | |||
|
|||
It("podman run with named volume", func() { |
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.
Can you drop a comment that points to #10188?
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.
Why, this fixes the permissions of the volume created, I don't believe we care about the timestamps.
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.
Ok It now handled timestamps.
2491338
to
4581de8
Compare
All kinds of test unhappiness @Rhadan |
668c6df
to
b37ccb2
Compare
Fixes: #10606 |
LGTM |
Permission of volume should match the directory it is being mounted on. Fixes: containers#10188 Signed-off-by: Daniel J Walsh <[email protected]>
Permission of volume should match the directory it is being mounted on.
Fixes: #10188
Signed-off-by: Daniel J Walsh [email protected]