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.

Sign up for GitHub

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

Jump to bottom

I can deploy a DMN decision #8531

Merged
10 commits merged into from
Jan 7, 2022
Merged

I can deploy a DMN decision #8531

10 commits merged into from
Jan 7, 2022

Conversation

saig0
Copy link
Member

@saig0 saig0 commented Jan 6, 2022 •
edited

Description

  • added new records for DMN: DecisionRecord and DecisionRequirementsRecord
    • including new value types: DECISION and DECISION_REQUIREMENTS
    • including new intents: DecisionIntent.CREATED and DecisionRequirementsIntent.CREATED
  • split the DRG record into a metadata part that does not contain the DMN binary resource
    • the metadata record will be used for the deployment record eventually instead of the DRG record itself to avoid that the deployment contains the resource twice
    • added a duplicate property for the metadata and to align with the process record
  • refactored the DeploymentTransformer to allow more than BPMN resources
    • extracted the BPMN transformer but without bigger changes
  • adjusted some test cases and added the .bpmn file extension to the resource name on deployment
    • previously, the transformer did not check the file extension but assumed that it is a BPMN resource
    • now, the file extension is required because it is used to choose the transformer
    • under the assumption that a BPMN file has a .bpmn file extension (e.g. by using the Camunda modeler), this behavior change should not affect users

Related issues

closes #8067

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

@saig0
feat(protocol): new records for DMN
e7da1ce 
* add new records for DMN resources: DRG and decision
* split the DRG record into a metadata part that does not contain the DMN binary resource
* the metadata record will be used for the deployment record eventually instead of the DRG record itself to avoid that the deployment contains the resource twice
@saig0
refactor(engine): prepare the deployment of other resource types
3bddd91 
* extract interface for transforming a resource to support transformers for other resource types
* fix test cases by adding bpmn file extension to resource name
@saig0
feat(engine): deploy DMN resources
e8a7a4b 
* accept deployment with DMN resources
* reject deployment if a DMN resource is invalid
* write one DRG record for each DMN resource
* write one decision record for each decision in a DRG
@saig0
refactor(engine): extract dmn transformer
2c9d97f
@saig0
refactor(engine): extract bpmn transformer
29d0c47
@saig0
refactor(engine): extract resource transformer interface
9974c98
@saig0 saig0 marked this pull request as ready for review January 6, 2022 13:22
@saig0 saig0 requested a review from korthout January 6, 2022 13:22
@saig0
Copy link
Member Author

saig0 commented Jan 6, 2022

@korthout I'm sorry that the PR got so big 😅 But the changes are a bit less than it seems because of some refactorings and DMN files for tests.

korthout
korthout approved these changes Jan 7, 2022
View reviewed changes
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.

Great work! A large PR, but of high quality 👏 LGTM

Please have a look at my notes and the CodeQL warning before merging.

protocol/revapi.json Outdated Show resolved Hide resolved
...col/src/main/java/io/camunda/zeebe/protocol/record/value/deployment/DecisionRecordValue.java Show resolved Hide resolved
...main/java/io/camunda/zeebe/engine/processing/deployment/transform/DeploymentTransformer.java Show resolved Hide resolved
.../java/io/camunda/zeebe/protocol/impl/record/value/deployment/DecisionRequirementsRecord.java
Comment on lines +84 to +87
@Override
public boolean isDuplicate() {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

❓ Do we want to always consider these records duplicate? Or did you implement this as a first iteration, and we can implement duplication checks for it later?

Copy link
Member Author

Choose a reason for hiding this comment

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

We will implement the duplication checks later with #8174.

However, the records will always be no duplicates. We write the records only for new versions.

But we will use the property for the new deployment command #8065. If a DMN is already deployed then it is added to the deployment response and marked as duplicate. Same as for processes.

I added the property already to avoid later changes in the records and ES templates.

@saig0
Copy link
Member Author

saig0 commented Jan 7, 2022

bors merge

@ghost
Copy link

ghost commented Jan 7, 2022

Build succeeded:

@ghost ghost merged commit 8b63fe8 into develop Jan 7, 2022
@ghost ghost deleted the 8067-deploy-dmn branch January 7, 2022 14:02
@korthout korthout mentioned this pull request Jan 26, 2024
Merged
14 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korthout korthout korthout approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

I can deploy a DMN decision
2 participants
@saig0 @korthout