-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add linux bind volume support #73
Conversation
cli/commands/project_sync.go
Outdated
cmd.out.Verbose.Printf("Setting up local volume: %s", volumeName) | ||
cmd.SetupBindVolume(volumeName) | ||
default: | ||
cmd.out.Verbose.Printf("Volume Sync not supported on: %s", platform) |
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.
Does unison not work on Windows?
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.
Don't know, we have not tried it yet, but at a minimum the binary would likely be something like unison.exe
so our current command would not be supported. I think windows also does cli options differently.
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.
Ah, so currently we are in aggressive conflict with Windows. Perhaps in the recent change to generator-outrigger-drupal, I should have it use the "devcloud" options for non-Darwin, instead of for linux specifically.
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.
I'm confused what hole are we leaving Windows users in? Is it "Add Unison support on Windows" or is it "Treat Windows like Linux"?
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.
Windows support would be "Treat Windows like Mac"
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.
Oh, I see what you mean.... let's default to what mac does and lets just use Linux as the single case.
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.
I like this approach.
Moved from WIP to legit PR. Verified |
cli/commands/project_sync.go
Outdated
cmd.out.Verbose.Printf("Setting up local volume: %s", volumeName) | ||
cmd.SetupBindVolume(volumeName) | ||
default: | ||
cmd.out.Verbose.Printf("Volume Sync not supported on: %s", platform) |
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.
I'm confused what hole are we leaving Windows users in? Is it "Add Unison support on Windows" or is it "Treat Windows like Linux"?
"volume", "create", | ||
"--opt", "type=none", | ||
"--opt", fmt.Sprintf("device=%s", workingDir), | ||
"--opt", "o=bind", |
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.
To be clear, the lack of specifying ro meant this is a read/write binding?
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.
Are you suggesting that we explicitly mark it :rw
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 confirming its rw
.
func (cmd *ProjectSync) RunStop(ctx *cli.Context) error { | ||
if runtime.GOOS == "linux" { | ||
cmd.out.Info.Println("No unison container to stop, using local bind volume") |
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.
Since we are creating the volume as an external, shouldn't this stop the volume for them?
And if so, as long as any action is taken I think logging about no-op should be verbose.
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.
Volumes are not start or stopped, they just exist. Are you suggesting we do the volume remove here in the stop?
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.
Nope, you are right, we should probably not do a volume remove here.
Some exploration into having linux use bind volume mounts (instead of unison) so that we don't need different configs for volumes on mac & linux