Add Sync support with unit tests

[stephen-soltesz]

This change adds support for Command.Sync. This should complete the functionality needed for gcp-config to manage declarative configurations.

A final change will add cloud build automation to run stctl sync with the schedule and configs defined in the "Codified Daily Archive Transfers" design.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 22

• 64 of 64 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+2.4%) to 100.0%

---

Totals
Change from base Build 18:	2.4%
Covered Lines:	185
Relevant Lines:	185

---

💛 - Coveralls

[gfr10598]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)

---

internal/stctl/sync.go, line 20 at r1 (raw file):

// If a matching description is found with different values for IncludePrefixes
// or StartTimeOfDay, then the original job is disabled and a new job created.
func (c *Command) Sync(ctx context.Context) (*storagetransfer.TransferJob, error) {

Should probably document what the returned TransferJob is.

---

internal/stctl/sync.go, line 29 at r1 (raw file):

	// List jobs and find first that matches canonical description.
	logx.Debug.Println("Listing jobs")
	err := c.Client.Jobs(ctx, func(resp *storagetransfer.ListTransferJobsResponse) error {

put the func decl on a new line, to make it easier to see that the body is a func. Alternatively, declare the func first, and pass it by name to Jobs().

---

internal/stctl/sync.go, line 37 at r1 (raw file):

			logx.Debug.Print(pretty.Sprint(job))
			if desc == job.Description {
				// By convention there will only be a single transfer job between two buckets.

Don't know what this comment means.

---

internal/stctl/sync.go, line 39 at r1 (raw file):

				// By convention there will only be a single transfer job between two buckets.
				found = job
				return errFound

I find this semantics confusing - return an error when something is found, instead of not found. Can this be inverted?

---

internal/stctl/sync.go, line 45 at r1 (raw file):

		return nil
	})
	if err != errFound && err != nil {

Review incomplete - I will pick up reviewing here.

[stephen-soltesz]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)

---

internal/stctl/sync.go, line 20 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Should probably document what the returned TransferJob is.

Done.

---

internal/stctl/sync.go, line 29 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

put the func decl on a new line, to make it easier to see that the body is a func. Alternatively, declare the func first, and pass it by name to Jobs().

Done.

---

internal/stctl/sync.go, line 37 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Don't know what this comment means.

Updated text.

---

internal/stctl/sync.go, line 39 at r1 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I find this semantics confusing - return an error when something is found, instead of not found. Can this be inverted?

Seems obvious now that you say it. Good idea. Done.

[gfr10598]

after considering remaining comments.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)

---

internal/stctl/sync.go, line 77 at r2 (raw file):

// Verify that the two arrays have the same values.
func includesEqual(configured []string, desired []string) bool {

you might get what you need here from deep.Equals() - not sure. Also not sure if that is appropriate for production code, but take a look.

[stephen-soltesz]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @gfr10598)

---

internal/stctl/sync.go, line 77 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

you might get what you need here from deep.Equals() - not sure. Also not sure if that is appropriate for production code, but take a look.

We would still need to sort them. deep.Equal could replace the for loop below.

Yeah, I agree deep.Equals isn't quite right since it's for testing. I would welcome a more general package to replace the entire includesEqual function.

Maybe something based on https://github.com/deckarep/golang-set (seems like a solid library based on the advertised users).

I'll keep the more general strategy in mind if we need to do this kind of operation again. For this one instance, I'm satisfied with the current implementation. Let me know if you disagree.

[gfr10598]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @gfr10598)