NO-JIRA: Display whoami information as JSON or YAML #1324
Conversation
|
/retest-required |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
christianh814
changed the title
|
/retest-required |
Signed-off-by: Christian Hernandez <[email protected]> changing to output of json or yaml Signed-off-by: Christian Hernandez <[email protected]> updated to use output of yaml or json Signed-off-by: Christian Hernandez <[email protected]> added the GVK to the output Signed-off-by: Christian Hernandez <[email protected]>
christianh814
force-pushed
the
whoami-show-groups
branch
from
26f79b8 to
790d0f4
Compare
christianh814
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-aws-ovn-serial |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
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. |
|
/reopen |
|
@christianh814: Reopened this PR. In response to this:
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. |
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
WalkthroughReplaces cli-runtime import/use of genericiooptions with genericclioptions, adds printer support and output flag wiring, updates WhoAmIOptions and constructors, enforces validation against combining --output with show flags, and adds a printing branch that retrieves and prints a User object using the configured printer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/cli/whoami/whoami.go (4)
5-5: Consider using kubectl's built-in printer utilities instead of custom marshaling.As noted in a previous review, kubectl provides built-in printing flags and utilities (e.g.,
genericclioptions.PrintFlags) that handle YAML/JSON output standardization, including proper APIVersion/Kind handling and formatting. This would eliminate the need for these manual imports and reduce code duplication.Reference the kubectl whoami implementation: https://github.com/kubernetes/kubectl/blob/5ff92a69e3ecb06912d5f468186d458bf610244e/pkg/cmd/auth/whoami.go#L64
Also applies to: 24-24
56-56: Remove unusedShowGroupsfield.This field is declared but never used anywhere in the code. As previously noted, it appears unrelated to the current implementation.
If the intention is to add a flag to selectively show/hide groups in the output, please wire it up properly or remove it in a future commit.
185-197: Critical: Potential nil pointer dereference and bypasses existing logic.Several issues here:
Nil pointer dereference: Line 187 calls
u.SetGroupVersionKind()before checking the error on line 188. IfUsers().Get()fails,uwill benil, causing a panic.Bypasses SelfSubjectReview: As previously noted, the code should use the existing
WhoAmI()function, which triesSelfSubjectReviewfirst (which includes group information) and falls back toUsers().Get(). Directly callingUsers().Get()bypasses this logic and may not return group memberships in some cluster configurations.Inconsistent behavior: The
WhoAmI()function prints the username to stdout as a side effect. This case doesn't, leading to inconsistent user experience.Fix 1: Check error before calling SetGroupVersionKind
case o.Output == "yaml": u, err := o.UserInterface.Users().Get(context.TODO(), "~", metav1.GetOptions{}) - u.SetGroupVersionKind(userv1.GroupVersion.WithKind("User")) if err != nil { return err } + u.SetGroupVersionKind(userv1.GroupVersion.WithKind("User"))Fix 2: Refactor to use WhoAmI() and built-in printers
Better yet, refactor both YAML and JSON cases to call
o.WhoAmI()and use kubectl's built-in printer utilities, as suggested in the previous review.
198-209: Same critical issues as the YAML case, plus code duplication.This case has the same problems:
- Nil pointer dereference: Line 200 calls
SetGroupVersionKind()before checking the error on line 201.- Bypasses SelfSubjectReview logic: Should use
WhoAmI()instead of directUsers().Get().- Code duplication: Nearly identical to the YAML case (lines 185-197), violating DRY principle.
The duplication and issues would be resolved by refactoring to use kubectl's built-in printer utilities, which handle both YAML and JSON formats with proper error handling and GVK management.
🧹 Nitpick comments (1)
pkg/cli/whoami/whoami.go (1)
162-166: Consider deferring UserInterface initialization.Moving the initialization to the beginning of
Run()means it's created even when not needed (e.g., whenShowToken,ShowContext,ShowServer, orShowConsoleUrlare true). The original placement (after the switch cases) was more efficient.However, if you adopt built-in printer utilities as suggested in previous reviews, the initialization placement may need to be reconsidered based on the new architecture.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/whoami/whoami.go(7 hunks)
Signed-off-by: Christian Hernandez <[email protected]>
|
@ardaguclu Updated/resolved comments. PTAL cc @kaovilai |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/whoami/whoami.go (1)
92-113: Fix double printing: WhoAmI writes to stdout and the printer prints again, breaking -o JSON/YAML.When
-ois used,Run()callsWhoAmI()which already writes the username too.Out, then the printer emits JSON/YAML. This produces mixed/invalid output. MakeWhoAmI()pure (return the user only) and print username only in the default path.Apply this diff:
@@ func (o WhoAmIOptions) WhoAmI() (*userv1.User, error) { res, err := o.AuthV1Client.SelfSubjectReviews().Create(context.TODO(), &v1.SelfSubjectReview{}, metav1.CreateOptions{}) if err == nil { me := &userv1.User{ ObjectMeta: metav1.ObjectMeta{ Name: res.Status.UserInfo.Username, }, Groups: res.Status.UserInfo.Groups, } - fmt.Fprintf(o.Out, "%s\n", me.Name) return me, nil } else { klog.V(2).Infof("selfsubjectreview request error %v, falling back to user object", err) } me, err := o.UserInterface.Users().Get(context.TODO(), "~", metav1.GetOptions{}) - if err == nil { - fmt.Fprintf(o.Out, "%s\n", me.Name) - } return me, err } @@ case o.resourcePrinterFunc != nil: // If output format is specified, get the user info and print it in the requested format me, err := o.WhoAmI() if err != nil { return err } // Set GroupVersionKind so it's properly displayed me.SetGroupVersionKind(userv1.GroupVersion.WithKind("User")) return o.resourcePrinterFunc(me, o.Out) } - // Default behavior: just print the username - _, err = o.WhoAmI() - return err + // Default behavior: just print the username + me, err := o.WhoAmI() + if err != nil { + return err + } + fmt.Fprintf(o.Out, "%s\n", me.Name) + return nilAlso applies to: 208-217, 219-222
🧹 Nitpick comments (3)
pkg/cli/whoami/whoami.go (3)
115-145: Initialize clients in Complete(), not Run().Follow kubectl/oc command patterns: construct clients in
Complete()so validation/early failures surface beforeRun(), and to keepRun()focused on execution. Move creation ofUserV1InterfaceandAuthenticationV1InterfacetoComplete()aftero.ClientConfigis set.Also applies to: 179-190
31-41: Clarify allowed --output values (restrict to yaml/json or update docs/examples).You’re using PrintFlags, which enables many formats (json, yaml, name, go-template, etc.). The PR text says “JSON or YAML”. Either:
- Restrict to yaml|json in Validate(), or
- Explicitly support all PrintFlags formats and update help/examples accordingly.
If restricting, minimally add:
func (o *WhoAmIOptions) Validate() error { + if o.PrintFlags.OutputFlagSpecified() { + if f := o.PrintFlags.OutputFormat; f != nil && *f != "yaml" && *f != "json" { + return fmt.Errorf("--output must be 'yaml' or 'json'") + } + } // Check if output flag is used with other show flags if o.PrintFlags.OutputFlagSpecified() && (o.ShowToken || o.ShowContext || o.ShowServer || o.ShowConsoleUrl) { return fmt.Errorf("--output cannot be used with --show-token, --show-context, --show-server, or --show-console") }Otherwise, please update command help and examples to mention the supported formats.
Also applies to: 87-87, 146-151
38-41: Add examples for -o usage.Help users discover the new feature.
Apply this diff:
var whoamiExample = templates.Examples(` # Display the currently authenticated user oc whoami + # Display the current user as JSON + oc whoami -o json + # Display the current user as YAML + oc whoami -o yaml `)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/whoami/whoami.go(6 hunks)
🔇 Additional comments (1)
pkg/cli/whoami/whoami.go (1)
55-59: Nice: standard PrintFlags wiring and exclusivity validation.Good move to genericclioptions.PrintFlags and to prohibit mixing
-owith show flags. This aligns with kubectl UX.Also applies to: 61-66, 68-90, 146-151
|
Lol |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: christianh814, kaovilai The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@christianh814: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Overall idea looks good to me. Dropped a few comments. Also it is better to add unit tests to verify the behavior in whoami_test.go
Thank you
| "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/cli-runtime/pkg/genericiooptions" | ||
| "k8s.io/cli-runtime/pkg/genericclioptions" |
There was a problem hiding this comment.
This is deprecated. genericiooptions needs to be used.
There was a problem hiding this comment.
But isn't genericclioptions used for PrintFlags? I see I can use genericiooptions for IOStream. Both are used right? Because rollback and startBuild also use this approach
There was a problem hiding this comment.
genericclioptions has been deprecated in favor of genericiooptions. So theoretically you can use genericclioptions. However, it is always preferable to use the recommended one (i.e. genericiooptions), since the other one will be removed entirely at some point.
| return nil | ||
| case o.resourcePrinterFunc != nil: | ||
| // If output format is specified, get the user info and print it in the requested format | ||
| me, err := o.WhoAmI() |
There was a problem hiding this comment.
WhoAmI function still prints the username into the terminal. So the output of this function is not an valid json/yaml. I think we can pass a boolean to print username or not.
|
New changes are detected. LGTM label has been removed. |
|
I saw merge commits in your PR history. This shouldn't happen. Could you please rebase onto the first commit?. |
|
if you use merge method squash it shouldn't matter? can do via label for tide to follow. |
6 participants
Added an option
-oor--outputthat displays the user information in YAML or JSON.Closes: #1323
Signed-off-by: Christian Hernandez [email protected]