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

Update bpmn-coverage.md #4155

Merged
merged 5 commits into from
Dec 9, 2024
Merged

Update bpmn-coverage.md #4155

merged 5 commits into from
Dec 9, 2024

Conversation

aleksander-dytko
Copy link
Contributor

@aleksander-dytko aleksander-dytko commented Aug 14, 2024

We support Data Object and Data Store in Modeler + for execution. This needs to be reflected in bpmn-coverage

Description

When should this change go live?

  • This is a bug fix, security concern, or something that needs urgent release support.
  • This is already available but undocumented and should be released within a week.
  • This on a specific schedule and the assignee will coordinate a release with the DevEx team. (apply hold label or convert to draft PR)
  • This is part of a scheduled alpha or minor. (apply alpha or minor label)
  • There is no urgency with this change and can be released at any time.

PR Checklist

  • My changes are for an already released minor and are in /versioned_docs directory.
  • My changes are for the next minor and are in /docs directory (aka /next/).

We support Data Object and Data Store in Modeler + for execution. This needs to be reflected in bpmn-coverage
Copy link
Contributor

github-actions bot commented Aug 14, 2024

👋 🤖 🤔 Hello, @christinaausley! Did you make your changes in all the right places?

These files were changed only in docs/. You might want to duplicate these changes in versioned_docs/version-8.6/.

  • docs/components/modeler/bpmn/bpmn-coverage.md

You may have done this intentionally, but we wanted to point it out in case you didn't. You can read more about the versioning within our docs in our documentation guidelines.

@aleksander-dytko aleksander-dytko requested review from korthout and a team August 14, 2024 11:57
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.

While we allow processes with DataObject and DataStore, we don't really support these BPMN concepts. We simply don't use them in the execution.

I'm not confident in saying that we provide coverage for these concepts. I believe this would require using DataInput and DataOutput for Activities like Service Tasks. Instead, we just don't break when BPMN models contain DataObject and DataStore, but they are not actively used in the execution of the model.

IMO, we should discuss this with other BPMN experts before merging.

@aleksander-dytko
Copy link
Contributor Author

@korthout thanks for sharing your opinion.

@saig0 @nikku could you support us here to define what would be needed so that we can claim we support Data store and data object elements?

@akeller akeller added the component:modeler Issues related with Modeler project label Aug 14, 2024
@nikku
Copy link
Member

nikku commented Aug 14, 2024

@saig0 @nikku could you support us here to define what would be needed so that we can claim we support Data store and data object elements?

TLDR: Data stores, input and output have always been modeling only from Camunda's perspective, in C8 and C7. We don't support any of the "data features" beyond modeling (for documentation purposes) that the BPMN standard defines.


BPMN implements data on two layers:

  • Visual, i.e. data stores and data objects, documentation only
  • For execution (+visual) using IO mappings (data input, and data output)

The later one is a direct competition to our own data mappings (both C7 and C8).

An answer to our users could be to mark them as supported (*) with the disclaimer that this includes "modeling only/for documentation" and that we do not support IO-mappings as defined in the BPMN standard.


From the spec:

image

image

image

Copy link
Contributor

@christinaausley christinaausley left a comment

Choose a reason for hiding this comment

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

Let me know when you're ready for final review @aleksander-dytko 👍

@akeller
Copy link
Member

akeller commented Aug 26, 2024

@aleksander-dytko - nudging you on this docs PR.

@christinaausley
Copy link
Contributor

@aleksander-dytko Ready for final review here? 😄

@akeller
Copy link
Member

akeller commented Sep 23, 2024

@aleksander-dytko what's the status of this PR?

@christinaausley
Copy link
Contributor

Followed up with Aleksander on Slack and I'm going to help refactor this PR to include Nico’s suggestion (disclaimer for modeling only).

@christinaausley
Copy link
Contributor

@aleksander-dytko I've incorporated a disclaimer for DataObject and DataStore -- if this looks good to you, I will backport.

@@ -140,15 +140,19 @@ import CompensationSvg from './assets/bpmn-symbols/compensation.svg'

## Data

:::note
`DataObject` and `DataStore` are supported by Camunda for modeling purposes only. Camunda does not support any additional data features the BPMN standard defines.
Copy link
Member

Choose a reason for hiding this comment

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

We could also frame this as "we don't support BPMN standard IO mappings (but of course allow you to map IO (in our very simple, CAmunda style) [link to our documentation for that]". This may read as "We do not support data features".

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "DataObjectandDataStore`, like other BPMN standard IO mappings, are supported by Camunda for modeling purposes only."? We could leave out "Camunda does not support any additional data features the BPMN standard defines."

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops I think we forgot about this @nikku -- WDYT here? CC @aleksander-dytko

Copy link
Member

Choose a reason for hiding this comment

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

We could leave out "Camunda does not support any additional data features the BPMN standard defines."

Yea, we could leave it out, or link to our own support for I/O. Probably easiest to leave it out.

@christinaausley
Copy link
Contributor

christinaausley commented Nov 25, 2024

@nikku This is ready for final review 👍

@aleksander-dytko Should this be backported?

@christinaausley
Copy link
Contributor

I am going to go ahead and merge this. If needed, can backport in a separate PR. CC @aleksander-dytko as I know he is OOO.

@christinaausley christinaausley dismissed korthout’s stale review December 9, 2024 15:41

resolved in commit

@christinaausley christinaausley merged commit a700c36 into main Dec 9, 2024
6 checks passed
@christinaausley christinaausley deleted the data-bpmn-support branch December 9, 2024 15:42
@aleksander-dytko
Copy link
Contributor Author

@christinaausley thanks for following up!

This feature was introduced with 8.2 - please backport this feature for versions <=8.2. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:modeler Issues related with Modeler project
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants