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

Disallow Connectors (Element Templates) Prior to Camunda 8 #31

Merged
merged 2 commits into from
Jun 9, 2022
Merged

Disallow Connectors (Element Templates) Prior to Camunda 8 #31

merged 2 commits into from
Jun 9, 2022

Conversation

philippfromme
Copy link
Collaborator

@philippfromme philippfromme commented May 27, 2022 •
edited

Disallow any zeebe:modelerTemplate attributes.

Closes #30

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label May 27, 2022
@philippfromme philippfromme mentioned this pull request May 27, 2022
Closed
2 tasks
@philippfromme philippfromme force-pushed the 30-connectors branch 10 times, most recently from 0d53289 to 78666f6 Compare June 3, 2022 06:25
@philippfromme philippfromme marked this pull request as ready for review June 7, 2022 07:28
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jun 7, 2022
@nikku
Copy link
Member

nikku commented Jun 7, 2022 •
edited

Given my previous comment (#30 (comment)) could you elaborate why you chose to move forward like this?

I know that that = foobar input bindings are not supported in < Camunda 8.

I believe a business requirement is that we want connectors (also the ones a user potentially creates / all element templated tasks) to appear not supported in the web modeler (but also the desktop modeler).

@philippfromme
Copy link
Collaborator Author

I believe a business requirement is that we want connectors (also the ones a user potentially creates / all element templated tasks) to appear not supported in the web modeler (but also the desktop modeler).

So disallowing element templates altogether would be the way to go?

@nikku
Copy link
Member

nikku commented Jun 8, 2022

Yes. Because they won't work (connector or not) for basic cases.

@philippfromme
Copy link
Collaborator Author

Yes. Because they won't work (connector or not) for basic cases.

Will adjust.

@philippfromme philippfromme changed the title Disallow Connectors Prior to Camunda 8 WIP Disallow Connectors Prior to Camunda 8 Jun 8, 2022
@philippfromme philippfromme changed the title WIP Disallow Connectors Prior to Camunda 8 Disallow Connectors (Element Templates) Prior to Camunda 8 Jun 8, 2022
@philippfromme
Copy link
Collaborator Author

Element templates are now disallowed alltogether. Ready for review.

nikku
nikku reviewed Jun 8, 2022
View reviewed changes
index.js Outdated
@@ -10,6 +10,7 @@ module.exports = {
'has-error-reference': 'error',
'has-loop-characteristics': 'error',
'has-message-reference': 'error',
'has-no-template': 'error',
Copy link
Member

Choose a reason for hiding this comment

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

The last concern I have is this name. Can we find a better one that clearly states templates are not supported? As it looks like we settled on has* already, so maybe this name is just a thing that users of this plug-in will understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any suggestions for a better naming scheme? We can still change it. Right now no one actually gets to see those names.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe templates-not-supported?

Copy link
Member

Choose a reason for hiding this comment

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

Or no-template-support?

Copy link
Member

Choose a reason for hiding this comment

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

Or no-templates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And the opposite would then be error-reference instead of has-error-reference?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. I'd use eslint / bpmnlint as an inspiration that setup this pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Shortened it would likely be no-template to match bpmnlint exactly:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, will adjust.

@philippfromme
Copy link
Collaborator Author

@nikku I renamed the rules (cf. b98ac91).

Since you've already approved, can I go ahead and merge?

@nikku
Copy link
Member

nikku commented Jun 9, 2022

Much better like this. Awesome that you followed up 💪

@nikku nikku merged commit 98f776d into main Jun 9, 2022
@nikku nikku deleted the 30-connectors branch June 9, 2022 11:33
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jun 9, 2022
@philippfromme philippfromme mentioned this pull request Aug 1, 2022
Closed
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku approved these changes

@pinussilvestrus pinussilvestrus Awaiting requested review from pinussilvestrus

@barmac barmac Awaiting requested review from barmac

@MaxTru MaxTru Awaiting requested review from MaxTru

Assignees

@philippfromme philippfromme

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Rule to Allow Connectors for Camunda 8 Only
2 participants
@philippfromme @nikku