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

feat: support engine profile selection for DMN #2877

Merged
merged 1 commit into from
Apr 1, 2022
Merged

feat: support engine profile selection for DMN #2877

merged 1 commit into from
Apr 1, 2022

Conversation

barmac
Copy link
Collaborator

@barmac barmac commented Mar 31, 2022 •
edited
Loading

@barmac barmac requested review from a team, philippfromme, smbea and andreasgeier and removed request for a team March 31, 2022 14:43
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Mar 31, 2022
@barmac barmac force-pushed the 2872-engine-profile-for-DMN branch from d0684cc to 5c993b6 Compare March 31, 2022 15:01
@barmac barmac mentioned this pull request Mar 31, 2022
Merged
@andreasgeier
Copy link

andreasgeier commented Mar 31, 2022 •
edited
Loading

Result after clicking on the create C7/C8 DMN button from welcome tab:

Screen Shot 2022-03-31 at 18 16 54

Log message

[diagram_1.dmn] No provider for "e"! (Resolving: executionPlatform -> updateExecutionPlatformCommand -> e)
    at a (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:112:11)
    at error (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:98:14)
    at get (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:152:18)
    at file:///Applications/Camunda%20Modeler%20DMN%20Version%20Select.app/Contents/Resources/app.asar/public/3.3.js:1:38603
    at Array.map (<anonymous>)
    at map (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:170:30)
    at fnDef (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:185:14)
    at providers (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:146:43)
    at file:///Applications/Camunda%20Modeler%20DMN%20Version%20Select.app/Contents/Resources/app.asar/public/3.3.js:1:56365
    at Array.forEach (<anonymous>) [ error ]
This error may be the result of a plug-in compatibility issue. [ info ]
[Disable plug-ins (restarts the app)](https://github.com/camunda/camunda-modeler/pull/2877#) [ info ]
[diagram_1.dmn] No provider for "e"! (Resolving: executionPlatform -> updateExecutionPlatformCommand -> e)
    at a (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:112:11)
    at error (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:98:14)
    at get (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:152:18)
    at file:///Applications/Camunda%20Modeler%20DMN%20Version%20Select.app/Contents/Resources/app.asar/public/3.3.js:1:38603
    at Array.map (<anonymous>)
    at map (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:170:30)
    at fnDef (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:185:14)
    at providers (webpack:///node_modules/diagram-js/node_modules/didi/dist/index.esm.js:146:43)
    at file:///Applications/Camunda%20Modeler%20DMN%20Version%20Select.app/Contents/Resources/app.asar/public/3.3.js:1:56365
    at Array.forEach (<anonymous>) [ error ]
This error may be the result of a plug-in compatibility issue. [ info ]
[Disable plug-ins (restarts the app)](https://github.com/camunda/camunda-modeler/pull/2877#) [ info ]

@andreasgeier
Copy link

It's not a plugin issue. Disabling plugins does not help.

andreasgeier
andreasgeier suggested changes Mar 31, 2022
View reviewed changes
Copy link

@andreasgeier andreasgeier left a comment

Choose a reason for hiding this comment

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

Back to you @barmac :)

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

barmac commented Mar 31, 2022

I can reproduce it. It must be something related to minification.

@barmac barmac mentioned this pull request Apr 1, 2022
Closed
@barmac barmac force-pushed the 2872-engine-profile-for-DMN branch 2 times, most recently from fe21962 to bf6235e Compare April 1, 2022 09:03
@barmac
Copy link
Collaborator Author

barmac commented Apr 1, 2022

@andreasgeier Please download the artifact now. It should work correctly (works on my machine).

@barmac barmac requested a review from andreasgeier April 1, 2022 09:15
@barmac barmac added needs review Review pending and removed in progress Currently worked on labels Apr 1, 2022
MaxTru
MaxTru requested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

Works for me.

Have some small questions and findings

client/src/app/tabs/dmn/__tests__/DmnEditorSpec.js Outdated
// then
expect(wrapper.find('EngineProfile').exists()).to.be.true;

console.log(instance.getCached().engineProfile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove me :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

client/src/app/tabs/cloud-dmn/__tests__/DmnEditorSpec.js Outdated
// then
expect(wrapper.find('EngineProfile').exists()).to.be.true;

console.log(instance.getCached().engineProfile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

client/src/app/tabs/cloud-dmn/DmnEditor.js Outdated
const modeler = this.getModeler();
const activeViewer = modeler.getActiveViewer();

if (!activeViewer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this happen? If it can happen, why not test it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It occurred that we can safely remove this.

client/src/app/tabs/cloud-dmn/DmnEditor.js Outdated
this.engineProfile.setCached(engineProfile);
} catch (err) {

// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes from https://github.com/camunda/camunda-modeler/blob/develop/client/src/app/tabs/bpmn/BpmnEditor.js#L453. Perhaps we can just mark it specifically that we don't need to react on error here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact, this doesn't happen as we don't allow unknown execution platform at this point.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Apr 1, 2022
@barmac barmac force-pushed the 2872-engine-profile-for-DMN branch 2 times, most recently from cb65fb2 to e4787c4 Compare April 1, 2022 10:06
@barmac barmac marked this pull request as draft April 1, 2022 10:36
@barmac
Copy link
Collaborator Author

barmac commented Apr 1, 2022

This requires some additional work.

@barmac barmac force-pushed the 2872-engine-profile-for-DMN branch from e4787c4 to 72a12cc Compare April 1, 2022 10:45
@barmac barmac marked this pull request as ready for review April 1, 2022 10:49
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 1, 2022
@barmac
Copy link
Collaborator Author

barmac commented Apr 1, 2022

Ready for review again. The artifacts will be built in a couple of minutes.

andreasgeier
andreasgeier approved these changes Apr 1, 2022
View reviewed changes
Copy link

@andreasgeier andreasgeier left a comment

Choose a reason for hiding this comment

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

✅ LGTM

@barmac barmac requested a review from MaxTru April 1, 2022 11:44
MaxTru
MaxTru approved these changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

Nice!

@barmac barmac merged commit b4c71dc into develop Apr 1, 2022
@barmac barmac deleted the 2872-engine-profile-for-DMN branch April 1, 2022 11:58
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasgeier andreasgeier andreasgeier approved these changes

@MaxTru MaxTru MaxTru approved these changes

@philippfromme philippfromme Awaiting requested review from philippfromme philippfromme was automatically assigned from camunda/modeling-dev

@smbea smbea Awaiting requested review from smbea smbea was automatically assigned from camunda/modeling-dev

Assignees

@barmac barmac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Engine profile for Cloud DMN
3 participants
@barmac @andreasgeier @MaxTru