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

Add json,yaml output format support to kubectl create, kubectl apply #38112

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Dec 5, 2016

Fixes: #37390

Release note:

Added support for printing in all supported `--output` formats to `kubectl create ...` and `kubectl apply ...`

This patch adds the ability to specify an output format other than
"name" to kubectl create .... It can be used in conjunction with the
--dry-run option. Converts unstructured objects into known types in
order to support all --output values.

The patch prints *resource.Infos returned by the server. If a resource does not yet exist (and the --dry-run option is not set), the resource is created and printed in the specified format.

@kubernetes/cli-review @fabianofranz

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 5, 2016
@k8s-reviewable
Copy link

This change is Reviewable

func decodeUnstructuredObject(obj runtime.Object) (runtime.Object, bool) {
switch obj.(type) {
case *runtime.UnstructuredList, *runtime.Unstructured, *runtime.Unknown:
if objBytes, err := runtime.Encode(api.Codecs.LegacyCodec(), obj); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very, very wrong. What are you trying to do?

Copy link
Contributor Author

@juanvallejo juanvallejo Dec 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deads2k

This looks very, very wrong. What are you trying to do?

I'm attempting to convert to a known type, I used this implementation in filter.go as an example.

@deads2k
Copy link
Contributor

deads2k commented Dec 5, 2016

Is the output what I sent or what I got back. I expect it to be what I got back.

@deads2k
Copy link
Contributor

deads2k commented Dec 5, 2016

I'm attempting to convert to a known type, I used this implementation in filter.go as an example.

Why would I want to do that? The whole point of these commands is to be generic if possible.

@juanvallejo
Copy link
Contributor Author

@deads2k

Why would I want to do that? The whole point of these commands is to be generic if possible.

--output wide returned an error such as:

error: unknown type &runtime.Unstructured{Object:map[string]interface {}{"status":map[string]interface {}{"lastVersion":1}, "kind":"BuildConfig", "apiVersion":"v1", "metadata":map[string]interface {}{"name":"gitauthtest", "namespace":"default", "selfLink":"/oapi/v1/namespaces/default/buildconfigs/gitauthtest", "uid":"39b2c5a1-b1b3-11e6-9a01-507b9dac96e1", "resourceVersion":"800167", "creationTimestamp":"2016-11-23T19:30:00Z", "labels":map[string]interface {}{"app":"gitauthtest", "template":"gitserver"}, "annotations":map[string]interface {}{"openshift.io/generated-by":"OpenShiftNewApp"}}, "spec":map[string]interface {}{"runPolicy":"Serial", "source":map[string]interface {}{"type":"Git", "git":map[string]interface {}{"uri":"http://gitserver-tokenauth.linux.xip.io/ruby-hello-world"}, "sourceSecret":map[string]interface {}{"name":"builder-token-nbme5"}}, "strategy":map[string]interface {}{"type":"Source", "sourceStrategy":map[string]interface {}{"from":map[string]interface {}{"kind":"ImageStreamTag", "namespace":"openshift", "name":"ruby:latest"}}}, "output":map[string]interface {}{}, "resources":map[string]interface {}{}, "postCommit":map[string]interface {}{}, "nodeSelector":interface {}(nil), "triggers":[]interface {}{}}}}

if I did not convert the object here, however another solution in the downstream PR was also to just output something like Output format not supported if a printer required object conversion.

@deads2k
Copy link
Contributor

deads2k commented Dec 5, 2016

--output wide returned an error such as:

I'd rather see the conversion contained in something like the printer that knows what it cares about. As we move GET to the server, this should fall out of the flow

@fabianofranz
Copy link
Contributor

@kubernetes/kubectl

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Dec 5, 2016

@deads2k

I'd rather see the conversion contained in something like the printer that knows what it cares about. As we move GET to the server, this should fall out of the flow

Thanks for the feedback. It looks like runtime.Unstructurted objects are already being converted to known types in the HumanReadablePrinter's printObj method. I went ahead and updated create.go so that a printer with an unversioned mapping is used, which allows me to send an unstructured info.Object to this method.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2016
@mengqiy
Copy link
Member

mengqiy commented Dec 5, 2016

@juanvallejo Do you have another PR to fix the similar issue with kubectl apply. Ref: #34028 (comment)

@juanvallejo
Copy link
Contributor Author

@ymqytw

Do you have another PR to fix the similar issue with kubectl apply

Sorry about that, I had forgotten to address this. Will add as part of this PR. Thanks!

@juanvallejo juanvallejo changed the title Add json,yaml output format support kubectl create Add json,yaml output format support to kubectl create, kubectl apply Dec 5, 2016
@juanvallejo juanvallejo force-pushed the jvallejo/add-output-format-support-kubectl-create branch from 63c4d8b to 1112a57 Compare December 5, 2016 22:34
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Dec 6, 2016

@ymqytw
Updated kubectl apply to support different output formats as well, PTAL c6658ea

Sample output

$ kubectl apply -f https://raw.githubusercontent.com/mdshuai/testfile-openshift/master/petset/hello-petset.yaml -o wide
NAME      CLUSTER-IP   EXTERNAL-IP   PORT(S)   AGE       SELECTOR
foo       None         <none>        80/TCP    9s        app=hello-pod
NAME           DESIRED   CURRENT   AGE
hello-petset   2         2         8s        hello-pod   docker.io/deshuai/hello-pod:latest   app=hello-pod

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 6, 2016
@@ -251,6 +251,20 @@ func RunApply(f cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *App
}

count++
if len(output) > 0 && !shortOutput {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract this code into pkg/kubectl - appears to be identical across all three.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton done! extracted this out to kubectl/cmd/util/printing.go PTAL

if err != nil {
return err
}
return printer.PrintObj(info.Object, out)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is using the printer declared in line 142 although I realize that this entire part could read a bit better. I'll go ahead and rename the printer declared inside of the !generic || printer == nil block to something a bit more specific, like nonGenericPrinter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ymqytw

This line is using the printer declared in line 142 although I realize that this entire part could read a bit better. I'll go ahead and rename the printer declared inside of the !generic || printer == nil block to something a bit more specific, like nonGenericPrinter

Sorry, misunderstood which line you were pointing out. I see what you mean, I'll go ahead and update. Please ignore the above comment.

@mengqiy
Copy link
Member

mengqiy commented Dec 7, 2016

This PR should have a release note, since it changes the output behavior.

@juanvallejo juanvallejo force-pushed the jvallejo/add-output-format-support-kubectl-create branch from 66f3121 to 3249527 Compare December 7, 2016 14:58
@juanvallejo
Copy link
Contributor Author

@ymqytw

This PR should have a release note, since it changes the output behavior.

Added a release note

@juanvallejo
Copy link
Contributor Author

@fabianofranz or @ymqytw Is this patch okay to test? Thanks!

@mengqiy
Copy link
Member

mengqiy commented Dec 8, 2016

@k8s-bot ok to test

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 29e65475f4460f9c362c79c689b73f28c3e1fa22. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 29e65475f4460f9c362c79c689b73f28c3e1fa22. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@juanvallejo juanvallejo force-pushed the jvallejo/add-output-format-support-kubectl-create branch from 29e6547 to 3916587 Compare December 8, 2016 20:36
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 29e65475f4460f9c362c79c689b73f28c3e1fa22. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed release-note-label-needed labels Dec 10, 2016
This patch adds the ability to specify an output format other than
"name" to `kubectl create ...`. It can be used in conjunction with the
`--dry-run` option. Converts unstructured objects into known types in
order to support all `--output` values.
@juanvallejo juanvallejo force-pushed the jvallejo/add-output-format-support-kubectl-create branch from 3916587 to cbe4790 Compare December 12, 2016 21:09
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2016
@juanvallejo
Copy link
Contributor Author

@ymqytw or @deads2k are there any more comments on this patch? Thanks!

@deads2k
Copy link
Contributor

deads2k commented Dec 14, 2016

The description still isn't clear to me. Is this showing me what was submitted or what was actually created (returned from the API). You definitely need tests.

Also, the printing API needs a significant overhaul. I'll let @fabianofranz decide what order things need to happen. I think this reduces overall readability.

@deads2k deads2k assigned fabianofranz and unassigned deads2k Dec 14, 2016
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Dec 14, 2016

@deads2k

The description still isn't clear to me. Is this showing me what was submitted or what was actually created (returned from the API).

Thanks, went ahead and updated the description to clarify this. The printed objects are the ones returned from the server. If an info is returned and does not yet exist in the server (and the --dry-run option was not also provided with the --output flag), then it is first created and then it is printed in the format specified.

@juanvallejo
Copy link
Contributor Author

@deads2k

Also, the printing API needs a significant overhaul. I'll let @fabianofranz decide what order things need to happen. I think this reduces overall readability.

@fabianofranz I agree that adding another helper to handle printing resources does not feel completely right. The reason why the new helper PrintResourceInfoForCommand was added in this PR was to handle cases where --output wide would cause a panic due to the way it is currently handled in kubectl.GetPrinter. Perhaps the best solution would be to update that line in resource_printer.go to return a NewHumanReadablePrinter, or maybe have it actually return an error if it is going to return a nil printer and handle returning a HumanReadablePrinter with the correct PrintOptions in cmdutil.PrinterForCommand.

Maybe this is something that could be added to the kubectl umbrella issue for printers?

@fabianofranz
Copy link
Contributor

Added #38779 to the printers and describers umbrella. I agree it needs a significant overhaul which we are proposing to do after an analysis of the existing issues, but let's do that in a separate effort.

@fabianofranz
Copy link
Contributor

Any other comments here?

@fabianofranz fabianofranz added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2016
@fabianofranz
Copy link
Contributor

@k8s-bot bazel test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3951ae4 into kubernetes:master Jan 3, 2017
@juanvallejo juanvallejo deleted the jvallejo/add-output-format-support-kubectl-create branch January 9, 2017 15:11
waseem added a commit to waseem/kubernetes that referenced this pull request 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 pull request 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 pull request 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
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants