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(engine): support inside properties for multi-instance completion #9334

Merged

Conversation

lzgabel
Copy link
Contributor

@lzgabel lzgabel commented May 8, 2022

Description

The BPMN 2.0 specification defined the following properties of a multi-instance body instance:

  • numberOfInstances
  • numberOfActiveInstances
  • numberOfCompletedInstances
  • numberOfTerminatedInstances

This PR makes numberOfCompletedInstances and numberOfTerminatedInstances available for use in the completion condition expression. Although they are available in that expression, they do not exist as process variables. These properties take precedence over process variables with the same name.

Out of scope: using these properties in other expressions

Related issues

closes #2893

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

@lzgabel
Copy link
Contributor Author

lzgabel commented May 8, 2022

Hi @korthout. Please take a moment to review this code. 🙇

Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Thanks @lzgabel 🚀 I really like the changes.

❌ The numberOfTerminatedInstances will not really be useful to the completion condition, because when an instance has terminated we will terminate the multi-instance as well, so we'll never get to evaluate the completion condition again. Nevertheless, it's good to add the logic for it because we can use it for other things. To make it useful, let's keep track of the value instead of calculating it, similar to numberOfCompletedInstances.

PS: please ignore the failed automated checks. There appears to be something wrong with the CI, and we're looking into it.

EDIT: We've found a solution for the problems with the CI. Please rebase the PR on the current main branch.

@lzgabel lzgabel force-pushed the 2893-lz-multi-instance-properties branch from 30991b2 to b272d1f Compare May 10, 2022 03:13
@lzgabel
Copy link
Contributor Author

lzgabel commented May 10, 2022

Hi @korthout , I've fixed all review comments, please check this out. 🙇

Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

That looks great @lzgabel 👍

❌ I see now that I missed 1 thing on the last review. We should only increment the numberOfCompletedInstances on completed elements, not on completing elements. The same goes for the numberOfTerminatedInstances (should be done on terminated elements, not on terminating).

@lzgabel
Copy link
Contributor Author

lzgabel commented May 10, 2022

Hi @korthout. I've fixed all review comments, please check this out. 🙇

Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Well done @lzgabel 👏 I really like it

bors merge

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit c62bb73 into camunda:main May 10, 2022
@lzgabel lzgabel deleted the 2893-lz-multi-instance-properties branch May 10, 2022 15:42
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.

I can access the numberOf* multi-instance properties
2 participants