Skip to content
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

support non-string types in trigger parameter sources #1236

Closed
gavinmead opened this issue May 30, 2021 · 7 comments
Closed

support non-string types in trigger parameter sources #1236

gavinmead opened this issue May 30, 2021 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@gavinmead
Copy link

Is your feature request related to a problem? Please describe.
If a destination field is not a string (ex: k8s Deployment replicas), then k8s trigger operations will fail.

The issue seems to be that https://github.com/argoproj/argo-events/blob/master/sensors/triggers/params.go#L236 getValueByKey(...) returns a string

Example
Use a service account that can create Deployments

apiVersion: argoproj.io/v1alpha1
kind: Sensor
metadata:
  name: webhook
spec:
  template:
    serviceAccountName: k8s-sa
  dependencies:
    - name: test-dep
      eventSourceName: webhook
      eventName: example
  triggers:
    - template:
        name: webhook-workflow-trigger
        k8s:
          group: apps
          version: v1
          resource: deployments
          operation: create
          source:
            resource:
              apiVersion: apps/v1
              kind: Deployment
              metadata:
                name: test-deployment
                labels:
                  app: whalesay
              spec:
                replicas: 1
                selector:
                  matchLabels:
                    app: whalesay
                template:
                    containers:
                    - name: cowsay
                      image: docker/whalesay:latest
                      command: [cowsay]
                      args: ["THIS_WILL_BE_REPLACED"]
          parameters:
            - src:
                dependencyName: test-dep
                dataKey: body.message
              dest: spec.template.containers.0.args.0
            - src:
                dependencyName: test-dep
                dataKey: body.replicas
              dest: spec.replicas

send the following event

curl -d '{"message":"this is my first webhook", "replicas": 2}' -H "Content-Type: application/json" -X POST http://localhost:12000/example

You'll see the following error in the sensor logs

{"level":"info","ts":1622402963.8126473,"logger":"argo-events.sensor","caller":"standard-k8s/standar-k8s.go:171","msg":"creating the object...","sensorName":"webhook","triggerName":"webhook-workflow-trigger","triggerType":"Kubernetes"}
{"level":"error","ts":1622402963.8473272,"logger":"argo-events.sensor","caller":"sensors/listener.go:271","msg":"failed to execute a trigger","sensorName":"webhook","error":"failed to execute trigger: timed out waiting for the condition: Deployment in version \"v1\" cannot be handled as a Deployment: v1.Deployment.Spec: v1.DeploymentSpec.Replicas: readUint32: unexpected character: �, error found in #10 byte of ...|eplicas\":\"2\",\"select|..., bigger context ...|t\",\"namespace\":\"argo-events\"},\"spec\":{\"replicas\":\"2\",\"selector\":{\"matchLabels\":{\"app\":\"whalesay\"}},\"|...","errorVerbose":"timed out waiting for the condition: Deployment in version \"v1\" cannot be handled as a Deployment: v1.Deployment.Spec: v1.DeploymentSpec.Replicas: readUint32: unexpected character: �, error found in #10 byte of ...|eplicas\":\"2\",\"select|..., bigger context ...|t\",\"namespace\":\"argo-events\"},\"spec\":{\"replicas\":\"2\",\"selector\":{\"matchLabels\":{\"app\":\"whalesay\"}},\"|...\nfailed to execute trigger\ngithub.com/argoproj/argo-events/sensors.(*SensorContext).triggerOne\n\t/home/runner/work/argo-events/argo-events/sensors/listener.go:328\ngithub.com/argoproj/argo-events/sensors.(*SensorContext).triggerActions\n\t/home/runner/work/argo-events/argo-events/sensors/listener.go:269\ngithub.com/argoproj/argo-events/sensors.(*SensorContext).listenEvents.func1.3\n\t/home/runner/work/argo-events/argo-events/sensors/listener.go:181\nruntime.goexit\n\t/opt/hostedtoolcache/go/1.14.15/x64/src/runtime/asm_amd64.s:1373","triggerName":"webhook-workflow-trigger","triggeredBy":["test-dep"],"triggeredByEvents":["31396262316664352d333237632d346232632d396434362d383730396335386662383038"],"stacktrace":"github.com/argoproj/argo-events/sensors.(*SensorContext).triggerActions\n\t/home/runner/work/argo-events/argo-events/sensors/listener.go:271\ngithub.com/argoproj/argo-events/sensors.(*SensorContext).listenEvents.func1.3\n\t/home/runner/work/argo-events/argo-events/sensors/listener.go:181"}

Describe the solution you'd like
Add an optional dataType field (defaulting to string) to the src to help resolve the parameters to a particular type supported by gjson.

            - src:
                dependencyName: test-dep
                dataKey: body.replicas
                dataType: int
              dest: spec.replicas

Message from the maintainers:

If you wish to see this enhancement implemented please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

@gavinmead gavinmead added the enhancement New feature or request label May 30, 2021
@VaibhavPage
Copy link
Contributor

Will add it in v1.5

@VaibhavPage VaibhavPage self-assigned this Jul 17, 2021
@whynowy whynowy added this to the v1.5 milestone Jul 21, 2021
@bdegeeter
Copy link

I have a similar use case where replacing an array and object is needed for a more generic trigger template.

with a message body like this. I'd like to replace and array or object in a k8s template.

'{"parameters": { "foo":"bar" }, "args": ["one", "two", "three"]}'
k8s:
  group: ""
  version: v1
  resource: pods
  operation: create
  source:
    resource:
      apiVersion: v1
      kind: Pod
      metadata:
        generateName: hello-world-
      spec:
        containers:
          - name: hello-container
            args:
              - "hello-world"
            command:
              - cowsay
            image: "docker/whalesay:latest"
  parameters:
    - src:
        dependencyName: test-dep
        dataKey: body.args
      dest: spec.containers.0.args

I'm curious about the complexity of a solution that would support more complex data structures in templates.

@whynowy whynowy removed this from the 2021 Q3 milestone Oct 13, 2021
reinvantveer added a commit to reinvantveer/reinvantveer.github.io that referenced this issue Feb 6, 2022
…not allow templating integer data types, see argoproj/argo-events#1236

Signed-off-by: reinvantveer <[email protected]>
reinvantveer added a commit to reinvantveer/reinvantveer.github.io that referenced this issue Feb 6, 2022
…not allow templating integer data types, see argoproj/argo-events#1236

Signed-off-by: reinvantveer <[email protected]>
@jackivanov
Copy link

Any updates on this?

@AalokAhluwalia
Copy link
Contributor

@whynowy I've been taking a look at this issue this week and started making changes. A couple questions I was hoping you'd have some thoughts on:

  1. Would it be helpful instead of having a field in the sensor like dataType to use a boolean field useRawDataValue? When set to true, the data value will be used without converting to string. When set to false, and by default, the string representation will be used (no change from existing behavior). If we used a dataKey field that took a string, we'd have to write code to do the type checking, casting + input validation. It would something like this:
  - src:
      dependencyName: test-dep
      dataKey: body.replicas
      useRawDataValue : <true> | <false>   # Possible new boolean field
      dataType: number | json | boolean | string (default)  # Other possible new string field
    dest: spec.replicas
  1. Should there be any changes to the type handling for the parameter default value type as part of this issue?
  2. Does this look like a complete list of types that should be supported after the change:
  • string
  • number (floats, ints)
  • boolean
  • json (objects, lists)
  1. It looks like a week ago, the team merged fix: payload serialization in sensor. Fixes #2272 #2273 to fix json serialization. Does that mean the scope of this issue is just numbers and booleans?

@whynowy
Copy link
Member

whynowy commented Nov 18, 2022

@AalokAhluwalia - You are right, only numbers and booleans have problems right now, but I more prefer your first idea, to have a useRawDataValue. If there's such a field with a value true, my understanding is there's no need to specify dataType, alway using raw type should work for numbers and booleans, right?

@AalokAhluwalia
Copy link
Contributor

That's what I thought too. The boolean field does seem simpler and will work. One problem with specifying the type in the yaml is the case when there's a mismatch between event type and yaml type.

@reinvantveer
Copy link

🙌

whynowy pushed a commit that referenced this issue Dec 12, 2022
bilalba pushed a commit to intuit-data-os/argo-events that referenced this issue Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants