Provide Unison sync support for projects by febbraro · Pull Request #32 · phase2/rig

febbraro · 2017-05-17T18:00:10Z

This PR provides 2 commands, project sync:start and project sync:stop.

These commands will discover the volume name in the following order of fallback.

Argument passed to the command
Configuration in local project .outrigger.yml file (format of config to follow)
Inspecting the docker-compose.yml file (we need to figure out if this file should be configurable, I think it will have to be)
The name of the current directory

The sync command has configuration that can be provided in the .outrigger.yml file in he following format.

version: "1.0"
sync:
  volume: project-sync
  ignores:
    - "Name .gitignore"
    - "Path node_modules"

The ignores follow the unison ignore patterns. http://www.cis.upenn.edu/~bcpierce/unison/download/releases/stable/unison-manual.html#ignore

…launch and checking for readiness

…t in doctor

…rrrr

…lural made sense to do that in the first palce, but commands like project sync:start / sync:stop make sense to live in the same struct

…arge codebases

grayside

Overall this looks like a strong starting place, and I think we could probably move forward on this basis. I had a lot of comments, some petty, some about making this more robust or expanding on the DX. Those could be captured as follow-ups for the most part.

Once we merge this, we should make sure all further changes to the rig project branch are added by PRs, as I think that existing branch will become too unwieldy to review.

grayside · 2017-05-17T18:16:22Z

brew-publish.sh

  depends_on "docker-compose"
  depends_on "docker-machine-nfs"
+  depends_on "unison"
+  depends_on "eugenmayer/dockersync/unox"


I thought we were avoiding a dependency on docker-sync?

It's not really dockersync, it is just a utility that does the fswatching that he put in a brew tap called dockersync. We are not actually using dockersync

grayside · 2017-05-17T18:18:52Z

cli/commands/project.go

 	"strings"

-	"github.com/phase2/rig/cli/commands/project"
+  "github.com/phase2/rig/cli/commands/project"


Looks like a tab vs/ spaces issue. Please tell me you are setup to use spaces? 😄

Should be spaces, but mostly I just run go fmt and commit. Should I study that more?

grayside · 2017-05-17T18:20:33Z

cli/commands/project.go

 	}

-	return command
+	syncStart := ProjectSync{}


Might be slightly more clear to name this syncDefinition

Agreed, should change.

grayside · 2017-05-17T18:20:51Z

cli/commands/project/project-config.go


+type ProjectSync struct {
+	Volume    string
+	Ignore 		[]string


grayside · 2017-05-17T18:22:18Z

cli/commands/project_sync.go

@@ -0,0 +1,235 @@
+package commands


Should we maybe group all the project things to the project sub-package?

There is really no concept of sub packages. They are all "Standalone" I did this previously, but since there is a BaseCommand we extend, I then got an error with cyclical imports. It was a mess, but no way around it. Since there are not subpackages it seems like it would just be easier if project_config.go was just moved into the main commands package.

grayside · 2017-05-17T18:27:58Z

cli/commands/project_sync.go

+    "-v", fmt.Sprintf("%s:/unison", volumeName),
+    "-e", "UNISON_DIR=/unison",
+    "-l", fmt.Sprintf("com.dnsdock.name=%s", volumeName),
+    "-l", "com.dnsdock.image=volume.outrigger",


I haven't seen a period inside a dnsdock image designation before. Is that allowed?

Yeah it is. Turns out we dont ever use that domain name though, so really it can just go away. I inspect the containers and use the IP address due to that cgo issue we rolled back

Is there some benefit to surfacing a domain to developers, e.g., for troubleshooting purposes?

I guess it is easier if folks want to point a local run unison process at it for some reason, but I dunno, it maybe just confusing, or it may just help people understand what thecontainer is even there for, Thoughts?

grayside · 2017-05-17T18:29:09Z

cli/commands/project_sync.go

+    "-l", fmt.Sprintf("com.dnsdock.name=%s", volumeName),
+    "-l", "com.dnsdock.image=volume.outrigger",
+    "--name", volumeName,
+    "outrigger/unison:latest",


Given unison needs the client and server on a fairly exact version, should we add a very specific Docker Tag to our image and then derive the tag based on the detected version available in the local system? That's a good opportunity to catch an error and provide guidance.

Good idea. I'll see what I can do.

grayside · 2017-05-17T18:31:29Z

cli/commands/project_sync.go

+}
+
+// Load the proper compose file
+func (cmd *ProjectSync) LoadComposeFile() (*ComposeFile, error) {


Custom parsing the compose file means any of the docker-compose magic such as variable handling will not be available. Would it be worth seeing if we can add a dependency on a golang library for this? I believe docker/libcompose is deprecated but maybe there's something else we can use?

I tried. Non of the libraries include either the most recent version of compose (v3) or the "overriding rules" that come from multiple files. They just dont have compose file parsing such that it is a library to use at the moment. Happy to transition over to one when it exists though.

grayside · 2017-05-17T18:32:15Z

cli/commands/project_sync.go

+
+// Load the proper compose file
+func (cmd *ProjectSync) LoadComposeFile() (*ComposeFile, error) {
+  yamlFile, err := ioutil.ReadFile("./docker-compose.yml")


So this "fallback" doesn't work for the build container?

Well, it doesn't inspect the build.yml file, but both of them need the same exact volumes spec in their files.

grayside · 2017-05-17T18:32:33Z

cli/commands/project_sync.go

+
+// Load the proper compose file
+func (cmd *ProjectSync) LoadComposeFile() (*ComposeFile, error) {
+  yamlFile, err := ioutil.ReadFile("./docker-compose.yml")


Should we respect the COMPOSE_FILE env var, or is that just helpful enough to be confusing?

Maybe confusing. Also if we do the docker-compose.overrride.yml thing as our "Dev" env we'd actually need to read that file instead.

mike-potter · 2017-05-17T20:23:07Z

cli/commands/project_sync.go

+		"-repeat", "watch",
+		"-prefer", ".",
+		"-logfile", logFile,
+		"-ignore", "Name .git",


Should this be "Name .git" or "Path .git"?

Nevermind, "Name .git" is correct.

grayside · 2017-05-17T22:00:33Z

cli/commands/project_sync.go

+		"-repeat", "watch",
+		"-prefer", ".",
+		"-logfile", logFile,
+		"-ignore", "Name .git",


Shouldn't there be some processing to combine this with the sync configuration for ignores I see in the project.ProjectSync struct?

grayside · 2017-05-17T22:01:50Z

cli/commands/project_sync.go

+		cmd.out.Error.Println(err)
+	}
+
+	cmd.out.Error.Fatal("Unable to determine a name for the sync volume")


This skates by review, but once we have a docs page for this we should link to it here.

… the sync volume

febbraro and others added 12 commits May 12, 2017 13:30

parsing compose files for volumes that are external and end in -sync

96cf46d

Added a few more steps of unison container setup

ad079f9

refactored start sync into a separate file, implemented local unison …

7318308

…launch and checking for readiness

Added dependencies on unison and unox

cfe8244

added support for setting filesystem configuration and checking for i…

615d281

…t in doctor

pulled out all libdocker stuff, just did not seem to be working. Grrr…

5201728

…rrrr

Added stop and next iterations of unison process

c72062c

early config for flexible ignores

02b97d7

update the Commands() function to returns an array of commands. The p…

de7a532

…lural made sense to do that in the first palce, but commands like project sync:start / sync:stop make sense to live in the same struct

More tweaking for OSX file system limits

a64664d

Updated setting machine kernel config, also allows for verbose mode

23fc293

Finalized the proper vm/local settings needed for unison to work on l…

d5084b2

…arge codebases

febbraro requested review from grayside, mike-potter and tekante May 17, 2017 18:00

febbraro added the in progress label May 17, 2017

grayside reviewed May 17, 2017

View reviewed changes

febbraro added 3 commits May 17, 2017 12:39

renamed sync variable

3205a54

added sync as an alias to sync:sCtart

6b97d6b

go fmt

94f37dc

mike-potter reviewed May 17, 2017

View reviewed changes

febbraro added 2 commits May 17, 2017 13:38

Launch a minor version compatible unison container

84ef708

clarified error message

a8b069b

grayside reviewed May 17, 2017

View reviewed changes

febbraro added 2 commits May 17, 2017 15:04

ENsured the last fallback added the suffix -sync to the name used for…

e3ad267

… the sync volume

Removed ignore on .git, we do git things in build containers sometimes

04712cd

grayside approved these changes May 17, 2017

View reviewed changes

febbraro merged commit a4b3693 into feature/rig-project May 17, 2017

febbraro removed the in progress label May 17, 2017

febbraro deleted the feature/unison-sync branch May 17, 2017 22:17

Conversation

febbraro commented May 17, 2017

Uh oh!

grayside left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants