Allow brackets inside Tekton expressions #376
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
tekton-robot
added
the
size/M
|
/hold |
tekton-robot
added
the
do-not-merge/hold
Manually verified with @afrittoli that he does indeed consent to Co-Authorship 😝 |
dibyom
added
cncf-cla: yes
|
/cla yes |
|
Looks like I can't manually update the cla label anymore 😞 @afrittoli you might have to comment /hold cancel |
tekton-robot
removed
the
do-not-merge/hold
|
@googlebot I consent |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
/lgtm |
| return results | ||
| } | ||
| // Splits string on $( to find potential Tekton expressions | ||
| maybeExpressions := strings.Split(in, "$(") |
There was a problem hiding this comment.
How do we want to handle expressions like $($())? Let's also add a test to verify the behavior.
There was a problem hiding this comment.
yeah, so this is the interesting one 😄
currently, we'd return $() i.e. the inner expression
There was a problem hiding this comment.
$ is usually the root element in JSONPath though in the kubectl jsonpath template syntax that we are using does not mention $ at all
There was a problem hiding this comment.
kubectl get pod -o=jsonpath='{$(.items[0])}'
error: error parsing jsonpath {$(.items[0])}, unrecognized character in action: U+0028 '('
There was a problem hiding this comment.
so, IMO we can document that if you have embed $()...we'll just use the innermost one e.g. "$($($(blahblah)))" will return $(blahblah)
what do you think @wlynch @afrittoli ?
There was a problem hiding this comment.
the innermost rule applies and we get $(foo)
There was a problem hiding this comment.
Returning a error is definitely an option though documented/defined behavior for nested $( is also an option. Don't feel too strongly about either
There was a problem hiding this comment.
Ignoring the -bar bit feels unexpected to me. I'm leaning towards making it an error.
There was a problem hiding this comment.
The bar is only ignored as part of the jsonpath expression...when we do the repalcements -> it still shows up as part of the string i,e, if $(foo) evals to "abc" the value of the binding is $($(abc)-bar)
There was a problem hiding this comment.
Ah! Okay, documenting that as a first step sounds good to me then! We can always revisit this later.
tekton-robot
added
size/L
We were previously using a regex to extract expressions within TriggerBindings. The regex only extracted between the initial `$(` and the first occurrence of the `)`. This meant that some valid JSONPath expressions that contained brackets (e.g. filters) were incorrectly extracted. This commit fixes the issue by splitting the string on `$(` and then extracting until the first "unbalanced" occurrence of `)`. Fixes tektoncd#365 Co-Authored-By: Andrea Frittoli <[email protected]> Signed-off-by: Dibyo Mukherjee <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wlynch 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 |
tekton-robot
added
the
approved
We were previously using a regex to extract expressions within
TriggerBindings. The regex only extracted between the initial
$(and the first occurrence of the
). This meant that some validJSONPath expressions that contained brackets (e.g. filters) were
incorrectly extracted.
This commit fixes the issue by splitting the string on
$(and thenextracting until the first unbalanced occurrence of
).Fixes #365
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes