-
Notifications
You must be signed in to change notification settings - Fork 493
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
Support wide output format for get commands (-o wide) #3129
Support wide output format for get commands (-o wide) #3129
Conversation
Hi @yeedove. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, I left a minor comment 👍
/ok-to-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OutputFormatWIDE
type is not currently handled by the switch statement below. Please add a case statement for OutputFormatWIDE
type:
gateway-api/gwctl/pkg/printer/policies.go
Lines 88 to 96 in e50fd3b
switch format { | |
case utils.OutputFormatJSON, utils.OutputFormatYAML: | |
pp.printClientObjects(clientObjects, format) | |
case utils.OutputFormatTable: | |
pp.printPoliciesTable(sortedPolicies) | |
default: | |
fmt.Fprintf(os.Stderr, "unknown output format '%s' found\n", format) | |
os.Exit(1) | |
} |
gateway-api/gwctl/pkg/printer/policies.go
Lines 129 to 137 in e50fd3b
switch format { | |
case utils.OutputFormatJSON, utils.OutputFormatYAML: | |
pp.printClientObjects(clientObjects, format) | |
case utils.OutputFormatTable: | |
pp.printCRDsTable(sortedPolicyCRDs) | |
default: | |
fmt.Fprintf(os.Stderr, "unknown output format '%s' found\n", format) | |
os.Exit(1) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yeedove! This looks great, just have one minor request which should be quite straightforward.
gwctl/pkg/printer/backends.go
Outdated
if wide { | ||
columnNames = []string{"NAMESPACE", "NAME", "TYPE", "REFERRED BY ROUTES", "AGE", "POLICIES"} | ||
} else { | ||
columnNames = []string{"NAMESPACE", "NAME", "TYPE", "REFERRED BY ROUTES", "AGE"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a bit different from what's documented but I've come to realize that we should avoid any parameter from being displayed which needs additional calculations.
Calculating things like "REFERRED BY ROUTES" and "POLICIES" will take additional time (which becomes significant when there's lot's of resources). Hence it's preferrable to only include them in the -o wide
format and not as a default.
tl;dr: Can you please move REFERRED BY ROUTES
to wide format as well?
(In case you are wondering, we'll separately work on not calculating the additional values by default int the resourceModel -- as of now, that distinction doesn't exist`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, done!
Can you please also edit the release note and avoid the |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal, yeedove The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
…s#3129) * Support wide output format for get commands (-o wide) * Add a case statement for OutputFormatWide type * Move REFERRED BY ROUTES to wide format * Move REFERRED BY ROUTES to wide format
…s#3129) * Support wide output format for get commands (-o wide) * Add a case statement for OutputFormatWide type * Move REFERRED BY ROUTES to wide format * Move REFERRED BY ROUTES to wide format
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #2889
Does this PR introduce a user-facing change?: