-
Notifications
You must be signed in to change notification settings - Fork 2
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
Do not error for job worker user task #182
Do not error for job worker user task #182
Conversation
CC @misiekhardcore as you implemented the task listener rule (potential learning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments.
index.js
Outdated
@@ -249,7 +248,7 @@ module.exports = { | |||
} | |||
|
|||
return type; | |||
}, null) | |||
}, null) || 'error' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, the all
configuration contains null
value for rules not present in the presets.
@@ -96,7 +96,6 @@ const camundaCloud86Rules = withConfig({ | |||
|
|||
const camundaCloud87Rules = withConfig({ | |||
...omit(camundaCloud86Rules, [ 'no-task-listeners' ]), | |||
'zeebe-user-task': 'error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to remove the rule and all related tests for now. We don't keep rules in the codebase that aren't used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a complete rule removal is a breaking change. I don't think it's necessary for the bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider the rule being there a bug. We remove it, we fix the bug. 😆
rules/camunda-cloud/task-listener.js
Outdated
const taskListeners = findExtensionElement(node, 'zeebe:TaskListeners'); | ||
|
||
if (!taskListeners) { | ||
return; | ||
} | ||
|
||
let errors = hasExtensionElement(node, 'zeebe:UserTask'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still going to report the same error which only states that the extension element is required. It doesn't hint at the fact that this extension element is only required if another extension element is present. We have a similar error type for properties (cf. https://github.com/camunda/bpmnlint-plugin-camunda-compat/blob/main/rules/utils/error-types.js#L17). I'd consider adding a EXTENSION_ELEMENT_DEPENDENT_REQUIRED
error type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use the error you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK so there is no such error yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have just clarified that we will remove the check since you cannot model a job worker user task with task listeners in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done via df3d363
This is to not lint a situation which can be achieved via manual XML modification. Related to #182 (comment)
Works for me. This is supporting future platforms, rules are "a floating target" for these. As long as we don't break API we can phrase things as minor. |
The rule was added too early as the feature is still supported in all versions of Camunda. Related to camunda/camunda-modeler#4718
This is to not lint a situation which can be achieved via manual XML modification. Related to #182 (comment)
8a47161
to
f6313a1
Compare
Good job! 👍🏻 |
Two parts:
Related to camunda/camunda-modeler#4690
Related to camunda/camunda-modeler#4718