Add CLI version subcommand and flag #537
Conversation
brandur
force-pushed
the
brandur-cli-version
branch
from
cc5c1bd
to
e79357e
Compare
I was trying to help which River CLI I had installed and realized there's no easy way to know this currently because it can't currently reveal its version information in any built-in way. Here, add a `river version` subcommand that prints the version of the Go module it's distributed as, along with the version of Go used to build it, making accessing version information easy. I also added support for a `river --version` flag. There's no common standard on whether version should be revealed by subcommand or flag, and since `--version`'s (unlike `-v`) never going to be used for anything else, we may as well be permissable in what we accept. I also add a basic framework for a CLI "integration" test suite that allows CLI commands to be tested from the level of the Cobra command, which is useful for checking flags and such. It has the downside in that there's no way to inject a test transaction into it, so it's not always appropriate for use right now.
brandur
force-pushed
the
brandur-cli-version
branch
from
e79357e
to
5008c24
Compare
|
Thanks! Also, I had to tie golangci-lint to 1.60.1. It was upgrading to the new 1.60.2 in CI (not even available yet on Homebrew) which despite a patch-level release, came with a bunch of breaking changes. Ugh. |
Yes, golangci-lint 1.60.2 is quite broken. |
| fmt.Fprintf(os.Stderr, "failed: %s\n", err) | ||
| fmt.Fprintf(os.Stdout, "failed: %s\n", err) |
There was a problem hiding this comment.
I'm surprised by this change.
If you had used command.StdOut, I might have understand, but here reporting errors to stdout seems strange.
Maybe I simply missed something about your PR
If it's OK to use stdout here for error reporting, adding an inline comment could help
There was a problem hiding this comment.
Good catch. That's actually an accident — I was refactoring lots of stuff related to output to find a good way of capturing it for comparison in tests, and this accidentally got hit. Opened #547 with a fix.
| require.Equal(t, strings.TrimSpace(fmt.Sprintf(` | ||
| River version (unknown) | ||
| Built with %s | ||
| `, buildInfo.GoVersion)), strings.TrimSpace(bundle.out.String())) |
There was a problem hiding this comment.
I would have used require.Contains to do not link the test and the templating
| require.Equal(t, strings.TrimSpace(fmt.Sprintf(` | |
| River version (unknown) | |
| Built with %s | |
| `, buildInfo.GoVersion)), strings.TrimSpace(bundle.out.String())) | |
| require.Contains(t, bundle.out.String(), "River version (unknown)") | |
| require.Contains(t, bundle.out.String(), "Built with " + buildInfo.GoVersion) |
There was a problem hiding this comment.
Use of Contains does seem plausible. I think this is a little more of a personal preference thing though — I kind of like how when you match the entire output your tests are more strict. i.e. If there was any extra output in there accidentally, they'd catch the problem. This comes with the tradeoff of possibly being a little more brittle in case of minor whitespace changes that aren't really that important to know about.
This corrects a problem from #537, wherein I was doing a lot of refactoring for testing purposes, and accidentally changed a place where failures were reported from stderr to stdout.
This corrects a problem from #537, wherein I was doing a lot of refactoring for testing purposes, and accidentally changed a place where failures were reported from stderr to stdout.
I was trying to help which River CLI I had installed and realized
there's no easy way to know this currently because it can't currently
reveal its version information in any built-in way.
Here, add a
river versionsubcommand that prints the version of the Gomodule it's distributed as, along with the version of Go used to build
it, making accessing version information easy.
I also added support for a
river --versionflag. There's no commonstandard on whether version should be revealed by subcommand or flag,
and since
--version's (unlike-v) never going to be used foranything else, we may as well be permissable in what we accept.
I also add a basic framework for a CLI "integration" test suite that
allows CLI commands to be tested from the level of the Cobra command,
which is useful for checking flags and such. It has the downside in that
there's no way to inject a test transaction into it, so it's not always
appropriate for use right now.