NotificationsYou must be signed in to change notification settings
Fork1k
Star11.2k

feat(cli): add --output={text,json} to version cmd#7010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 10 commits intomainfromcj/version-json

Apr 5, 2023

Merged

feat(cli): add --output={text,json} to version cmd#7010

johnstcn merged 10 commits intomainfromcj/version-json

Apr 5, 2023

Conversation

Copy link

Member

johnstcn commentedApr 5, 2023•
edited
Loading

$ build/coder-slim_$(./scripts/version.sh)_linux_amd64 version --output=textCoder v0.21.3-devel+b099e5c58 Wed Apr  5 12:03:29 UTC 2023https://github.com/coder/coder/commit/b099e5c58c9f8dbb2fbbded3d559da7923cab3f8Slim build of Coder, does not support the  server  subcommand.$ build/coder-slim_$(./scripts/version.sh)_linux_amd64 version --output=json{  "version": "v0.21.3-devel+b099e5c58",  "build_time": "2023-04-05T12:03:29Z",  "external_url": "https://github.com/coder/coder/commit/b099e5c58c9f8dbb2fbbded3d559da7923cab3f8",  "slim": true,  "agpl": false}

feat(cli): add --json output to version cmd

af0508e

johnstcn self-assigned this

Apr 5, 2023

johnstcn added4 commits

April 5, 2023 11:16

fixup! feat(cli): add --json output to version cmd

c8c610e

fixup! feat(cli): add --json output to version cmd

d3d5a69

fixup! feat(cli): add --json output to version cmd

b9ada46

fixup! feat(cli): add --json output to version cmd

caf2b46

johnstcn marked this pull request as ready for review

April 5, 2023 10:57

johnstcn requested review frommafredri andmtojek

April 5, 2023 10:57

mafredri approved these changes

Apr 5, 2023

View reviewed changes

mtojek approved these changes

Apr 5, 2023

View reviewed changes

Copy link

Member

mtojek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit-picks can be ignored

cli/version_test.go Outdated

		err := inv.WithContext(ctx).Run()
		require.NoError(t, err)
		actual := buf.String()
		actual = strings.Replace(actual, "\r\n", "\n", -1)

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit: ReplaceAll?

cli/version.go Outdated

		// version prints the coder version
		func (RootCmd) version() clibase.Cmd {
		handleHuman := func(inv *clibase.Invocation) error {
		var str strings.Builder

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit: do you need a string builder if there is no way to break execution in the middle of processing?

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not sure, but this is just the previous code I copy-pasta'ed.

Copy link

Member

mafredri commentedApr 5, 2023

Hmm, should we call the flag output/format to align with table formatter and potentially support other formats in the future?

Copy link

MemberAuthor

johnstcn commentedApr 5, 2023

Hmm, should we call the flag output/format to align with table formatter and potentially support other formats in the future?

Good point, we should have a consistent naming (although the current format is decidedly un-tabular)

Copy link

Member

mafredri commentedApr 5, 2023•
edited
Loading

Good point, we should have a consistent naming (although the current format is decidedly un-tabular)

Yeah, using a clibase enum with onlyjson seems sufficient here, only allowing-{o,-output} json (or maybe json and text, default being text). But users being able to rely on the--output flag seems like a good idea.

johnstcn added5 commits

April 5, 2023 12:59

feat(cliui): add TextFormat

f218bcf

use --output specifier instead

1f403a2

fixup! use --output specifier instead

a7ec68d

gen

747e63f

strings.ReplaceAll

b099e5c

Copy link

MemberAuthor

johnstcn commentedApr 5, 2023

Good point, we should have a consistent naming (although the current format is decidedly un-tabular)
Yeah, using a clibase enum with onlyjson seems sufficient here, only allowing-{o,-output} json (or maybe json and text, default being text). But users being able to rely on the--output flag seems like a good idea.

Turns out I needed to add atextFormat option tocli/cliui which wasn't too bad. It also forced me to somewhat abstract where we get the version info from, which isn't a bad thing. I haven't figured out how to plumb random version info into the unit tests yet, but that's a good follow-on PR.