mount: some refactor and improved GoDoc about Windows support#31
mount: some refactor and improved GoDoc about Windows support#31kolyshkin merged 2 commits intomoby:masterfrom
Conversation
|
Relates to moby/moby#41458 (comment) ping @kolyshkin @cpuguy83 PTAL |
|
Thanks! Is it possible to add some CI job to prevent this kinds of mishap in the future? |
|
It looks like Perhaps we can make both no-op? This will
|
thaJeztah
force-pushed
the
dont_panic
branch
from
6a27b5f to
8b5e03d
Compare
|
@kolyshkin updated, but I noticed that before 0c569e0, Actually thinking now if we should accept the breaking change and update downstream code or not 🤔 (the package is still pre v1.0, so from that perspective, it should be ok) |
|
If you can just change Yet we need to decide first whether we want them both to be no-op or panic. I don't have a strong preference but would like to hear other opinions. |
|
moby/moby#41458 (comment) it seems that the panic behavior is better than no-op. In these two cases, it clearly tells the test is not working (and it should not work on Windows). So, I'm in favor of panic. |
thaJeztah
force-pushed
the
dont_panic
branch
from
8b5e03d to
b4a0e04
Compare
thaJeztah
changed the title
| // RecursiveUnmount is not implemented on Windows. | ||
| func RecursiveUnmount(target string) error { |
There was a problem hiding this comment.
So, alternatively, I guess we could not implement at all on Windows (move this to a _linux.go or _unix.go wdyt?
There was a problem hiding this comment.
Yeah, I think the rename + build flags will be slightly cleaner.
|
What I'd like to see here is a minimal patch. Moving lots of code around makes it harder to dig through git history, and unless there is a good reason to, I'd rather not do it. Maybe I'm wrong and this is unavoidable. |
|
The situation currently is that we have separate files for platform-specific implementations, but there's code in files that doesn't work on the platform it's enabled on. So, yes, I agree that a smaller diff would be good, but it looks like code is in the wrong files. |
thaJeztah
force-pushed
the
dont_panic
branch
from
b4a0e04 to
7e37eaf
Compare
|
I see @kolyshkin preferred "Yeah, I think the rename + build flags will be slightly cleaner."; let me have a look what that looks like. |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
dont_panic
branch
2 times, most recently
from
585c7f1 to
832bd36
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
@kolyshkin pushed a commit with those changes; if you think this looks good, I'll squash |
thaJeztah
changed the title
3 participants
Commit 0c569e0 (#26) added a fast-path for Linux, but inadvertendly changed the behavior on Windows, which previously handled RecursiveUnmount() as a no-op, and now panicked.This patch makes RecursiveUnmount() a no-op again on Windows.[Kir: fixed commit/PR reference]