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

Add zeebe:Properties Support #745

Merged
merged 6 commits into from
Sep 1, 2022
Merged

Add zeebe:Properties Support #745

merged 6 commits into from
Sep 1, 2022

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Aug 26, 2022

Changes

  • add zeebe:Property support for all flow elements
  • add zeebe:Property support for element templates

brave_gXLuVx6yb8


Closes #731

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Aug 26, 2022
@philippfromme philippfromme marked this pull request as ready for review August 30, 2022 08:00
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Aug 30, 2022
@nikku
Copy link
Member

nikku commented Aug 31, 2022

Is this ready for review?

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.

Do we test optional binding, i.e. entry only being created if non-empty?

@philippfromme philippfromme force-pushed the zeebe-properties branch 2 times, most recently from 4925649 to 9c3c496 Compare August 31, 2022 13:58
@philippfromme
Copy link
Contributor Author

Do we test optional binding, i.e. entry only being created if non-empty?

I added the missing tests. CI should be green in a minute.

@nikku
Copy link
Member

nikku commented Aug 31, 2022

Thanks. Also coverage is happy now! :)

@nikku
Copy link
Member

nikku commented Aug 31, 2022

Where exactly did you add the tests 👀

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.

Optional does not work as expected.

In fact I could reproduce using output logging (#747) that an empty optional property npm run start:cloud-templates is still serialized.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Aug 31, 2022
@philippfromme
Copy link
Contributor Author

Optional does not work as expected.

In fact I could reproduce using output logging (#747) that an empty optional property npm run start:cloud-templates is still serialized.

I'll look into that.

@philippfromme
Copy link
Contributor Author

@nikku I'm not sure what you mean. Do you mean that a property with "optional": true shouldn't end up in the XML unless there is value? I mistook that for not required. 😅

image

@nikku
Copy link
Member

nikku commented Sep 1, 2022

@philippfromme Exactly. Empty fields (according to our optional behavior) shall not be serialized. Mirrors optional input and output mappings.

@philippfromme philippfromme force-pushed the zeebe-properties branch 3 times, most recently from d0ab636 to 27faba6 Compare September 1, 2022 12:03
@philippfromme philippfromme requested a review from nikku September 1, 2022 12:49
@philippfromme
Copy link
Contributor Author

@philippfromme Exactly. Empty fields (according to our optional behavior) shall not be serialized. Mirrors optional input and output mappings.

Okay, implemented.

@nikku
Copy link
Member

nikku commented Sep 1, 2022

Verified optional entry change via 0d75f18.

Looks good to me.

@fake-join fake-join bot merged commit cb9be46 into master Sep 1, 2022
@fake-join fake-join bot deleted the zeebe-properties branch September 1, 2022 14:05
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Sep 1, 2022
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.

Support zeebe:property for templating
2 participants