-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Add support for projected volume in describe function #70158
Add support for projected volume in describe function #70158
Conversation
/sig cli |
infos := []string{} | ||
for _, source := range projected.Sources { | ||
if source.Secret != nil { | ||
infos = append(infos, fmt.Sprintf(" - Secret:\n"+ |
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.
Indent by 4 spaces here
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.
Also, to keep consistency with other volume describers, I'd be in favor of removing the -
at the beginning of this section, and just squashing it down so it reads like:
SecretName: ...
SecretOptional: ...
TokenExpirationSeconds: ...
...
/kind feature |
07b5497
to
249a8e0
Compare
@juanvallejo Thanks for the review. Comments addressed, now the output looks like:
DownwardAPI is special, since data in DownwardAPI is too complicate to print, so if DonwardAPI volume exists in the projected volume, just print "DownwardAPI" with no data. WDYT |
@neolit123 Done |
} | ||
} | ||
|
||
w.Write(LEVEL_2, "Type:\tProjected (a volume that contains injected data)\n") |
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.
s/a volume that contains injected data/a volume that contains injected data from multiple sources/
?
Sure, alternatively, we could print "true" or the amount of items referenced under downwardAPI
or
|
/lgtm Thanks, cc @soltysh for approval |
@soltysh @juanvallejo comments addressed, now the output looks like:
|
/test pull-kubernetes-verify |
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.
Also, please squash your changes into a single commit.
@@ -837,6 +839,31 @@ func printConfigMapVolumeSource(configMap *api.ConfigMapVolumeSource, w PrefixWr | |||
configMap.Name, optional) | |||
} | |||
|
|||
func printProjectedVolumeSource(projected *api.ProjectedVolumeSource, w PrefixWriter) { | |||
infos := []string{} |
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.
Why you're creating this array if you're then writing it, why not just directly writing to writer?
91bdc5d
to
240e214
Compare
@soltysh Comments addressed, PTAL |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh, WanLinghao 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 |
Currently, the output of
kubectl describe pod xxx
shows as below when it has projected volume bounds:After this patch, the output will be like