Nested TriggerBinding expressions $($()) - Githubissues

tektoncd / triggers

Event triggering with Tekton!

Apache License 2.0

558 stars 420 forks source link

Nested TriggerBinding expressions $($()) #385

Closed dibyom closed 3 years ago

dibyom commented 4 years ago

Following the discussion in https://github.com/tektoncd/triggers/pull/376#discussion_r370209602 let's discuss what the behavior should be for nested Tekton expressions in TriggerBindings

Current documented behavior

Nested expressions are allowed. However, only the text in innermost $() is evaluated as JSONPath, the rest is treated as regular strings. From the docs:

If the $() is embedded inside another $() we will use the contents of the innermost $() as the JSONPath expression

$($(body.b)) -> $(body.b)
$($($(body.b))) -> $(body.b)

Questions

Is there an actual use case for this?
Should we just return an error instead of the behavior above?

dibyom commented 4 years ago

/kind question /cc wlynch

vtereso commented 4 years ago

In this format, it seems like we are allowing for the inner value to be resolved recursively rather than just taking the text. I think an error is probably better?

tekton-robot commented 4 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot commented 4 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

tekton-robot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

tekton-robot commented 4 years ago

@tekton-robot: Closing this issue.

In response to [this](https://github.com/tektoncd/triggers/issues/385#issuecomment-673669605): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >/close > >Send feedback to [tektoncd/plumbing](https://github.com/tektoncd/plumbing). Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

dibyom commented 4 years ago

This is another "unintentional" feature/bug that we should fix

tekton-robot commented 4 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

dibyom commented 3 years ago

Still a bug that we need to fix :)

tekton-robot commented 3 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale with a justification. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close with a justification. If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

jmcshane commented 3 years ago

@dibyom what is the "correct" behavior here? has that ever been decided?

dibyom commented 3 years ago

Leaning towards just making this an error.

dibyom commented 3 years ago

/close

tekton-robot commented 3 years ago

@dibyom: Closing this issue.

In response to [this](https://github.com/tektoncd/triggers/issues/385#issuecomment-839826272): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.