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

feat: handle zeebe task retries #880

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ajeans
Copy link

@ajeans ajeans commented Feb 15, 2023

WIP / RFC

This PR is related to camunda/camunda-modeler#2936 and hopes to allow to see and edit retries in zeebe/cloud/v8 element templates.
This should be doable now that schema support landed with camunda/element-templates-json-schema#90

This is very definitely not working and probably very misguided

  • mostly duplicating code related to taskDefinition:type to service taskDefinition:retries
  • type and retries are two keys attached to the taskDefinition, yet I cannot figure out how to merge those two attributes when processing a change on one of them... so I just duplicated the independent taskCreation logic
  • I tried to see how this behaved by running npm run start:cloud-templates but I couldn't figure out how to inject the latest / master element-templates-json-schema... So no way to test this.

CC @nikku I'm afraid I won't be able to go much further without help.

@barmac barmac self-requested a review February 20, 2023 14:08
@barmac
Copy link
Member

barmac commented Feb 20, 2023

Hi @ajeans, I will look into this and try to guide you.

@barmac
Copy link
Member

barmac commented Feb 20, 2023

OK so I had an initial look and indeed we duplicate a lot, but there are no tests to verify that the solution actually works. My suggestion would be to start with the tests which will then guide the solution:

  1. https://github.com/bpmn-io/bpmn-js-properties-panel/blob/master/test/spec/provider/cloud-element-templates/create/TemplateElementFactory.spec.js#L196
  • add a test which verifies zeebe:taskDefinition:retries is properly added
  • add a test which verifies the task definition has correct properties when both bindings are used in a template
  1. https://github.com/bpmn-io/bpmn-js-properties-panel/blob/master/test/spec/provider/cloud-element-templates/properties/CustomProperties.spec.js#L148
  • add tests for zeebe:taskDefinition:retries
  • add integration test which verifies nothing breaks if both bindings are given

This should give us a good base which will drive the implementation.

If you need any support, feel free to ask questions.

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

See comment above.

@smbea
Copy link
Contributor

smbea commented Mar 6, 2023

@ajeans do you plan to work on this? Otherwise let's just close this PR

@ajeans
Copy link
Author

ajeans commented Mar 6, 2023

@smbea Unfortunately, I didn't even manage to run the existing tests (step 1 of @barmac directions).

npm test
> [email protected] test
> karma start karma.config.js

06 03 2023 17:25:59.350:INFO [preprocessor.env]: Publishing variables:  []
Webpack bundling...
[
  {
    moduleIdentifier: '/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js',
    moduleName: './node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js',
    loc: '3:36-47',
    message: "export 'BaseContext' (imported as 'BaseContext') was not found in 'lezer-feel' (possible exports: normalizeContextKey, parser, trackVariables)",
    moduleId: './node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js',
    moduleTrace: [ [Object], [Object], [Object], [Object], [Object], [Object] ],
    details: undefined,
    stack: '    at HarmonyImportSpecifierDependency.getLinkingErrors (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/dependencies/HarmonyImportDependency.js:160:8)\n' +
      '    at HarmonyImportSpecifierDependency._getErrors (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/dependencies/HarmonyImportSpecifierDependency.js:202:15)\n' +
      '    at HarmonyImportSpecifierDependency.getWarnings (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/dependencies/HarmonyImportSpecifierDependency.js:178:16)\n' +
      '    at Compilation.reportDependencyErrorsAndWarnings (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compilation.js:3132:24)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compilation.js:2729:28\n' +
      '    at eval (eval at create (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:29:1)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/FlagDependencyExportsPlugin.js:385:11\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2830:7\n' +
      '    at Object.each (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2850:39)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/FlagDependencyExportsPlugin.js:361:18\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2830:7\n' +
      '    at Object.each (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2850:39)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/FlagDependencyExportsPlugin.js:51:16\n' +
      '    at Hook.eval [as callAsync] (eval at create (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)\n' +
      '    at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/tapable/lib/Hook.js:18:14)\n' +
      '    at Compilation.finish (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compilation.js:2714:28)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compiler.js:1182:19\n' +
      '    at processTicksAndRejections (node:internal/process/task_queues:78:11)\n' +
      '\n' +
      "ModuleDependencyWarning: export 'BaseContext' (imported as 'BaseContext') was not found in 'lezer-feel' (possible exports: normalizeContextKey, parser, trackVariables)\n" +
      '    at Compilation.reportDependencyErrorsAndWarnings (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compilation.js:3137:23)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compilation.js:2729:28\n' +
      '    at eval (eval at create (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:29:1)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/FlagDependencyExportsPlugin.js:385:11\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2830:7\n' +
      '    at Object.each (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2850:39)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/FlagDependencyExportsPlugin.js:361:18\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2830:7\n' +
      '    at Object.each (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/neo-async/async.js:2850:39)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/FlagDependencyExportsPlugin.js:51:16\n' +
      '    at Hook.eval [as callAsync] (eval at create (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)\n' +
      '    at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/tapable/lib/Hook.js:18:14)\n' +
      '    at Compilation.finish (/home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compilation.js:2714:28)\n' +
      '    at /home/ajeansen/workspace/oss/bpmn-js-properties-panel/node_modules/webpack/lib/Compiler.js:1182:19\n' +
      '    at processTicksAndRejections (node:internal/process/task_queues:78:11)'
  }
]
asset commons.js 26.2 MiB [emitted] (name: commons) (id hint: commons)
asset runtime.js 8.18 KiB [emitted] (name: runtime)
asset testBundle.4238075174.js 1.01 KiB [emitted] (name: testBundle.4238075174)
Entrypoint testBundle.4238075174 26.2 MiB = runtime.js 8.18 KiB commons.js 26.2 MiB testBundle.4238075174.js 1.01 KiB

WARNING in ./node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js 3:36-47
export 'BaseContext' (imported as 'BaseContext') was not found in 'lezer-feel' (possible exports: normalizeContextKey, parser, trackVariables)
 @ ./node_modules/@bpmn-io/variable-resolver/lib/util/feelUtility.js 6:0-51 102:36-50 103:27-41 140:51-65 220:28-42
 @ ./node_modules/@bpmn-io/variable-resolver/lib/ZeebeVariableResolver.js 3:0-53 33:29-44
 @ ./node_modules/@bpmn-io/variable-resolver/lib/index.js 2:0-64 8:30-51
 @ ./test/spec/provider/HOCs/withVariableContext.spec.js 6:0-73 143:28-55
 @ ./test/ sync .spec\.js$ ./spec/provider/HOCs/withVariableContext.spec.js test/spec/provider/HOCs/withVariableContext.spec.js
 @ ./test/testBundle.js 1:17-57

webpack 5.75.0 compiled with 1 warning in 9489 ms
06 03 2023 17:26:10.273:INFO [karma-server]: Karma v6.4.1 server started at http://localhost:9877/
06 03 2023 17:26:10.274:INFO [launcher]: Launching browsers ChromeHeadless with concurrency unlimited
06 03 2023 17:26:10.277:INFO [launcher]: Starting browser ChromeHeadless
06 03 2023 17:26:10.845:INFO [Chrome Headless 111.0.5555.0 (Linux x86_64)]: Connected on socket Ea-WZWo7AvfC2YU0AAAB with id 55917867
Chrome Headless 111.0.5555.0 (Linux x86_64) ERROR
  Uncaught TypeError: Class extends value undefined is not a constructor or null
  at webpack-internal:///./node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js:8:70
  
  TypeError: Class extends value undefined is not a constructor or null
      at eval (webpack-internal:///./node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js:8:70)
      at ./node_modules/@bpmn-io/variable-resolver/lib/util/VariableContext.js (/tmp/_karma_webpack_35545/commons.js:260:1)
      at __webpack_require__ (/tmp/_karma_webpack_35545/runtime.js:31:42)
      at eval (webpack-internal:///./node_modules/@bpmn-io/variable-resolver/lib/util/feelUtility.js:7:74)
      at ./node_modules/@bpmn-io/variable-resolver/lib/util/feelUtility.js (/tmp/_karma_webpack_35545/commons.js:271:1)
      at __webpack_require__ (/tmp/_karma_webpack_35545/runtime.js:31:42)
      at eval (webpack-internal:///./node_modules/@bpmn-io/variable-resolver/lib/ZeebeVariableResolver.js:7:75)
      at ./node_modules/@bpmn-io/variable-resolver/lib/ZeebeVariableResolver.js (/tmp/_karma_webpack_35545/commons.js:205:1)
      at __webpack_require__ (/tmp/_karma_webpack_35545/runtime.js:31:42)
      at eval (webpack-internal:///./node_modules/@bpmn-io/variable-resolver/lib/index.js:7:80)

And trying to run the test from my IDE (Intellij) fails in the same way.

Is there a DEVELOP documentation with some help for this project? Otherwise, I'm sorry but I'm just stuck...

@nikku
Copy link
Member

nikku commented Mar 7, 2023

@ajeans Did you npm install before you executed the tests?

@smbea Let me try if I can unstuck this PR until the end of the week.

@ajeans
Copy link
Author

ajeans commented Mar 9, 2023

@barmac that's complicated :)

node -v && npm -v

returns

v16.18.1
8.19.2

but npm ci fails on my Ubuntu 22.04 with

...
npm WARN deprecated [email protected]: core-js-pure@<3.23.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Some versions have web compatibility issues. Please, upgrade your dependencies to the actual version of core-js-pure.
npm ERR! code 1
npm ERR! git dep preparation failed
npm ERR! command /snap/node/6895/bin/node /usr/local/lib/node_modules/npm/bin/npm-cli.js install --force --cache=/home/ajeansen/.npm --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! > [email protected] prepare
npm ERR! > run-s build
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm ERR! code 1
npm ERR! npm ERR! path /home/ajeansen/.npm/_cacache/tmp/git-cloneWio5kv
npm ERR! npm ERR! command failed
npm ERR! npm ERR! command sh -c -- run-s build
npm ERR! 
npm ERR! npm ERR! A complete log of this run can be found in:
npm ERR! npm ERR!     /home/ajeansen/.npm/_logs/2023-03-09T16_09_29_440Z-debug-0.log

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/ajeansen/.npm/_logs/2023-03-09T16_09_19_937Z-debug-0.log

So I used yarn which is at version 1.22.19 on my machine...

yarn install

and it seems to work fine.

yarn install v1.22.19
info No lockfile found.
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
[1/4] Resolving packages...
warning @bpmn-io/[email protected]: WARNING: This project has been renamed to @bpmn-io/element-template-icon-renderer. Install using @bpmn-io/element-template-icon-renderer instead.
warning react-svg-loader > react-svg-core > [email protected]: This SVGO version is no longer supported. Upgrade to v2.x.x.
warning react-svg-loader > react-svg-core > svgo > [email protected]: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning " > [email protected]" has unmet peer dependency "preact@>=10".
warning "@bpmn-io/properties-panel > @bpmn-io/feel-editor > @codemirror/[email protected]" has unmet peer dependency "@lezer/common@^1.0.0".
warning " > @testing-library/[email protected]" has unmet peer dependency "preact@>=10 || ^10.0.0-alpha.0 || ^10.0.0-beta.0".
warning " > @testing-library/[email protected]" has unmet peer dependency "preact@^10.4.8".
[4/4] Building fresh packages...
success Saved lockfile.
$ run-s bundle
yarn run v1.22.19
$ rollup -c

src/index.js → dist/bpmn-js-properties-panel.umd.js...
created dist/bpmn-js-properties-panel.umd.js in 13.8s

src/index.js → dist/index.js, dist/index.esm.js...
created dist/index.js, dist/index.esm.js in 4.8s
Done in 19.50s.
Done in 151.85s.

After that, both yarn test and npm test trigger errors in @bpmn-io/variable-resolver

@barmac
Copy link
Member

barmac commented Mar 15, 2023

Hi @ajeans,

Sorry see that the installation fails on your machine. In the team, we discussed a solution which could however make it unnecessary to separately implement support for templating each of the properties of task definition. Namely, we could have a single binding type zeebe:taskDefinition which would set the properties via name, e.g.

{
  "type": "zeebe:taskDefinition",
  "name": "retries"
}

Thus, if Zeebe ever supports property foo, you'd just use it as the name in your binding without additional code:

{
  "type": "zeebe:taskDefinition",
  "name": "foo"
}

I believe this is a viable solution which would pay off long-term.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Queued in backlog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants