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.

Sign up for GitHub

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

Jump to bottom

fix: remove --json-output flag from logs command, fixes #962 #5695

Merged
merged 1 commit into from Jan 26, 2024
Merged

fix: remove --json-output flag from logs command, fixes #962 #5695

merged 1 commit into from Jan 26, 2024

Conversation

GuySartorelli
Copy link
Collaborator

The Issue

ddev logs doesn't support JSON formatted output, and there's no clear way to add that functionality given the way the logs are currently fetched and outputted.
This is discussed in #962

How This PR Solves The Issue

We don't display the flag when running ddev help logs or ddev logs -h.
This is the same way this is handled for the composer command (see composer.go) and custom commands (see commands.go)

Manual Testing Instructions

Run ddev help logs and ddev logs -h - the --json-output flag shouldn't be shown as an option.

Automated Testing Overview

No automated tests added - I couldn't find associated tests for composer or for custom commands so it didn't seem sensible to add them for this.

Related Issue Link(s)

#962

Release/Deployment Notes

Doesn't affect deployments.

@GuySartorelli GuySartorelli requested a review from a team as a code owner January 15, 2024 06:09
@GuySartorelli GuySartorelli mentioned this pull request Jan 15, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 15, 2024 •
edited

Download the artifacts for this pull request:

See Testing a PR

stasadev
stasadev reviewed Jan 15, 2024
View reviewed changes
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.

Run ddev help logs and ddev logs -h - the --json-output flag shouldn't be shown as an option.

Tried these, the flag is hidden as described.

And I found another edge case, that doesn't really matter: --json-output is still there when you pass a wrong flag to ddev logs like this ddev logs --foobar.

@GuySartorelli
Copy link
Collaborator Author

And I found another edge case, that doesn't really matter: --json-output is still there when you pass a wrong flag to ddev logs like this ddev logs --foobar.

Oh! That's unexpected... I wonder where that's getting its output from. I presume that same thing is happening for custom commands as well then (presumably not for composer - I bet that's handled differently).

I'll spend some time trying to figure out where that's coming from.

@GuySartorelli
fix: remove --json-output flag from logs command, fixes ddev#962
343765a 
`ddev logs` doesn't support JSON formatted output, and there's no clear
way to add that functionality given the way the logs are currently
fetched and outputted.
We shouldn't display the flag in help if it's not supported.
@GuySartorelli GuySartorelli force-pushed the 20240115_GuySartorelli_remove-json-output-from-logs-command branch from 24a513b to 343765a Compare January 17, 2024 07:17
@GuySartorelli
Copy link
Collaborator Author

Turns out that's an entirely separate function called FlagErrorFunc() which gives identical output to the help function, but generated separately. So we just have to treat that function the same way as the help function.

This doesn't need to be done for composer or for custom commands, because flag parsing isn't handled by cobra for those (everything is just passed straight through, it seems)

stasadev
stasadev approved these changes Jan 17, 2024
View reviewed changes
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.

Looks good to me, a great study of Cobra's capabilities.

@stasadev stasadev merged commit 9d60b74 into ddev:master Jan 26, 2024
18 checks passed
@GuySartorelli GuySartorelli deleted the 20240115_GuySartorelli_remove-json-output-from-logs-command branch January 30, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stasadev stasadev stasadev approved these changes

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@GuySartorelli @stasadev