[WIP] feat(cli): experimental go frontend CLI #329

edvald · 2018-10-12T17:30:22Z

This is just a step towards the Go CLI. We should be able to merge this without any impact on the current way of using garden, but since this introduces some changes to the build flows we may want to review and merge this.

To get this working, you need to do the following:

Move your garden repo directory to $GOPATH/src/github.com/garden-io/garden
Install dep (brew install dep)
Run dep ensure in your repo directory
Run gulp build to build the CLI

You may then want to alias garden to the built binary (garden-cli/build/garden) when developing.

This is all highly experimental, there are several steps remaining until we get to our final destination of having a single-binary installation.

edvald · 2018-10-23T09:09:11Z

garden-cli/docker/sync/Dockerfile

+
+RUN mkdir host-mount
+
+CMD cp -r host-mount/. project/ \


If the volume already exists, this may fail. We could also do an initial rsync between the directories here (or if unison has a "one-way" sync command, that would also work).

Is it possible for the mount to not exist before the container starts up? Never seen that.

Also can we use absolute paths /host-mount it makes it more explicit. (although paths are relative to the working directly which would be / by default.

garden-cli/docker/sync/Dockerfile

edvald · 2018-10-23T09:11:25Z

garden-cli/dockerutil/docker.go

+	args := []string{"volume", "inspect", volumeName}
+	err := Exec(args, true)
+	if err != nil {
+		return false


Probably want a TODO here to distinguish between different potential errors.

edvald · 2018-10-23T09:13:45Z

garden-cli/main.go

+		err = runSyncContainer(projectName, gitRoot, relPath)
+		if err != nil {
+			os.Exit(1)
+		}


Probably better to just do check(err) here, or use log.Fatal(<some clearer message>). Same below.

Also, we'd need to make sure the initial copy is complete before running the service below. We may need to run the sync container once with the copy/rsync command and wait for that to complete, before starting the unison process.

edvald · 2018-10-23T09:19:13Z

garden-cli/run.go

-			// Mount the project directory.
-			// TODO: sync into a garden-sync container instead
-			"--volume", fmt.Sprintf("%s:/project:delegated", gitRoot),
+			"--volume", fmt.Sprintf("%s/.git:/root/garden/.git", gardenHomeDir),


Hmm, this doesn't seem right. ~/.garden is not supposed to contain a git root.

Should this be /root/.git it's the users git config (but why do we need this?)

I think it's just a mistake, we shouldn't need this at all.

Yeah, that one just slipped in accidentally. My bad.

edvald · 2018-10-23T09:27:35Z

garden-cli/docker/sync/Dockerfile

+# system dependencies
+RUN apk add --no-cache unison
+
+RUN mkdir host-mount


I suggest using absolute paths, not sure if / is guaranteed as a default working dir

edvald · 2018-10-23T09:33:39Z

garden-cli/main.go

 	if err != nil {
 		os.Exit(1)
 	}
 }
+
+// FIXME: We need a proper way to check if the command requires file system watch. This is not it.
+func fsWatchEnabled(args []string) bool {


I'm a little skeptical about having this check. If we have a long-running sync container, it's also probably an unnecessary optimization. If we always start by doing an initial copy when creating the volume, then we should be able to assume that if the sync container is already running, that the synced volume is up to date. So we then don't have to have separate flows.

How would we like to manage the sync containers in general? Should they just run along indefinitely, even though a user might have several projects and thus several unison processes running? Or should there be some clean up mechanism?

We might want to implement some sort of timeout mechanism, so that they shut down after a certain time of inactivity. A little tricky to implement though, so I kinda filed that as an optimization we could make later. That said, the unison process shouldn't consume a lot of resources if the user isn't working on the project.

With the approach above, I figured we'd stop the sync container after each run (and perhaps clean up any dangling sync containers) and then run it again next time a command requires file system watch. That would increase the start time of long running commands but not the other ones and there would always only be one sync process running.

...on second thought, that's probably going to be too slow.

Different tradeoffs I guess, but I think the differing flows depending on the command, and the potentially long startup times are the worse choice. Especially when we'll be going over the network (although the implementation will be somewhat different then anyway).

garden-cli/sync.go

drubin · 2018-10-23T08:44:33Z

garden-cli/run.go

 )

-// TODO: we may want to use one of the docker go client libs instead of shelling out here, but this works for now so...
-func runGardenService(projectName string, gitRoot string, relPath string, args []string) error {
+func runGardenService(projectName string, gitRoot string, relPath string, watch bool, args []string) error {
 	homeDir := getHomeDir()
 	gardenHomeDir := getGardenHomeDir()
 	containerName := "garden-run-" + randSeq(6)


We should use a predictable container and if its already running kill it and restart.

This could lead to issues if there is a disconnect event between our app and docker, then there are dangling containers running.

This container isn't persistent for now. We create a new one for each command. When we have an actual running service we will change this.

yes I am suggesting we switch this out sooner rather than later. I rand into issues where things broke and I had dangling containers.

But I guess the concern does stand for watching commands... One thing we could do would be using the parent process PID instead of the random letters, that would make it easy to see which containers have an attached CLI process.

mmm we should just use a predictable name based on project (how ever we define that, name, path/*)

PID is pretty much random too. I am suggesting we need idempotency across runs (i.e command stopping and restarting)

Ah sorry I totally miss understood this. I assumed when we ran all the commands they check for an existing container and exec into it.

Why do we do this instead of just starting up a container(if not already running) and exec'ing into it ? It seems like a simpler considering we are trying to watch events and state?

This would mean you would be able to behave more like client-> server model (albeit still with the exec instead of api)

We might do that as an optimization actually - should for sure be faster to start. Just wanted to get this working and then make that one of the next steps.

I believe it will result in simpler easier to maintain code that is easier to develop on. It's also how I believed we did it originally.

It also means we can test things with simple docker exec -it as a rapid verification method.

We did plan on doing it that way, just haven't gotten there yet. @eysi09 up to you if we change it in this PR or later.

garden-cli/run.go

edvald · 2018-10-23T12:35:06Z

.circleci/config.yml

            sudo apt-get update
-            sudo apt install rsync cmake libicu-dev pkg-config golang-1.11-go
+            sudo apt -y install software-properties-common


This installs a lot of stuff, maybe we can go a cheaper route with this. @drubin can you help us get the CI passing here? 👼

I am busy fixing this actually on my Fork drubin#1.

I am splitting every thing out so we can also leverage caching and multi jobs running at the same time, it will simplify all this if we can actually leverage the pre-built images for what they were meant for.

If we need the CLI to pass I can always backport the building configs into this branch? but we still actively trying to fix a bunch of things so thought it could wait a few hours?

Ok great. Just wanted to see if the flag above would get it running.

Oh no stress, let's just wait then until you're done with your thing. Btw, very happy about everything you just said .)

I think we should leave this for now. No point trying to optimise it further when we are going to replace it all any way. if you add another sudo apt-get update after add-apt-repository it should pass.

drubin · 2018-10-23T12:41:30Z

.circleci/config.yml

+            sudo apt-get update
+            sudo apt -y install software-properties-common
+            # Need software-properties-common for the add-apt-repository command
+            sudo add-apt-repository -y ppa:gophers/archive


You need another sudo apt-get update

drubin · 2018-10-25T11:47:21Z

garden-cli/kubernetes/bootstrap.go

@@ -0,0 +1,255 @@
+package kubernetes


Can we delete this file? There is no need to merge a commented out file?

drubin · 2018-10-25T11:50:48Z

garden-cli/Dockerfile

@@ -0,0 +1,7 @@
+FROM alpine:3.8


Do you think we can move this out to garden-sync before merging? If its too much effort we can do it after.

I think it would clean things up technically we wont publish this as garden-cli its just a dependancy that the go cli will use just like garden-service?

+1 on that. These are basically two separate modules with different code bases.

drubin

LGTM, after a rebase

edvald changed the title ~~feat(cli): experimental go frontend CLI~~ [WIP] feat(cli): experimental go frontend CLI Oct 12, 2018

edvald force-pushed the go-cli-pt1 branch 10 times, most recently from a1fc5d1 to 5b78a77 Compare October 12, 2018 18:25

eysi09 force-pushed the go-cli-pt1 branch 5 times, most recently from 37d11f7 to 0991b52 Compare October 23, 2018 09:03

edvald commented Oct 23, 2018

View reviewed changes

garden-cli/docker/sync/Dockerfile Outdated Show resolved Hide resolved

edvald commented Oct 23, 2018

View reviewed changes

drubin reviewed Oct 23, 2018

View reviewed changes

edvald commented Oct 23, 2018

View reviewed changes

drubin reviewed Oct 23, 2018

View reviewed changes

drubin mentioned this pull request Oct 24, 2018

chore(ci): Clean up circleci configs #339

Merged

drubin force-pushed the go-cli-pt1 branch from 772d6ef to 04a4e1f Compare October 24, 2018 10:51

drubin reviewed Oct 25, 2018

View reviewed changes

eysi09 force-pushed the go-cli-pt1 branch 3 times, most recently from eb65c98 to a9644b3 Compare October 25, 2018 12:05

eysi09 approved these changes Oct 25, 2018

View reviewed changes

drubin approved these changes Oct 25, 2018

View reviewed changes

feat(cli): experimental go frontend CLI

71c5e38

eysi09 force-pushed the go-cli-pt1 branch from a9644b3 to 71c5e38 Compare October 25, 2018 12:28

eysi09 merged commit 8ab5846 into master Oct 25, 2018

eysi09 deleted the go-cli-pt1 branch October 25, 2018 12:32

[WIP] feat(cli): experimental go frontend CLI #329

[WIP] feat(cli): experimental go frontend CLI #329

Conversation

edvald commented Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drubin left a comment

Choose a reason for hiding this comment

edvald commented Oct 12, 2018 •

edited

Loading