revert "fix wrong output when using jsonpath" #104172
Conversation
k8s-ci-robot
added
release-note-none
|
Hi @atiratree. 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. |
k8s-ci-robot
added
sig/api-machinery
|
/assign @caesarxuchao @soltysh |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
These errors doesn't seem to be legit. |
There was a problem hiding this comment.
/lgtm
/approve
@atiratree can you open a cherry-pick against 1.22 to fix this problem since that's where the regression was introduced.
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
k8s-ci-robot
removed
the
needs-priority
|
ping @caesarxuchao for approval |
| "when range is used in a certain way in script, additional line is printed", | ||
| `{range .items[?(.status.phase=="Running")]}{.metadata.name}{" is Running\n"}`, | ||
| "range over pods without selecting the last one", | ||
| `{range .items[?(.status.phase=="Running")]}{.metadata.name}{" is Running\n"}{end}`, |
There was a problem hiding this comment.
shouldn't we have both cases tested, ie with and without end?
There was a problem hiding this comment.
Should we test invalid syntaxes here? What should be the behaviour?
- the output is broken and outputs additional line (current behaviour)
- or should we fast fail with invalid syntax error? (would require changes)
There was a problem hiding this comment.
it was kind of graceful before to leave out the {end}, just a little wrong. Could be get it working easily too?
There was a problem hiding this comment.
In any case, I would rather try to avoid breaking behaviour that ran without error before. Fixing it is ok, but not suddenly fail.
There was a problem hiding this comment.
I am not sure how easy that would be.
But do we want to support an invalid syntax? I think it is a bit problematic of supporting this especially when you consider nested ranges. I would prefer an error here , but that would also break some scripts (more than keeping it failing slightly).
There was a problem hiding this comment.
I think that the support for missing {end} was rather accidental than intentional. I'd prefer we stick with having code simple and complying to jsonpath standard, than over-complicated to support something not entirely correct.
There was a problem hiding this comment.
I would prefer keeping graceful behaviour and not fix it and break users. But I am not blocking this. Both paths are acceptable.
This partially reverts commit 39cfe232325d66bcdbc935af7aaf7022562e7010and PR kubernetes#98057 the original problem was caused by not using {end} at the end of the range
This behaviour was broken by commit 39cfe23 and PR kubernetes#98057
atiratree
force-pushed
the
revert-jsonpath-fix
branch
from
d706cc0
to
b79859c
Compare
k8s-ci-robot
removed
the
lgtm
|
just split this into two commits - one for revert and one for the extra test |
|
ping @soltysh @caesarxuchao |
k8s-ci-robot
added
the
lgtm
|
/approve with #104172 (comment) in mind. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: atiratree, knight42, soltysh, sttts 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 |
k8s-ci-robot
added
the
approved
|
the failures don't seem connected to this PR |
|
If not too much trouble, can we link this PR as the fix for #104276? Thank you. |
done |
…04172-upstream-release-1.22 Automated cherry pick of #104172: revert "fix wrong output when using jsonpath"
k8s-ci-robot
added
release-note
What type of PR is this?
/kind bug
/kind regression
What this PR does / why we need it:
This partially reverts commit #98057
the original problem was caused by not using {end} at the end of the range. Please see attached jsonpath documentation on using range.
The reverted commit causes a new bug of not parsing missing keys incorrectly (added a unit test for this use case)
Which issue(s) this PR fixes:
Fixes #104276
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: