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

kubectl explain: adjust the description to make it more readable #121802

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ingvagabund
Copy link
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Make the description of kubectl explain command more readable

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ingvagabund
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added area/kubectl do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Nov 8, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 8, 2023
@ingvagabund ingvagabund force-pushed the adjust-kubectl-explain-description branch from 15974b5 to b799b3a Compare November 8, 2023 21:20
@k8s-ci-robot
Copy link
Contributor

@ingvagabund: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-node-e2e-containerd b799b3a link true /test pull-kubernetes-node-e2e-containerd

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.

@@ -35,12 +35,17 @@ import (

var (
explainLong = templates.LongDesc(i18n.T(`
Describe fields and structure of various resources.
Describe (sub-)fields and structure of various resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

I question how useful it is to denote subfields here. All the subfields are just simply fields of other fields, and it is demonstrated in the example text that there can be any number of fields that are children of resource fields.

This command describes the fields associated with each supported API resource.
Fields are identified via a simple JSONPath identifier:
This command describes the (sub-)fields associated with each supported API resource.
Fields are identified via a simple JSONPath accessor:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be selector or query instead of either identifier or accessor, because this is the language that the RFC uses.

Choose a reason for hiding this comment

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

The issue is that it’s not treated as a JSONPath query. It resembles JSONPath in that the field names are separated by dots. Otherwise the syntax isn’t JSONPath.

Describing it as a JSONPath selector or query would be a clear improvement if this were how the value were treated. But since it isn’t, it’s not much of a help in terms of aligning this part of the help text with the actual behavior.

As an example, JSONPath would allow “ explain storage.spec..proxy”. But it’s rejected here (error: field "" does not exist).

In my view, references to JSONPath suggest that the value will be treated as a JSONPath expression. It may be clearer to remove references to JSONPath, or to allow the use of JSONPath queries.

I can retest with the latest kubectl and post specific examples of rejected JSONPath expressions if that would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting, ok let me take a look at the backend code for this. This is more likely a bug with how I implemented the jsonpath support for explain.

That being said, it should be noted that jsonpath in Kubernetes is not full jsonpath. I've been meaning to extend the library to be more fully compliant with the jsonpath standard but have not been able to do so yet. But the .. should be acceptable in the library we have.

Choose a reason for hiding this comment

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

Thanks. I confirmed that it still doesn't work with v1.28.3. For example:

$ kubectl explain pods..securityContext
KIND:       Pod
VERSION:    v1

error: field "securityContext" does not exist

Interestingly, I get a different error from deployments than pods. I'm not sure how to address it, though. It could be helpful for the output to include a legal example.

$ kubectl explain deployments..securityContext
error: invalid jsonpath syntax, first node must be field node

As a quick fix, all help text and error messages could drop references to jsonpath and simply note that the field names are to be joined by dots.

Beyond that it could be helpful to allow JSONPath. Currently explain doesn't seem to take more than one resource type. So treating the input as JSONPath may raise the question of what happens when more than one field is matched.

Something like the following could be quite useful in directing users toward the right command:

Multiple fields matched. Try one of the following:

kubectl explain --api-version apps/v1 Deployment.spec.template.spec.containers.securityContext
kubectl explain --api-version apps/v1 Deployment.spec.template.spec.ephemeralContainers.securityContext
kubectl explain --api-version apps/v1 Deployment.spec.template.spec.initContainers.securityContext
kubectl explain --api-version apps/v1 Deployment.spec.template.spec.securityContext

Or something similar to the --recursive output, but limited to matching fields and their ancestors:

GROUP:      apps
KIND:       Deployment
VERSION:    v1

DESCRIPTION:
    Deployment enables declarative updates for Pods and ReplicaSets.

MATCHING FIELDS:
  spec  <DeploymentSpec>
    template    <PodTemplateSpec> -required-
      spec      <PodSpec>
        containers      <[]Container> -required-
          securityContext       <SecurityContext>
        ephemeralContainers     <[]EphemeralContainer>
          securityContext       <SecurityContext>
        initContainers  <[]Container>
          securityContext       <SecurityContext>
        securityContext <PodSecurityContext>

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not bias to quick fixes that simply update the help text. I think I know what the problem is, but I will have to investigate further. I'll try to get to this ASAP since I'm the one that implemented this in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pursuing this

Comment on lines +45 to +48
Subfield of each field is accessed through the same dotted notation:

<type>.<fieldName>[.<subfieldName>]
<type>.<fieldName>.<subfieldName>[.<subfieldName>]
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the help text references JSONPath, which is a well known query language, this feels unnecessarily verbose to me, especially when paired with the example section that displays how to access subfields for the user.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 18, 2024
@ingvagabund ingvagabund removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 27, 2024
@ingvagabund
Copy link
Contributor Author

@mpuckett159 @ChetHosey have you had a chance to take a closer look at this?

@ingvagabund
Copy link
Contributor Author

@mpuckett159 @ChetHosey ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

None yet

5 participants