volume: move validating volume dest from client to server. #11231
Conversation
[NO TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>
|
@rhatdan PTAL |
libpod/options.go
Outdated
| @@ -1456,6 +1456,10 @@ func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption { | |||
| return errors.Wrapf(err, "error processing options for named volume %q mounted at %q", vol.Name, vol.Dest) | |||
| } | |||
|
|
|||
| if err = parse.ValidateVolumeCtrDir(vol.Dest); err != nil { | |||
| return errors.Wrapf(err, "error validating destination path for named volume %q mounted at %q", vol.Name, vol.Dest) | |||
There was a problem hiding this comment.
This looks like you created a stutter. Does vol.Dest get mentioned in the err twice?
Don't start error messages with the word "error" otherwise they are printed on error with
Error: error ...
I know there are other errors in options.go with this problem, please fix them while you are at it.
I would like to eventually go through the error messages in all of podman and remove any that begin with "error".
Also see what this error message looks like when it happens. Is this information the user needs or does it hide the error amongst a flood of words.
There was a problem hiding this comment.
Yes this adds stutter i verified let me fix this :)
There was a problem hiding this comment.
@rhatdan resolved for options.go i'll create a seperate PR cleaning this in entire project.
openshift-ci
bot
added
the
approved
libpod/options.go
Outdated
| } | ||
|
|
||
| if err = parse.ValidateVolumeCtrDir(vol.Dest); err != nil { | ||
| return errors.Wrapf(err, "validating destination path for named volume %q mounted at %q", vol.Name, vol.Dest) |
There was a problem hiding this comment.
I think this will still stutter vol.Dest?
There was a problem hiding this comment.
oh my bad let me fix that i missed it.
There was a problem hiding this comment.
@rhatdan Fixed but there is more Error: error stutter coming from libpod/runtime_ctr.go should i fix that in the same PR ?
There was a problem hiding this comment.
No let's move that to a separate bigger PR.
flouthoc
force-pushed
the
move-volume-dest-to-server
branch
from
ad104e1 to
065d6a1
Compare
|
Fixing failing tests |
flouthoc
force-pushed
the
move-volume-dest-to-server
branch
5 times, most recently
from
c27ee10 to
0b9b3d6
Compare
|
@flouthoc a lot of red angry tests still |
flouthoc
force-pushed
the
move-volume-dest-to-server
branch
from
0b9b3d6 to
de742ec
Compare
There was a problem hiding this comment.
@mheon @TomSweeneyRedHat @rhatdan made some significant changes as compared to yesterday's code. Could I please get a review again ?
pkg/specgen/generate/storage.go
Outdated
| if _, ok := unifiedMounts[m.Destination]; !ok { | ||
| unifiedMounts[m.Destination] = m | ||
| if err = parse.ValidateVolumeCtrDir(m.Destination); err != nil { | ||
| return nil, nil, nil, errors.Wrapf(err, "validating destination path for common mount %q", m) |
There was a problem hiding this comment.
The output of this error is going to be hard to interpret by the user? The m struct for the user is going to be ugly. Does the user need it, and does the user know what a "common mount" is?
flouthoc
force-pushed
the
move-volume-dest-to-server
branch
from
de742ec to
2f6153a
Compare
[NO TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>
flouthoc
force-pushed
the
move-volume-dest-to-server
branch
from
2f6153a to
3cee855
Compare
pkg/specgen/generate/storage.go
Outdated
| return nil, nil, nil, err | ||
| } | ||
| cleanDestination := filepath.Clean(v.Destination) | ||
| if _, ok := unifiedOverlays[cleanDestination]; ok { |
There was a problem hiding this comment.
Logic here is wrong - the ok is flipped, needs to be !ok. This predates your changes, but we might as well fix it now.
There was a problem hiding this comment.
@mheon this was already here before my pr, should i create a separate PR after this so its easier to revert isolated change ? . WDYT ?
There was a problem hiding this comment.
Same PR but separate commit is fine
There was a problem hiding this comment.
@mheon please take a look , added a seperate commit.
[NO TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, 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 |
|
Am I right that this is not part of 3.4.4? |
|
It should be. |
github-actions
bot
added
the
locked - please file new issue/PR
Closes: #10900
Move validating destination path for volume from client to server side.