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 zeebe:calledElement templating #37

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Conversation

barmac
Copy link
Member

@barmac barmac commented Dec 7, 2023

This PR implements zeebe:calledElement binding. In the implementation, the template developer is required to provide inputs and outputs explicitly as variable propagation is disabled for the templated elements.

Related to camunda/camunda-modeler#3006

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Dec 7, 2023
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Looks great!

One concern I have is whether we actually need to set propagateAllChildVariables (and the other one) or not.

I believe (to be double-checked) the engine ships with sensible defaults there. We could rely on them and simplify our solution.

@barmac
Copy link
Member Author

barmac commented Dec 12, 2023

According to the docs, per default the engine propagates all of the variables from the child process to the call activity. So if we don't set the property to false and the element does not have outputs, the engine copies all of the called process variables to the parent scope. In the implementation, I want to enforce explicit mapping to avoid unexpected leak of variables to the parent scope.

Perhaps a better solution would be to propagate variables only when requested by the user, but that would have to be fixed within the engine. WDYT?

@nikku
Copy link
Member

nikku commented Dec 12, 2023

WDYT?

I completely agree with your rationale. We want to be safety first and prevent accidental leaks. Thanks for confirming.

Note that there is a catch though:

This behavior can be customized by defining output mappings at the call activity. The output mappings are applied on completing the call activity and only those variables that are defined in the output mappings are propagated.

So if we operate under the assumption that any sane re-usable building block / template uses output mappings, then we're fine without explicitly setting the property.

@barmac
Copy link
Member Author

barmac commented Dec 12, 2023

So if we operate under the assumption that any sane re-usable building block / template uses output mappings, then we're fine without explicitly setting the property.

I'm not sure if we can assume this. I can also imagine that a call activity does not emit outputs, and I don't want to write its internal variables to the parent scope:

image

@nikku
Copy link
Member

nikku commented Dec 12, 2023

You got me with "not emitting variables". This is a case, especially considering our "re-usable building block" theme we want to prevent surprises at all costs.


I shared this thought with the Zeebe folks, too.

@barmac barmac marked this pull request as ready for review December 13, 2023 16:51
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 13, 2023
@barmac
Copy link
Member Author

barmac commented Dec 13, 2023

I updated the PR with the already released schema/validator. @nikku I think we can merge and release it. WDYT?

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Looks great. Let's get this released.

@fake-join fake-join bot merged commit dd9001c into main Dec 13, 2023
8 checks passed
@fake-join fake-join bot deleted the call-activity branch December 13, 2023 17:17
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 13, 2023
@barmac barmac mentioned this pull request Dec 20, 2023
4 tasks
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.

2 participants