Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure help information respects --json-output flag, fixes #5567 #5572

Conversation

GuySartorelli
Copy link
Collaborator

The Issue

How This PR Solves The Issue

Replaces the default help function with one that outputs a JSON-formatted representation of a command when the --json-output flag is used.

Manual Testing Instructions

Run any version of the help command or -h flag in conjunction with the --json-output (or -j) flag, e.g:

  • ddev help -j
  • ddev auth -hj
  • ddev auth ssh -hj

Automated Testing Overview

I haven't added tests yet, because I want to make sure the approach is correct and acceptable before I do so. I wouldn't want to write tests only for them to be entirely invalidated by sweeping changes required in the implementation.

Once a maintainer has told me that the approach for the implementation is good, I'll add some tests.

Related Issue Link(s)

#5567

Release/Deployment Notes

N/A

Other notes

This is my first time contributing here and my first time writing anything in go - so it's likely I've done things in a completely backwards way - I don't have a good background on go conventions, nor this codebase, nor the cobra codebase or conventions, so please make sure to explain things clearly and in full when requesting changes.

Copy link

github-actions bot commented Nov 26, 2023

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Thanks so much for giving this a try! This is looking great. The one thing I'd say is probably structure the json the way all the other commands are structured; take a look at ddev describe -j for example, where the raw has the json, msg has the plain text. SO impressive! Thanks.

It also needs a gofmt, gofmt -w -s [cmd/ddev/cmd/root.go](https://github.com/ddev/ddev/pull/5572/files#diff-251397554946dd8f37a081ff0d8638b12916434618051440c546fa08c20cf1bc) - Most IDEs will do all this for you happily, including GoLand and vscode.

@rfay
Copy link
Member

rfay commented Nov 26, 2023

(And since your head is in this, I'd love it if you'd take a look at

which is our oldest open issue :) )

@GuySartorelli GuySartorelli force-pushed the 20231126_GuySartorelli_help-command-respects-json-output-flag branch from b5c98c9 to 654e146 Compare November 28, 2023 08:44
@GuySartorelli
Copy link
Collaborator Author

I've updated this to use output.UserOut.WithField() to output the JSON, which seems to be what the describe command is doing... and that's fine for using the help command like so:

ddev help -j
ddev help list -j

But if I try to use the help flag, we get the "test" output which indicates the --json-output flag is captured correctly, but we don't get any of the data I'm shoving into raw:

ddev -hj
ddev list -h -j

Do you know what might be causing the JSON to not be output correctly when -h is used as a flag?

@GuySartorelli GuySartorelli force-pushed the 20231126_GuySartorelli_help-command-respects-json-output-flag branch from 654e146 to 977b5ae Compare November 28, 2023 08:56
@GuySartorelli
Copy link
Collaborator Author

Nevermind, I think I found it. output.LogSetUp() needs to be called a second time. It's called once in root.init but that's before the --json-output flag is parsed. See

// LogSetup() has already been done, but now needs to be done
// again *after* --json flag is parsed.
output.LogSetUp()

Let me know what further changes are required with the implementation as a whole - once you're happy with that I'll get some tests written.

rawResult["Deprecated"] = command.Deprecated
rawResult["Hidden"] = command.Hidden

output.UserOut.WithField("raw", rawResult).Print()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I can't pass the plain text help output into the Print() call, since the original help function prints directly to the terminal rather than returning its result.

@rfay
Copy link
Member

rfay commented Nov 28, 2023

This is looking great. I tested with ddev help -j | jq, ddev help -j | jq -r .raw, ddev help describe -j |jq -r .raw

@rfay
Copy link
Member

rfay commented Nov 28, 2023

I guess the one thing I don't like right now is that it's inline in the root.go init(). init() is evil anyway, but could you refactor it to put it in a function in root_help_json.go or something?

@rfay rfay requested a review from stasadev November 28, 2023 20:01
@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented Nov 28, 2023

could you refactor it to put it in a function in root_help_json.go or something?

I can - but unless you say otherwise I'm going to call it literally root_help_json.go - "or something" is too vague for me to do anything with :p

I also note that you didn't say you tested it with -h or --help - those do work subtly different than directly invoking the help command, so it'll pay to make sure that works too.

@rfay
Copy link
Member

rfay commented Nov 28, 2023

Literally call it that!

@GuySartorelli GuySartorelli force-pushed the 20231126_GuySartorelli_help-command-respects-json-output-flag branch from 977b5ae to 23bcdc0 Compare November 29, 2023 00:21
@GuySartorelli
Copy link
Collaborator Author

That was way easier to refactor than I expected it to be haha.
I'll look at adding some tests when I get a little more time.

@rfay
Copy link
Member

rfay commented Nov 29, 2023

I can live without a test on this one, but would love fix and test for the logs one :) (new PR :) )

@GuySartorelli
Copy link
Collaborator Author

JSON format for logs is less important to me personally, so I won't promise anything on that front.

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

It works good.

The only exceptions I found (for example, ddev help composer) do not support the -j flag, which is expected:

@GuySartorelli
Copy link
Collaborator Author

I can live without a test on this one

Given this statement, is there anything left to do for this PR? Seems ready to merge, I think?

@rfay
Copy link
Member

rfay commented Dec 1, 2023

I hope to give it a run.

@GuySartorelli
Copy link
Collaborator Author

GuySartorelli commented Dec 13, 2023

I hope to give it a run.

Does that mean you want to try run it locally to test it before merging? Sorry, just trying to understand as I had trouble parsing that sentence.

I know you said you're okay without automated tests for this, but if I provided some would that speed things along at all?

@rfay
Copy link
Member

rfay commented Dec 13, 2023

Sorry I didn't get to test before leaving. Maybe @stasadev will get a chance to look. Thanks for the contribution.

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

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

The code looks fine and doesn't seem to break anything, I'll pull it today.

@stasadev stasadev merged commit 4580ec2 into ddev:master Dec 13, 2023
18 checks passed
@GuySartorelli GuySartorelli deleted the 20231126_GuySartorelli_help-command-respects-json-output-flag branch December 13, 2023 19:43
@GuySartorelli
Copy link
Collaborator Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants