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

Skip smart label in kubectl describe if field has special chars #75483

Merged
merged 1 commit into from Apr 1, 2019

Conversation

@gsadhani
Copy link
Contributor

commented Mar 19, 2019

Attempts to create smart label for fields containing special character ends
up looking very odd. This change skips creating smart labels for fields
containing special characters other than '-'.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Attempts to create smart label for fields containing special character ends
up looking very odd. This change skips creating smart labels for fields
containing special characters other than '-'.

<             Name:                                                          cluster-1-subnet-private
<             Kubernetes . Io / Cluster / Cluster - 1:                       owned
<             Sigs . K 8 S . Io / Cluster - API - Provider - Aws / Managed:  true
<             Sigs . K 8 S . Io / Cluster - API - Provider - Aws / Role:     common
<           Availability Zone:                                               us-east-1a
<           Cidr Block:                                                      10.0.1.0/24

>             Name:                                          cluster-1-subnet-private
>             kubernetes.io/cluster/cluster-1:               owned
>             sigs.k8s.io/cluster-api-provider-aws/managed:  true
>             sigs.k8s.io/cluster-api-provider-aws/role:     common
>           Availability Zone:                               us-east-1a
>           Cidr Block:                                      10.0.1.0/24

Which issue(s) this PR fixes:

Fixes kubernetes-sigs/cluster-api-provider-aws#260

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

In the 'kubectl describe' output, the fields with names containing special characters are displayed as-is without any pretty formatting. 
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Hi @gsadhani. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /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 by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 requested review from seans3 and soltysh Mar 19, 2019

@gsadhani

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

/assign @apelisse

@gsadhani

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@apelisse, @detiber I have addressed the comments. Is there anything else I need to fix?

// skip creating smart label if field name contains
// special characters other than '-'
if strings.IndexFunc(field, func(r rune) bool {
return (r < 'A' || r > 'Z') && (r < 'a' || r > 'z') && r != '-'

This comment has been minimized.

Copy link
@apelisse

This comment has been minimized.

Copy link
@gsadhani

gsadhani Mar 27, 2019

Author Contributor

Replaced the range check with unicode.IsLetter.

@k8s-ci-robot k8s-ci-robot added size/S and removed size/XS labels Mar 27, 2019

@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thanks!

/lgtm
/ok-to-test
/approve

@detiber

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

It looks like the tests will need to be updated for this change:

func TestDescribeUnstructuredContent(t *testing.T) {

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 27, 2019

@detiber

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

/test pull-kubernetes-e2e-gce

@gsadhani

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

/test pull-kubernetes-e2e-gce

Looks like a recent merge has introduced test issues: #75775
#75776

@msau42

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Those failing tests should not run in the pull job.

@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Those failing tests should not run in the pull job.

What do you mean @msau42 ?

@msau42

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

The test failure bugs linked above are not run in the pull jobs
#75775: This only runs in slow jobs
#75776: This is only failing for multizone/regional environments

@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Ok thanks, can you add more tests? (sorry for pushing back with more nits all the time ...)

@gsadhani

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Ok thanks, can you add more tests? (sorry for pushing back with more nits all the time ...)

Added more variations in test data in the test TestDescribeUnstructuredContent.

@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

I guess you could have created a test for smartLabelFor instead, but that's fine.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 29, 2019

@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

/hold

Can you squash your commits please?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, gsadhani

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

Skip smart label in kubectl describe if field has special chars
Attempts to create smart label for fields containing special chanracter ends
up looking very odd. This change skips creating smart labels for fields
containing special characters other than '-'.

@gsadhani gsadhani force-pushed the gsadhani:describe-output-fix branch from 6d77e01 to 029582d Mar 29, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 29, 2019

@gsadhani

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

/hold

Can you squash your commits please?

Squashed.

@apelisse

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 29, 2019

@soltysh

soltysh approved these changes Apr 1, 2019

Copy link
Contributor

left a comment

/hold cancel
/lgtm
/priority important-longterm

@k8s-ci-robot k8s-ci-robot merged commit 483cd0d into kubernetes:master Apr 1, 2019

17 checks passed

cla/linuxfoundation gsadhani authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@gsadhani gsadhani deleted the gsadhani:describe-output-fix branch Apr 1, 2019

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.