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

wire through custom-column print flags #61501

Merged
merged 1 commit into from Mar 27, 2018

Conversation

@juanvallejo
Copy link
Member

juanvallejo commented Mar 21, 2018

Release note:

NONE

Begin implementing pieces needed to retrieve custom-column printers from a set of flags.
Proposal: https://docs.google.com/document/d/19ZZFVe9oD1KQmk5uExggRWtRl_hKGfYnBXvHZJlgEro/edit#heading=h.pnvbfi14v4zz

cc @soltysh @deads2k @pwittrock

@k8s-ci-robot k8s-ci-robot requested review from eparis and sttts Mar 21, 2018

@juanvallejo juanvallejo force-pushed the juanvallejo:jvallejo/add-custom-columns-flags branch from ea03236 to 0d2ab18 Mar 21, 2018

}

if f.TemplateArgument != nil {
c.Flags().StringVar(f.TemplateArgument, "template", *f.TemplateArgument, "Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].")

This comment has been minimized.

@juanvallejo

juanvallejo Mar 21, 2018

Author Member

@soltysh @deads2k the --template flag is also bound by KubeTemplatePrintFlags here: https://github.com/kubernetes/kubernetes/pull/61304/files#diff-6ee7dad6fdeccf4e8fe8669b7080b962R95

A solution could be to entirely drop support for the --template flag when printing custom-columns, and expect users to specify a custom-columns template or template file via the --output flag: --output=custom-columns=NAME:.metadata.name

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

We can't do that, we need to maintain backwards compatibility.

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

Usually when talking about template we allow all three of them. Maybe we need an upper level struct that contains all 3 (custom, go-template and jsonpath) so they can share a single --template flag.

@soltysh soltysh self-assigned this Mar 22, 2018

@soltysh soltysh added this to the v1.11 milestone Mar 22, 2018

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Mar 22, 2018

/ok-to-test

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 22, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@juanvallejo @soltysh

Pull Request Labels
  • sig/cli: Pull Request will be escalated to these SIGs if needed.
  • priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
  • kind/feature: New functionality.
Help

@soltysh soltysh referenced this pull request Mar 22, 2018

Closed

Kubectl refactorings umbrella issue #61471

4 of 5 tasks complete
for format := range supportedFormats {
format = format + "="
if strings.HasPrefix(outputFormat, format) {
templateArg = outputFormat[len(format):]

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

Please use two distinct name, similarly like in #61304 so it's clear which is template format and which its value, aside from outputFormat which is your input parameter.

}

if f.TemplateArgument != nil {
c.Flags().StringVar(f.TemplateArgument, "template", *f.TemplateArgument, "Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].")

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

We can't do that, we need to maintain backwards compatibility.


decoder := scheme.Codecs.UniversalDecoder()

if outputFormat == "custom-columns-file" {

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

Ensure there's a test case covering this example.

}

if f.TemplateArgument != nil {
c.Flags().StringVar(f.TemplateArgument, "template", *f.TemplateArgument, "Template string or path to template file to use when -o=go-template, -o=go-template-file. The template format is golang templates [http://golang.org/pkg/text/template/#pkg-overview].")

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

Usually when talking about template we allow all three of them. Maybe we need an upper level struct that contains all 3 (custom, go-template and jsonpath) so they can share a single --template flag.

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Mar 22, 2018

The conflicting flags listed appear to be kubectl get only. I suspect that we can build a printer that wires them together much like the golang template pieces. I think this illustrates the flexibility of composing the options and demonstrates how kubectl get was previously polluting our printer stack for every other command we have.

@juanvallejo juanvallejo force-pushed the juanvallejo:jvallejo/add-custom-columns-flags branch from 0d2ab18 to ce0aaf4 Mar 22, 2018

@juanvallejo

This comment has been minimized.

Copy link
Member Author

juanvallejo commented Mar 22, 2018

@soltysh @deads2k thanks, review comments addressed.

@juanvallejo

This comment has been minimized.

Copy link
Member Author

juanvallejo commented Mar 23, 2018

/test pull-kubernetes-integration

2 similar comments
@juanvallejo

This comment has been minimized.

Copy link
Member Author

juanvallejo commented Mar 23, 2018

/test pull-kubernetes-integration

@juanvallejo

This comment has been minimized.

Copy link
Member Author

juanvallejo commented Mar 23, 2018

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 25, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm
/approve no-issue

return nil, true, fmt.Errorf("custom-columns format specified but no custom columns given")
}

decoder := scheme.Codecs.UniversalDecoder()

This comment has been minimized.

@soltysh

soltysh Mar 25, 2018

Contributor

Hmm... shouldn't that come from outside, iow. passed upon CustomColumnsPrintFlags creation in one of the commands? Can be done as a followup when wiring stuff together, let's discuss that o Monday.

return nil, true, fmt.Errorf("error reading template %s, %v\n", templateValue, err)
}
defer file.Close()
p, err := NewCustomColumnsPrinterFromTemplate(file, decoder)

This comment has been minimized.

@soltysh

soltysh Mar 25, 2018

Contributor

You can return p, true, err similarly to how you're doing that few lines below.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 25, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, soltysh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@juanvallejo juanvallejo force-pushed the juanvallejo:jvallejo/add-custom-columns-flags branch from ce0aaf4 to be9cff8 Mar 26, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 26, 2018

@juanvallejo juanvallejo force-pushed the juanvallejo:jvallejo/add-custom-columns-flags branch from be9cff8 to 491f033 Mar 26, 2018

@juanvallejo juanvallejo force-pushed the juanvallejo:jvallejo/add-custom-columns-flags branch from 491f033 to e4cdb9f Mar 26, 2018

@soltysh
Copy link
Contributor

soltysh left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 26, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Mar 27, 2018

Automatic merge from submit-queue (batch tested with PRs 61434, 61501, 59609, 61467, 61531). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit dcdf70a into kubernetes:master Mar 27, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation juanvallejo authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@juanvallejo juanvallejo deleted the juanvallejo:jvallejo/add-custom-columns-flags branch Mar 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.