Allow for binding a named pipe on Windows #2661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
from
38ec12d
to
e70c97d
Compare
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
3 times, most recently
from
edf558a
to
8fd5fe0
Compare
aznashwan
suggested changes
Nov 28, 2023
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
3 times, most recently
from
48b3686
to
9341076
Compare
aznashwan
suggested changes
Dec 4, 2023
There was a problem hiding this comment.
Second review pass done with some just some minor nits.
Currently unsure why the Linux mount-related tests are failing, recommend adding extra logging to the changed functions and maybe even the testutils.AssertXYZ() helper functions to further aid debugging.
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
from
f15fff1
to
313119a
Compare
profnandaa
reviewed
Dec 8, 2023
| if !os.IsNotExist(err) { | ||
| return fmt.Errorf("failed to stat %q: %w", src, err) | ||
| } | ||
| if err := os.MkdirAll(src, 0o755); err != nil { |
There was a problem hiding this comment.
not sure about the 755 permission, is it the least needed for everything to work -- seems a bit too much, anything in the 6xx ranges?
There was a problem hiding this comment.
Just moved this part of the code. But read and execute permission on the dir for users other than the owner seems ok.
@AkihiroSuda, should we change the permission?
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
from
e6a2d75
to
4ba6432
Compare
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
4 times, most recently
from
3c7843e
to
c0f1514
Compare
jsturtevant
reviewed
Dec 13, 2023
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
8 times, most recently
from
18cda77
to
fc5dae0
Compare
|
Needs rebase |
- Allow users to bind a named pipe on Windows
- Refactor windows volume spec split
- Split mount code for Linux and Windows environments
- Adds a check in create.go#L298 to log a warning only when there is an error
Signed-off-by: Christine Murimi <mor.tina@outlook.com>
TinaMor
force-pushed
the
fix-windows-volume-mount
branch
from
fc5dae0
to
e4bf8b9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
This PR fixes:
Also, adds a check in create.go#L298 to log a warning only when there is an error