Beautify kubectl help flag commands #104736
Conversation
k8s-ci-robot
added
kind/design
| @@ -90,7 +91,7 @@ func (templater *templater) HelpFunc() func(*cobra.Command, []string) { | |||
| t := template.New("help") | |||
| t.Funcs(templater.templateFuncs()) | |||
| template.Must(t.Parse(templater.HelpTemplate)) | |||
| out := term.NewResponsiveWriter(c.OutOrStdout()) | |||
| out := c.OutOrStderr() | |||
There was a problem hiding this comment.
Have to remove this and wrap each template string separately, or else for the help command -o output, ResponsiveWriter will move it to next line (as shown in the picture)
There was a problem hiding this comment.
Discussed with @eddiezane , we agreed it's fine
|
/sig cli |
k8s-ci-robot
added
sig/cli
|
/cc @soltysh |
lauchokyip
force-pushed
the
improveFlags
branch
4 times, most recently
from
92f1c69
to
357695a
Compare
|
/retest |
lauchokyip
force-pushed
the
improveFlags
branch
from
357695a
to
6c7b27e
Compare
k8s-ci-robot
added
the
lgtm
There was a problem hiding this comment.
How much of this is because of the kubectl get output types?
We can change that output if it simplifies this.
Example:
diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/get/get_flags.go b/staging/src/k8s.io/kubectl/pkg/cmd/get/get_flags.go
index 4bfd306975c..c32e688c110 100644
--- a/staging/src/k8s.io/kubectl/pkg/cmd/get/get_flags.go
+++ b/staging/src/k8s.io/kubectl/pkg/cmd/get/get_flags.go
@@ -161,7 +161,7 @@ func (f *PrintFlags) AddFlags(cmd *cobra.Command) {
f.CustomColumnsFlags.AddFlags(cmd)
if f.OutputFormat != nil {
- cmd.Flags().StringVarP(f.OutputFormat, "output", "o", *f.OutputFormat, fmt.Sprintf("Output format. One of: %s See custom columns [https://kubernetes.io/docs/reference/kubectl/overview/#custom-columns], golang template [http://golang.org/pkg/text/template/#pkg-overview] and jsonpath template [https://kubernetes.io/docs/reference/kubectl/jsonpath/].", strings.Join(f.AllowedFormats(), "|")))
+ cmd.Flags().StringVarP(f.OutputFormat, "output", "o", *f.OutputFormat, fmt.Sprintf("Output format. One of: \"%s\" See custom columns [https://kubernetes.io/docs/reference/kubectl/overview/#custom-columns], golang template [http://golang.org/pkg/text/template/#pkg-overview] and jsonpath template [https://kubernetes.io/docs/reference/kubectl/jsonpath/].", strings.Join(f.AllowedFormats(), " ")))
}
if f.NoHeaders != nil {
cmd.Flags().BoolVar(f.NoHeaders, "no-headers", *f.NoHeaders, "When using the default or custom-column output format, don't print headers (default print headers).")| stdout := os.Stdout | ||
| fd := stdout.Fd() | ||
| if !term.IsTerminal(fd) { | ||
| panic("file descriptor is not a terminal") |
There was a problem hiding this comment.
We should be able to return an error from the calling function.
There was a problem hiding this comment.
Yes we can but in the flagUsages, it's returning a string to the template so I wasn't sure returning an error is a good idea
func flagsUsages(f *flag.FlagSet) string {
flagBuf := new(bytes.Buffer)
printer := NewHelpFlagPrinter(flagBuf)
wrapLimit := term.GetWordWrapperLimit()
minFlagLen := uint(getMinFlagLen(f))
maxFlagLen := uint(getMaxFlagLen(f))
tabWidth := maxFlagLen - minFlagLen // the maximum width that will be padded by tabWriter
f.VisitAll(func(flag *flag.Flag) {
if flag.Hidden {
return
}
printer.PrintHelpFlags(flag, wrapLimit, tabWidth, minFlagLen, maxFlagLen)
})
printer.FlushTabWriter()
return flagBuf.String()
}flagUsages will be parsed as one of the templates function over here
func (templater *templater) templateFuncs(exposedFlags ...string) template.FuncMap {
return template.FuncMap{
"trim": strings.TrimSpace,
"trimRight": func(s string) string { return strings.TrimRightFunc(s, unicode.IsSpace) },
"trimLeft": func(s string) string { return strings.TrimLeftFunc(s, unicode.IsSpace) },
"gt": cobra.Gt,
"eq": cobra.Eq,
"rpad": rpad,
"appendIfNotPresent": appendIfNotPresent,
"flagsNotIntersected": flagsNotIntersected,
"visibleFlags": visibleFlags,
"flagsUsages": flagsUsages,
"cmdGroups": templater.cmdGroups,
"cmdGroupsString": templater.cmdGroupsString,
"rootCmd": templater.rootCmdName,
"isRootCmd": templater.isRootCmd,
"optionsCmdFor": templater.optionsCmdFor,
"usageLine": templater.usageLine,
"exposed": func(c *cobra.Command) *flag.FlagSet {
exposed := flag.NewFlagSet("exposed", flag.ContinueOnError)
if len(exposedFlags) > 0 {
for _, name := range exposedFlags {
if flag := c.Flags().Lookup(name); flag != nil {
exposed.AddFlag(flag)
}
}
}
return exposed
},
}
}There was a problem hiding this comment.
I was thinking since template.FuncMap functions can return an error that you could pass it from flagsUsages as well. Not sure what the behavior is though for errors there.
There was a problem hiding this comment.
template.FuncMap is intended to replace the input and output a string for the template
There was a problem hiding this comment.
I took a quick look at this and returning an error from those can be handled gracefully here.
_output/bin/kubectl get --help 1>/dev/null
Error: template: usage:12:43: executing "usage" at <flagsUsages $visibleFlags>: error calling flagsUsages: file descriptor is not a terminal
diff --git a/staging/src/k8s.io/kubectl/pkg/util/templates/templater.go b/staging/src/k8s.io/kubectl/pkg/util/templates/templater.go
index d2d5e6d87ed..6914d0cda4f 100644
--- a/staging/src/k8s.io/kubectl/pkg/util/templates/templater.go
+++ b/staging/src/k8s.io/kubectl/pkg/util/templates/templater.go
@@ -218,11 +218,14 @@ func (t *templater) usageLine(c *cobra.Command) string {
return usage
}
-func flagsUsages(f *flag.FlagSet) string {
+func flagsUsages(f *flag.FlagSet) (string, error) {
flagBuf := new(bytes.Buffer)
printer := NewHelpFlagPrinter(flagBuf)
- wrapLimit := term.GetWordWrapperLimit()
+ wrapLimit, err := term.GetWordWrapperLimit()
+ if err != nil {
+ return "", err
+ }
minFlagLen := uint(getMinFlagLen(f))
maxFlagLen := uint(getMaxFlagLen(f))
tabWidth := maxFlagLen - minFlagLen // the maximum width that will be padded by tabWriter
@@ -234,7 +237,7 @@ func flagsUsages(f *flag.FlagSet) string {
printer.PrintHelpFlags(flag, wrapLimit, tabWidth, minFlagLen, maxFlagLen)
})
printer.FlushTabWriter()
- return flagBuf.String()
+ return flagBuf.String(), nil
}
func getFlagFormatWithTab(f *flag.Flag) string {
diff --git a/staging/src/k8s.io/kubectl/pkg/util/term/term_writer.go b/staging/src/k8s.io/kubectl/pkg/util/term/term_writer.go
index 26d429ad033..17d59e25832 100644
--- a/staging/src/k8s.io/kubectl/pkg/util/term/term_writer.go
+++ b/staging/src/k8s.io/kubectl/pkg/util/term/term_writer.go
@@ -17,6 +17,7 @@ limitations under the License.
package term
import (
+ "errors"
"io"
"os"
@@ -74,15 +75,15 @@ func NewWordWrapWriter(w io.Writer, limit uint) io.Writer {
}
}
-func GetWordWrapperLimit() uint {
+func GetWordWrapperLimit() (uint, error) {
stdout := os.Stdout
fd := stdout.Fd()
if !term.IsTerminal(fd) {
- panic("file descriptor is not a terminal")
+ return 0, errors.New("file descriptor is not a terminal")
}
terminalSize := GetSize(fd)
if terminalSize == nil {
- panic("terminal size is nil")
+ return 0, errors.New("terminal size is nil")
}
var limit uint
switch {
@@ -93,7 +94,7 @@ func GetWordWrapperLimit() uint {
case terminalSize.Width >= 80:
limit = 80
}
- return limit
+ return limit, nil
}
func (w wordWrapWriter) Write(p []byte) (nn int, err error) {
@eddiezane When I was testing most of the commands, I think the flags |
And if we drop those characters and change the output does it reduce this PR at all? |
I don't think so 😅 , getting rid of these characters will only prevent the wrapper to add a newline character to strings with |
k8s-ci-robot
added
sig/testing
lauchokyip
force-pushed
the
improveFlags
branch
from
e7de473
to
1c031d4
Compare
k8s-ci-robot
added
size/L
|
/hold cancel |
k8s-ci-robot
added
approved
|
somehow messed up the rebasing, sorry if it creates too much noise |
|
/lgtm |
|
@lauchokyip: you cannot LGTM your own 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/test-infra repository. |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
|
/retest |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brianpursley, dixudx, eddiezane, lauchokyip 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 |
|
/remove sig-windows |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
|
@lauchokyip: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
What type of PR is this?
/kind design
What this PR does / why we need it:
Make the help flags of kubectl more readable / user friendly
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#1069
Special notes for your reviewer:
Before,
After,
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: