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.

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

Update kubectl.GetPrinter to not return both nil for the printer and nil for the error on wide format #38779

Closed
juanvallejo opened this issue Dec 14, 2016 · 1 comment · Fixed by #46265
Labels
area/kubectl help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@juanvallejo
Copy link
Contributor

juanvallejo commented Dec 14, 2016

Problem

Right now, printer helpers such as cmdutil.PrinterForCommand call kubectl.GetPrinter with the output format specified by a --output flag, in order to retrieve the right printer for that format.

When kubect.GetPrinter is called with a wide format, nil is returned for both the printer and the error. This has inevitably led to panics throughout kubectl commands whenever code that calls kubectl.GetPrinter (either directly or indirectly through cmdutil.PrinterForCommand`) attempts to use the retrieved printer if no errors are returned. For example:

// $ kubectl ... -o wide

outputFormat := cmdutil.GetFlagString(cmd, "output")
printer, generic, err := kubectl.GetPrinter(outputFormat, ...)
if err != nil {
    // handle error
    return
}

// any code here assumes no error AND that the printer exists, however since both the printer and the 
// error were nil for the "wide" outputFormat, the line below will cause a panic
printer.PrintObj(info.Object, out)

Commands like kubectl create and kubectl apply get around this by only supporting the name output format, however when expanding these commands to eventually use all supported output formats by resource printers (json, yaml, custom-columns, etc...), this becomes an issue.

Proposal

  1. A solution to this could either be to actually return an error in kubect.GetPrinter if the returned printer will be nil
  • which would then allow existing helpers such as cmdutil.PrinterForCommand to handle this and return a NewHumanReadablePrinter with the right PrintOptions for example,
  1. or to simply return a NewHumanReadablePrinter in kubectl.GetPrinter any time the output format specified == wide.

cc @fabianofranz @pwittrock @adohe @deads2k

@fabianofranz fabianofranz added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Dec 15, 2016
@fabianofranz
Copy link
Contributor

@kubernetes/sig-cli @kubernetes/kubectl

@fabianofranz fabianofranz added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Apr 25, 2017
waseem added a commit to waseem/kubernetes that referenced this issue May 31, 2017
This fixes kubernetes#38779.

This allows us to avoid case in which printers.GetStandardPrinter
returns nil for both printer and err removing any potential panics that
may arise throughout kubectl commands.

Please see kubernetes#38779 and kubernetes#38112 for complete context.

Add comment explaining adding handlers to printers.HumanReadablePrinter
also remove an unnecessary conversion of printers.HumanReadablePrinter
to printers.ResourcePrinter.
k8s-github-robot pushed a commit that referenced this issue Jun 3, 2017
Automatic merge from submit-queue (batch tested with PRs 41563, 45251, 46265, 46462, 46721)

Denote if a printer is generic.

This fixes #38779.

This allows us to avoid case in which printers.GetStandardPrinter
returns nil for both printer and err removing any potential panics that
may arise throughout kubectl commands.

Please see #38779 and #38112 for complete context.
mrIncompetent pushed a commit to kubermatic/kubernetes that referenced this issue Jun 6, 2017
This fixes kubernetes#38779.

This allows us to avoid case in which printers.GetStandardPrinter
returns nil for both printer and err removing any potential panics that
may arise throughout kubectl commands.

Please see kubernetes#38779 and kubernetes#38112 for complete context.

Add comment explaining adding handlers to printers.HumanReadablePrinter
also remove an unnecessary conversion of printers.HumanReadablePrinter
to printers.ResourcePrinter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants