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 applying parameters for complete trigger spec #230

Merged
merged 12 commits into from
Mar 18, 2019

Conversation

VaibhavPage
Copy link
Contributor

@VaibhavPage VaibhavPage commented Mar 13, 2019

This feature will allow replacing anything in trigger template on the fly. It will be very helpful in cases where triggger workflow is stored on S3 or Git and user wants to figure out the url, bucket or repo name based on the event payload. Also to make it easier for user, this change would differentiate parameters for trigger template and for workflow template.

@VaibhavPage VaibhavPage added this to the v0.9 milestone Mar 13, 2019
@VaibhavPage VaibhavPage self-assigned this Mar 13, 2019
@dtaniwaki
Copy link
Member

I'm not sure when we use WorkflowParameters over TemplateParameter. Could you provide some examples in the PR?

@VaibhavPage
Copy link
Contributor Author

Sure. I'll update the PR with the examples

@VaibhavPage
Copy link
Contributor Author

VaibhavPage commented Mar 14, 2019

For now consider this example ,



# The event payload for this example =
#
#  {
#   "name": "trigger-wf-1",
#   "namespace": "argo-events",
#   "bucket": "mybucket",
#   "key": "hello.yaml",
#   "image": "docker/busybox",
#  }
#
# The example refers to workflow stored on a S3 bucket. With normal sensor spec, you would hard-code the S3 bucket name, object key and other things in trigger.
# But user would like to have dynamic trigger spec where depending on the data in the payload.
# The example data payload contains information like bucket, key, name, namespace which is used to replace the placeholders in trigger spec.
# The image in payload will be used to replace the image in the workflow.
#
apiVersion: argoproj.io/v1alpha1
kind: Sensor
metadata:
  name: webhook-with-context-sensor
  labels:
    sensors.argoproj.io/sensor-controller-instanceid: argo-events
spec:
  deploySpec:
    spec:
      containers:
        - name: "sensor"
          image: "argoproj/sensor"
          imagePullPolicy: Always
      serviceAccountName: argo-events-sa
  dependencies:
    - name: "webhook-gateway:foo"
  eventProtocol:
    type: "HTTP"
    http:
      port: "9300"
  triggers:
    - template:
        name: THIS_WILL_BE_REPLACED_BY name
        resource:
          namespace: THIS_WILL_BE_REPLACED_BY namespace
          group: argoproj.io
          version: v1alpha1
          kind: Workflow
          source:
            s3:
              bucket:
                name: THIS_WILL_BE_REPLACED_BY bucket
                key: THIS_WILL_BE_REPLACED_BY key
              endpoint: minio-service.argo-events:9000
              insecure: true
              accessKey:
                key: accesskey
                name: artifacts-minio
              secretKey:
                key: secretkey
                name: artifacts-minio
      # Apply parameter at template
      templateParameters:
        - src:
            event: "webhook-gateway-http:foo"
            path: name
          dest: name
        - src:
            event: "webhook-gateway-http:foo"
            path: namespace
          dest: resource.namespace
        - src:
            event: "webhook-gateway-http:foo"
            path: bucket
          dest: resource.source.s3.bucket.name
        - src:
            event: "webhook-gateway-http:foo"
            path: key
          dest: resource.source.s3.bucket.key
      # Apply parameter for workflow
      workflowParameters:
        - src:
            event: "webhook-gateway-http:foo"
            path: image
          dest: spec.templates.0.container.image

@VaibhavPage VaibhavPage marked this pull request as ready for review March 14, 2019 19:55
@dtaniwaki
Copy link
Member

dtaniwaki commented Mar 15, 2019

Thank you for the example. I think I understand which is applied where. So, if is the workflow parameter applied only for workflow? What happenes if a user wants to trigger a pod?

@VaibhavPage
Copy link
Contributor Author

VaibhavPage commented Mar 15, 2019

You can apply workflowParameters on any kubernetes resource. I now realize workflowParameters may not be the apt name 😅 , it should be renamed to actionParameters or something more intuitive. open for suggestions.

@dtaniwaki
Copy link
Member

dtaniwaki commented Mar 15, 2019

Yeah, the name WorkflowParameter may confuse users.
How about ResourceParameter?

@VaibhavPage
Copy link
Contributor Author

@VaibhavPage
Copy link
Contributor Author

Or maybe we can rename ResourceParameter struct to Parameter and have WorkflowParameter renamed to ResourceParameter

@dtaniwaki
Copy link
Member

dtaniwaki commented Mar 15, 2019

Sounds good. Maybe, TriggerParameter instead of Parameter for the struct name because it's a parameter of Trigger?

@VaibhavPage
Copy link
Contributor Author

VaibhavPage commented Mar 15, 2019

I am planning to get rid of ResourceObj and resource key within the trigger, because we are not triggering anything except a k8s resource. Also will replace custiom gvk struct with k8s gvk struct.

Also nuking namespace and labels from trigger template. These fields complicate things unnecessary.

…rid of unnecessary structs in sensor trigger
@VaibhavPage
Copy link
Contributor Author

@magaldima @dtaniwaki updated the pr. please review

dtaniwaki
dtaniwaki previously approved these changes Mar 16, 2019
Copy link
Member

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

Overall, looks great to me! I left one comment about the name convention although it's trivial.

OWNERS Show resolved Hide resolved
pkg/apis/sensor/v1alpha1/types.go Outdated Show resolved Hide resolved
Copy link
Member

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

I left one comment about hack/generate-proto.sh.

hack/generate-proto.sh Outdated Show resolved Hide resolved
Copy link
Member

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

lgtm!

@VaibhavPage VaibhavPage requested a review from spk83 March 18, 2019 15:46
@spk83 spk83 merged commit a913daf into master Mar 18, 2019
@VaibhavPage VaibhavPage deleted the support-complete-trigger-parameterization branch March 20, 2019 16:28
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
* feature(): support applying parameters for complete trigger spec

* chore(): update generated files for sensor

* docs(): adding docs

* chore(): rename workflowParameters

* feature(): rename workflowParameters to resource parameters. getting rid of unnecessary structs in sensor trigger

* refactor(): rename ParameterSource. only generating idl from api structs.

* chore(): remove trailing backslash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants