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.

Sign up for GitHub

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

Jump to bottom

filtering ANSI escape characters #104962

Closed
wants to merge 1 commit into from
Closed

filtering ANSI escape characters #104962

wants to merge 1 commit into from

Conversation

RinkiyaKeDad
Copy link
Member

Signed-off-by: RinkiyaKeDad arshsharma461@gmail.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes ANSI escape characters not being filtered for some kubectl subcommands.

Which issue(s) this PR fixes:

Fixes #101695

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.:

/sig cli

@RinkiyaKeDad
filtering ANSI escape characters
8c9cb75 
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Sep 13, 2021
@k8s-ci-robot
Copy link
Contributor

@RinkiyaKeDad: 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RinkiyaKeDad
To complete the pull request process, please assign soltysh after the PR has been reviewed.
You can assign the PR to them by writing /assign @soltysh in a comment when ready.

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

@RinkiyaKeDad RinkiyaKeDad marked this pull request as ready for review September 13, 2021 11:54
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2021
@RinkiyaKeDad
Copy link
Member Author

/assign @soltysh

@pacoxu
Copy link
Member

pacoxu commented Sep 13, 2021

/cc

@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kubernetes-unit 8c9cb75 link /test pull-kubernetes-unit

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.

liggitt
liggitt reviewed Sep 14, 2021
View reviewed changes
staging/src/k8s.io/cli-runtime/pkg/printers/json.go
@@ -48,7 +49,8 @@ func (p *JSONPrinter) PrintObj(obj runtime.Object, w io.Writer) error {
if err != nil {
return err
}
_, err = w.Write(data)
stringLiteral := strconv.QuoteToASCII(string(data))
Copy link
Member

Choose a reason for hiding this comment

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

is the issue really contained to watch events output by the json printer?

I'm surprised it doesn't manifest in many places (output of kubectl describe, kubectl get, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

@liggitt do you suggest changing the _, err = w.Write(data) on line 79 instead?

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 there are probably lots of paths that let data in free-form string fields hit the console... -o json, -o yaml, maybe simple kubectl get / kubectl describe commands, etc

@pacoxu
Copy link
Member

pacoxu commented Sep 14, 2021 •
edited
Loading

I build a kubectl bin to test with your branch and it seems to be not solved.

EDITED: slack context about this issue https://kubernetes.slack.com/archives/C2GL57FJ4/p1631182928020600

@pacoxu pacoxu added this to In progress in SIG CLI Sep 15, 2021
@g3rzi
Copy link

g3rzi commented Oct 31, 2021

Hi,

Any update o this issue?

@RinkiyaKeDad
Copy link
Member Author

Hey @g3rzi, we are currently looking at this. You can check this slack thread for more recent updates 🙂

@dims
Copy link
Member

dims commented Jan 4, 2022

@RinkiyaKeDad Arsh, do we still need this? please reopen if needed

/close

@k8s-ci-robot
Copy link
Contributor

@dims: Closed this PR.

In response to this:

@RinkiyaKeDad Arsh, do we still need this? please reopen if needed

/close

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.

SIG CLI automation moved this from In progress to Done Jan 4, 2022
@RinkiyaKeDad
Copy link
Member Author

The issue still needs to be fixed @dims, but it's blocked on some things (more context here). Let's let this one remain closed - will open a new PR once we've decided how to fix it exactly.

@RSAlderman RSAlderman mentioned this pull request Jan 12, 2022
Closed
@wangyx1992
Copy link
Contributor

Hi, @RinkiyaKeDad. Is there any progress on this issue?

@RinkiyaKeDad
Copy link
Member Author

Hey @wangyx1992, I'm not actively following this issue now, so I'm not familiar with the latest updates, sorry. You can bring this up in #sig-cli on the Kuberenetes slack if you want to start the discussion again 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@pwittrock pwittrock Awaiting requested review from pwittrock

@seans3 seans3 Awaiting requested review from seans3

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@soltysh soltysh

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
SIG CLI
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ANSI escape characters in kubectl output are not being filtered
8 participants
@RinkiyaKeDad @k8s-ci-robot @pacoxu @g3rzi @dims @wangyx1992 @liggitt @soltysh