Skip to content

feat(events): improve parameter mapping for triggers #1397

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

Open
JamesDoingStuff wants to merge 1 commit into
base: main
Choose a base branch
from
+31 −17
Open

feat(events): improve parameter mapping for triggers #1397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/events/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ name: events
description: Data Analysis event triggering
type: application

version: 0.2.7
version: 0.2.8

dependencies:
- name: argo-events
Expand Down
7 changes: 6 additions & 1 deletion charts/events/crds/trigger-crds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,13 @@ spec:
properties:
name:
type: string
messagePath:
jsonPath:
type: string
default:
type: string
anyOf:
- required: [name, jsonPath]
- required: [name, default]
required:
- workflow
- eventName
Expand Down
24 changes: 18 additions & 6 deletions charts/events/hooks/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,27 @@ def sync(self, parent: dict, related: dict) -> SyncResponse:
templateArgs = []

for userParam in userParameters:
param_name: str = userParam.get("name", "")

@davehadley davehadley Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if user sets "name" but not "jsonPath" what happens?

@davehadley davehadley Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

or vice versa?

@JamesDoingStuff JamesDoingStuff Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's enforced in the CRD that name is always required, plus at least one of jsonPath and default

path: str = userParam.get("jsonPath", "")
default: str = userParam.get("default", "")

if not param_name:
continue

src = {"dependencyName": name}

if path:
src.update({"dataKey": "body." + path})

if default:
src.update({"value": default})

sensorParams.append({
"src": {
"dependencyName": name,
"dataKey": userParam.get("messagePath", "")
},
"dest": f"spec.arguments.parameters.{userParameters.index(userParam)}.value"
"src": src,
"dest": f"spec.arguments.parameters.#(name==\"{param_name}\").value"
})

templateArgs.append({"name": userParam.get("name")})
templateArgs.append({"name": param_name, "value": ""})

triggers.append({
"template": {
Expand Down
15 changes: 6 additions & 9 deletions charts/events/templates/example-trigger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,13 @@ spec:
workflow:
template: example-template
parameters:
- name: jpg
messagePath: body.parameters.jpg
- name: jpeg
messagePath: body.parameters.jpeg
- name: tif
messagePath: body.parameters.tif
jsonPath: parameters.tif

@davehadley davehadley Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I realise that is was me that suggested changing the name of this. But I now realise this might be a little confusing.

Even though the docs says JSONPath: https://argoproj.github.io/argo-events/APIs/
it actually follows GJSON syntax https://github.com/tidwall/gjson/blob/master/SYNTAX.md

Apparently GJSON does not exactly implement the current JSONPath standard https://datatracker.ietf.org/doc/html/rfc9535.

So it might be misleading to call this field JSONPath.

Maybe your original was choice was better.

@JamesDoingStuff JamesDoingStuff Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah right.. Do you think path would be ok, or is that too vague? Or maybe keyPath? I now feel like messagePath makes it sound like the path to the message rather than a path within the message. And if it's coming from a webhook then I wouldn't really think of it as a message

@davehadley davehadley Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should decide. I don't have a strong opinion. I think as long as we document it well it doesn't really matter. It's alpha so we can change it if we see it confuses users.

- name: tiff
messagePath: body.parameters.tiff
jsonPath: parameters.tiff
- name: png
messagePath: body.parameters.png
jsonPath: parameters.png
default: "true"
---
apiVersion: workflows.diamond.ac.uk/v1alpha1
kind: Trigger
Expand All @@ -37,7 +34,7 @@ spec:
template: access-visit-dir
parameters:
- name: visitdir
messagePath: body.doc.data_session_directory
jsonPath: doc.data_session_directory

---
apiVersion: workflows.diamond.ac.uk/v1alpha1
Expand All @@ -54,5 +51,5 @@ spec:
template: echo-workflow-template
parameters:
- name: message
messagePath: body.name
jsonPath: name
{{- end }}
Loading