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

feat: Support dataTemplate and contextTemplate for Trigger Parameters #543

Merged
merged 5 commits into from
Mar 21, 2020
Merged

feat: Support dataTemplate and contextTemplate for Trigger Parameters #543

merged 5 commits into from
Mar 21, 2020

Conversation

tinyzimmer
Copy link
Contributor

@tinyzimmer tinyzimmer commented Mar 15, 2020

This PR relates to the issue I opened: #541

One thing I am definitely open to maybe doing is maybe a number on ResolveParamValue. With the two additions it feels like it's getting a bit unruly.

I can add some coverage as well. So consider this just a WIP to get feedback on the approach.

As a side note, I am having trouble running make codegen

./hack/update-api-docs.sh
I0315 18:03:19.811707    6828 main.go:127] parsing go packages in directory github.com/argoproj/argo-events/pkg/apis/eventsources/v1alpha1
F0315 18:03:21.224675    6828 main.go:133] no API packages found in github.com/argoproj/argo-events/pkg/apis/eventsources/v1alpha1

But that doesn't seem right...

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@VaibhavPage VaibhavPage left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. This feature is awesome!! Just a couple of suggestions,

  1. You may need to run make codegen and commit updated files for deep copy and api docs.
  2. Can you also add an example sensor YAML file under examples/sensors folder that demonstrates the templating functionality?

@tinyzimmer
Copy link
Contributor Author

  1. You may need to run make codegen and commit updated files for deep copy and api docs.

I feel like an idiot, but I'm still struggling with this. On my mac it gives the error I showed above once it's done allegedly running deep copy funcs (that dont produce any changes).

Tried on Linux, and it ran all the way through, but again didn't actually generate any deep-copy code, and for the html/md files it just converted every kind to it's full path...e.g.

-<code>context</code></br> <em> Argo Events common.EventContext </em>
+<code>event</code></br> <em>
+github.com/argoproj/argo-events/pkg/apis/common.Event </em>

This is beginning to make me doubt my sanity.

  1. Can you also add an example sensor YAML file under examples/sensors folder that demonstrates the templating functionality?

I will add an example if not tonight, then sometime in the morning.

VaibhavPage
VaibhavPage previously approved these changes Mar 19, 2020
Copy link
Contributor

@VaibhavPage VaibhavPage left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I'll fix the make codegen part.

@tinyzimmer
Copy link
Contributor Author

Changes look good to me. I'll fix the make codegen part.

I'd be very curious what the issue was in case I want to contribute more in the future 😄

@VaibhavPage
Copy link
Contributor

As you have introduced string fields within an existing struct, the deep-copy script won't update anything. But, make codegen should at least update API docs under api folder and openapi_generated.go file. make codegen really just runs 1) https://github.com/argoproj/argo-events/blob/master/hack/update-codegen.sh 2) https://github.com/argoproj/argo-events/blob/master/hack/update-openapigen.sh and 3) https://github.com/argoproj/argo-events/blob/master/hack/update-api-docs.sh under the hood. So, you can run these files individually as well.

@VaibhavPage
Copy link
Contributor

Also, can you resolve the conflicts? Thanks.

@VaibhavPage VaibhavPage merged commit cad2834 into argoproj:master Mar 21, 2020
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
…argoproj#543)

* add dataTemplate and contextTemplate options to trigger parameters

* fix comment in api doc, add some test cases, return error from template if evaluates to empty string or <no value>

* add example

Co-authored-by: Vaibhav <[email protected]>
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