Command-line improvements

[mt-inside]

Bad branch name, but in this PR:

• Move cw gen to cw gen configs to make room for cw gen [clusters,services]
• Add cw version (what do people thing of the library choices?)

[mt-inside]

Fixes #14

[mt-inside]

Fixes #6

[ZackButcher]

Looks good, mostly a few style things I didn't notice when we VCd. Big change I'm asking is that we just define a version struct ourselves, I don't see go-version being very valuable for us (especially if we add tag like I also request below).

[ZackButcher]

nit: would be nice use the tag if available. See https://github.com/istio/istio/blob/974351942aa08ec2d9f2f6f350b9733b0f6130e4/bin/get_workspace_status.sh#L17-L23 and https://github.com/istio/istio/blob/974351942aa08ec2d9f2f6f350b9733b0f6130e4/bin/get_workspace_status.sh#L32-L41.

[ZackButcher]

So looking over Istio's version command, one of the things I like is that it prints both git commit and "version", where version is populated by the tag (if the commit has a tag; if not the version is something like unknown or <git commit sha>-clean/<git commit sha>-dirty). It's easy enough to add, but won't work with this library.

Then looking at the library, I don't think it's worth vendoring period. The library itself is fine, but one of the go-ism is "a little copying is better than a little dependency" (also auditing the code I don't love how ToShortened shortened is implemented, if I'm nitpicking things :P ). Given that the dependency is so simple, a struct that's just a wrapper around json.Marshal that also implements ToString, I'd rather just see us implement it ourselves in this package. Then we can easily include a version/tag field, for example.

[mt-inside]

Will do, good practice for me

[ZackButcher]

• Default value is already false, this should be rewritten as shortened bool
• Lets move this declaration down into func versionCmd(). The others need to be package level because we're setting them at build time with linker magic, but this is a flag so it shouldn't be declared at the package level.

[ZackButcher]

Suggested change

	Run: func(_ *cobra.Command, _ []string) {
	RunE: func(*cobra.Command, []string) error {

• With cobra always use the version that allows us to return an error.
• When you ignore both arguments to a function, you can omit all of them (you don't even need underscores); when you ignore some and use some you must put names for all of them (where _ is a valid "name").

[ZackButcher]

This should be re-written a touch for a few different reasons: a) we want to move the declaration of response as close to its usage site as possible, b) rather than relying on fmt.Printf to print to stdout for us, lets the form that accepts io.Writer.

dest := os.Stdout

version := goVersion.New(version, commit, date)
var out string
if short {
	out = version.ToShortened()
} else {
	out = version.ToJSON()
}
_, err := fmt.Fprintf(dest, "%+v", out)
return err

note: part of me also likes re-writing it as:

print := func(s string) error {
	_, err := fmt.Fprintf(os.Stdout, "%+v", s)
	return err
}

version := goVersion.New(version, commit, date)
if short {
	return print(version.ToShortened())
}
return print(version.ToJSON())

The first is likely more idiomatic, though ideally you want to remove the nesting. You could argue this is fairly idiomatic too:

dest := os.Stdout

version := goVersion.New(version, commit, date)
if short {
	_, err := fmt.Fprint(os.Stdout, "%+v", version.ToShortened())
	return err
}
_, err := fmt.Fprintf(dest, "%+v", version.ToJSON())
return err

[ZackButcher]

move into cmd/gen/package gen. Keep the top level cmd/gen.go with func genCmd() in it. Roughly, every top level cw command should be a file in cmd/, if they have subcommands those subcommands should live in cmd/<subcommand>/, and finally the top level cw itself lives in cmd/cw (since that makes naming nice and easy for us).

[ZackButcher]

You shouldn't need -o cw since we're building a package named cw now (IIRC).