all: convert most Bash scripts to Go by ijt · Pull Request #288 · google/go-cloud

ijt · 2018-08-06T23:12:15Z

Fixes #257

ijt · 2018-08-06T23:35:32Z

This is a work in progress. So far it shows conversion of one script to get feedback, and then based on that I'll convert the remaining scripts.

zombiezen · 2018-08-07T16:09:58Z

samples/guestbook/gcp/deploy.go

+
+// The deploy program builds the Guestbook server locally and deploys it to
+// GKE.
+


Remove blank line so it becomes package documentation.

zombiezen · 2018-08-07T16:10:43Z

samples/guestbook/gcp/deploy.go

@@ -0,0 +1,159 @@
+// Copyright 2018 Google LLC


Hm. If we have multiple "scripts" like this in the samples/guestbook/gcp directory, then the Go compiler is going to complain about redeclaration of main function.

Ok, I filed golang/go#26848 about this. Meanwhile, we can add // +build ignore to scripts like these.

aaaand it's rejected.

zombiezen · 2018-08-07T16:14:28Z

samples/guestbook/gcp/deploy.go

+)
+
+func main() {
+	guestbookDir := flag.String("guestbook_dir", "..", "directory containing the guestbook example")


".." adds a new assumption around working directory that the bash script didn't have... I don't see a good way around this, but it's worth noting.

zombiezen · 2018-08-07T16:16:13Z

samples/guestbook/gcp/deploy.go

+	keys := []string{"project", "cluster_name", "cluster_zone", "bucket", "database_instance", "database_region", "motd_var_config", "motd_var_name"}
+	tfState := make(map[string]string)
+	for _, k := range keys {
+		cmd := exec.Command("terraform", "output", "-state", tfStatePath, k)


Ah, this is one place where we can make the Go version more efficient: use the -json flag to get this all at once and use a struct to unmarshal the object.

zombiezen · 2018-08-07T16:17:41Z

samples/guestbook/gcp/deploy.go

+	kubeCmds := [][]string{
+		{"kubectl", "apply", "-f", filepath.Join(tempDir, "guestbook.yaml")},
+		// Force pull the latest image.
+		{"kubectl", "scale", "--replicas", "0", filepath.Join("deployment", guestbookDir)},


The argument must be the literal string "deployment/guestbook". It's a Kubernetes identifier, not a file path.

zombiezen · 2018-08-07T16:20:39Z

samples/guestbook/gcp/deploy.go

+	// Build Guestbook Docker image.
+	log.Printf("Building %s...", imageName)
+	build := exec.Command("vgo", "build", "-o", "gcp/guestbook")
+	build.Env = []string{"GOOS=linux", "GOARCH=amd64"}


Append to environment; don't replace it.

zombiezen · 2018-08-07T16:22:34Z

samples/guestbook/gcp/deploy.go

+	tfState := make(map[string]string)
+	for _, k := range keys {
+		cmd := exec.Command("terraform", "output", "-state", tfStatePath, k)
+		outb, err := cmd.CombinedOutput()


Here and elsewhere: it seems like it would be more beneficial to just connect the subprocess's stderr directly to the parent process's stderr.

I'm skeptical about this. I usually find it's better to log a high-level summary of what's going on and either send the subcommand outputs to files or only display them when errors occur. Let's discuss.

Summarizing our chat: If I recall correctly, you're saying that

CombinedOutput() can truncate.

These commands won't be sending an overwhelming amount of output to the screen.

It will help to see progress instead of it looking like the program has hung.

I agreed to forward the command stderrs to os.Stderr, possibly revisiting the decision if the signal-to-noise ratio gets too low and there's too much output.

zombiezen · 2018-08-07T16:27:19Z

samples/guestbook/gcp/deploy.go

+			time.Sleep(dt)
+			continue
+		}
+		endpoint := t.status.loadBalancer.ingress[0]


I don't think this will succeed.. encoding/json cannot unmarshal into unexported fields.

zombiezen · 2018-08-07T16:28:40Z

samples/guestbook/gcp/deploy.go

+		}
+		var t thing
+		if err := json.Unmarshal(out, t); err != nil {
+			dt := 5 * time.Second


Inducing sleep on getting invalid JSON output doesn't seem correct: sleep should happen after not finding an ingress point.

zombiezen · 2018-08-07T16:29:14Z

samples/guestbook/gcp/deploy.go

+}
+
+func deploy(guestbookDir string) error {
+	tfStatePath := filepath.Join(guestbookDir, "gcp", "terraform.tfstate")


While we're here, might as well make this a flag too.

zombiezen · 2018-08-07T16:43:03Z

Tangential, but have you looked at http://labix.org/pipe? It might help, but I've never used it myself.

ijt · 2018-08-08T01:26:45Z

I hadn't seen that pipe lib before. It looks handy, although I can't seem to find where it would be useful in this not-script anymore.

This means it can be run like `go1.11beta1 run ./localdb`.

This still does not work. It just hangs on the last call to nc -z. Maybe it will work to make Go try to connect to the port instead.

This way does not hang.

The MySQL command is still failing though.

zombiezen · 2018-08-09T21:28:28Z

@ijt, Is this ready to review? I've added @enocom.

ijt · 2018-08-10T00:53:36Z

@zombiezen, not just yet. I still have to migrate another provision-db.sh and a test.sh script.

ijt · 2018-08-17T21:34:07Z

Okay, I made those changes. PTAL.

zombiezen

LGTM after 2 comments, but please wait for @enocom to approve as well. Please also verify your changes work on the Mac before submitting.

zombiezen · 2018-08-20T15:56:51Z

samples/guestbook/aws/provision_db/main.go

+func main() {
+	log.SetFlags(0)
+	log.SetPrefix("aws/provision_db: ")
+	flags := map[string]*string{


(Here and elsewhere:) Using a map loses the compile safety of passing these variables directly. Use variables directly or perhaps a struct to group them.

zombiezen · 2018-08-20T15:58:20Z

samples/guestbook/gcp/provision_db/main.go

+	}
+	defer func() {
+		if _, err := run("docker", "kill", proxyContainerID); err != nil {
+			panic(fmt.Sprintf("killing docker container for proxy: %v", err))


Nope, still here.

ijt

Okay, changes are up. PTAL.

ijt · 2018-08-20T20:55:27Z

samples/guestbook/gcp/provision_db/main.go

+	}
+	defer func() {
+		if _, err := run("docker", "kill", proxyContainerID); err != nil {
+			panic(fmt.Sprintf("killing docker container for proxy: %v", err))


Hrm, well, this time for real.

ijt · 2018-08-20T21:51:37Z

samples/guestbook/aws/provision_db/main.go

+func main() {
+	log.SetFlags(0)
+	log.SetPrefix("aws/provision_db: ")
+	flags := map[string]*string{


enocom

One small nit. Otherwise, LGTM.

enocom · 2018-08-20T22:15:11Z

samples/guestbook/README.md


 ```shell
-./localdb.sh
+go run localdb/main.go


Since running this program requires additional Go libraries, you should probably add a go get ./localdb/... instruction above to avoid confusing the reader.

Issac Trotts added 2 commits August 6, 2018 15:26

replace guestbook/gcp/deploy.sh with deploy.go

d2a48eb

fix minor issues in deploy.go

976ac0a

googlebot added the cla: yes Google CLA has been signed! label Aug 6, 2018

zombiezen suggested changes Aug 7, 2018

View reviewed changes

Issac Trotts added 12 commits August 7, 2018 19:11

changes to deploy.go suggested by @zombiezen

ee567c6

convert localdb.sh from Bash to Go

b737471

Move localdb.go to localdb/main.go.

3b6c908

This means it can be run like `go1.11beta1 run ./localdb`.

Move gcp/deploy.go to gcp/deploy/main.go

966aa96

Make go run invocation work on go 1.10

493cd9c

Try a more direct call to nc -z in a Go for loop.

43c81c6

This still does not work. It just hangs on the last call to nc -z. Maybe it will work to make Go try to connect to the port instead.

Use net.Dial instead of nc -z.

073e705

This way does not hang.

Clean up db seeding section, show stderr.

17f66a8

use pipe.Line

a34fab4

The MySQL command is still failing though.

Add retry logic to db seeding. Remove wait for db "health".

05f2baa

gracefully shutdown on ctrl-C

f40ba50

remove redundant logging

6b25773

zombiezen requested a review from enocom August 9, 2018 21:28

Issac Trotts added 8 commits August 9, 2018 17:56

Add logging prefixes.

d069cfb

Replace gcp/provision-db.sh with provision_db/main.go

0548331

Convert aws/provision-db.sh to provision_db/main.go

ac8b7f8

fix broken go run command in main.tf

343adf7

replace tests/gcp/test.sh with test.go

f734623

Add os.Environ() to all calls

db71d8b

add missing gcloud

dfacd28

Clean up output from check for existence of vgo.

f4f946c

Issac Trotts added 13 commits August 17, 2018 12:54

Use flags in gcp/provision_db. Get schema from flag.

9a6497f

Add todo for SIGTERM.

5fe4411

Resolve symlinks

e376f34

Move def of gcp var.

9654222

Rename types according to k8s docs

407f451

Rename project field to projectID

adbcba8

Return nil byte slice on error.

4abd81c

Update local-exec of provision_db to use flags.

c3f05e8

Use filepath.Join

a284520

Remove Chdir.

c511e8e

Replace panics with log.Printfs

a1f177e

Remove stray TF_IN_AUTOMATION=1

8986d5a

Remove tests/aws/test.go

9e97c6b

Issac Trotts added 4 commits August 17, 2018 14:36

Clean up logging.

c123708

Rename serviceAccountVoldir to serviceAccountVolDir

8bcd2a4

Replace log.Printf(err) with log.Print(err)

b4cfd77

Merge branch 'master' into goscripts

e0e3cbd

zombiezen approved these changes Aug 20, 2018

View reviewed changes

Issac Trotts added 4 commits August 20, 2018 13:54

Convert panic to log.Printf.

93345bd

Use flag.VisitAll instead of a map and a range.

c69d2c4

aws/provision_db: Use flag.VisitAll instead of a map and a range.

5b6b7ca

Merge branch 'goscripts' of github.com:ijt/go-cloud into goscripts

3a746a0

ijt commented Aug 20, 2018

View reviewed changes

enocom approved these changes Aug 21, 2018

View reviewed changes

Issac Trotts added 2 commits August 21, 2018 08:42

enocom: add line to fetch deps for localdb

8e575ec

Turn off logging of dates and times.

0f4a401

enocom merged commit aa83f53 into google:master Aug 21, 2018

ijt mentioned this pull request Aug 21, 2018

samples/guestbook/gcp: provision-db.sh depends on jq #220

Closed

ijt deleted the goscripts branch August 21, 2018 18:08


		// The deploy program builds the Guestbook server locally and deploys it to
		// GKE.

Comments

Conversation

ijt commented Aug 6, 2018

Uh oh!

ijt commented Aug 6, 2018

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!

zombiezen commented Aug 7, 2018

Uh oh!

ijt commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zombiezen commented Aug 9, 2018

Uh oh!

ijt commented Aug 10, 2018

Uh oh!

ijt commented Aug 17, 2018

Uh oh!

zombiezen 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!

ijt 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!

enocom left a comment

Choose a reason for hiding this comment

Uh oh!

ijt commented Aug 8, 2018 •

edited

Loading