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
fix: 81134: display conflicted taint without a json representation #104011
Conversation
Welcome @manugupt1! |
Hi @manugupt1. 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 Once the patch is verified, the new status will be reflected by the 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
for _, taintAdd := range o.taintsToAdd { | ||
for _, taintRemove := range o.taintsToRemove { | ||
if taintAdd.Key != taintRemove.Key { | ||
continue | ||
} | ||
if len(taintRemove.Effect) == 0 || taintAdd.Effect == taintRemove.Effect { | ||
conflictTaint := fmt.Sprintf("{\"%s\":\"%s\"}", taintRemove.Key, taintRemove.Effect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's scrap the json altogether here and just make these key=value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't even output the conflicts for labels 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing the PR. I have made the requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddiezane before issuing the lgtm here, I think that your point in the second review is: we should also print the "conflictTaints" right?
@manugupt1 maybe doing something like:
return nil, nil, fmt.Errorf("can not both modify and remove a label in the same command: %v", conflictTaint)
or creating a pretifier just for this array would be really helpful for users :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rikatz sorry for the confusion. The return error is happening already below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Eddie, I didn't went to check above :)
/triage accepted |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eddiezane, manugupt1 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 |
/lgtm |
1. string is formatted as unsafe json. Since this is created on the fly; TaintKey and Effect are formatted as a 'key=effect' instead of '{key=effect}' eliminating unsafe json representation. 2. Adds a fail test case for the case where a taint with same key and effect is added and removed together. Manual Testing ``` ▶ ./_output/bin/kubectl taint nodes kind-control-plane key1=:NoSchedule key1=:NoSchedule- error: can not both modify and remove the following taint(s) in the same command: key1=NoSchedule ```
/test pull-kubernetes-integration Failure looks like it is related to calico. |
@rikatz can you please give another LGTM as I had rebased my changes earlier. Thanks! |
/lgtm |
/ok-to-test |
/retest |
From
#81134 (comment)
string is formatted as unsafe json. Since this is created on the fly;
TaintKey and Effect are formatted as a 'key=effect' instead of '{key=effect}'
eliminating unsafe json representation.
Adds a fail test case for the case where a taint with same key and effect is added
and removed together.
Manual Testing
Also, ran unit tests
What type of PR is this?
/kind bug
What this PR does / why we need it:
Earlier the string that was constructed was an unsafe json.
As recommended in #81134 I am adding an in-depth check
to not create a json as araw string.
Which issue(s) this PR fixes:
#81134
Special notes for your reviewer:
Please see Manual Testing to see that the user output did not change.
Does this PR introduce a user-facing change?