camunda-bpm-platform icon indicating copy to clipboard operation
camunda-bpm-platform copied to clipboard

Consolidate upstream/downstream libraries - follow ups

Open venetrius opened this issue 9 months ago • 6 comments

related to: https://github.com/camunda/camunda-bpm-platform/issues/3682

Spin:

PR follow up: spin/dataformat-xml-dom-jakarta/pom.xml

  <dependency>
      <groupId>jakarta.xml.bind</groupId>
      <artifactId>jakarta.xml.bind-api</artifactId>
      <version>${version.jakarta.xml.bind-api}</version>

❓Instead of a separate versioning, could we maybe also use the jakarta.jakartaee-bom here? That's already defined and used at several other places so it could be easier for future updates.

update <smc> tag from pom.xml-s (also for commons)

  • also remove master branch-es from https://github.com/camunda/camunda-bpm-helper-resources/blob/master/notice-file-update/file-list

Commons

readme

commons/README.md:

Java JRE 1.8+ is required.

is this still accurate?

why project.version needed for commons modules except typed-values in EE?

https://github.com/camunda/camunda-bpm-platform-ee/pull/914#discussion_r1594122503 answer see next section:

Typed values are a separate module.

Typed values have been moved to ./commons but not packaged with the commons-bom yet.

Commons is not using the platform defined version variable for Logback

Can we use the ${version.logback} property in commons/pom.xml? Right now, the version is hardcoded cf. https://github.com/search?q=repo%3Acamunda%2Fcamunda-bpm-platform%201.2.11&type=code

Connect

some libraries are used by both but did not update them as different mayor version:

connect:
<wiremock.version>1.58</wiremock.version>
<mockito.version>1.9.5</mockito.version>

platfrom:
<version.wiremock>2.27.2</version.wiremock>
<version.wiremock-jre8>2.27.2</version.wiremock-jre8>
<version.mockito>5.10.0</version.mockito>

camunda-template-engines-jsr223

uses higher dependency then platform:

      <artifactId>assertj-core</artifactId>
      <version>3.20.2</version>

platform :

    <version.assertj>2.9.1</version.assertj>

Breakdown

### Pull Requests
- [ ] https://github.com/camunda/camunda-bpm-platform/pull/4377
- [ ] https://github.com/camunda/camunda-bpm-helper-resources/pull/9
- [ ] https://github.com/camunda/camunda-bpm-platform-ee/pull/936
- [ ] https://github.com/camunda/camunda-bpm-platform/pull/4394

venetrius avatar May 08 '24 15:05 venetrius

Steps:

  • [x] (created task) Spin - could we use the existing jakarta.jakartaee-bom versioning instead of jakarta.xml.bind-api
  • [x] Spin - update scm tag
  • [x] Spin - remove master branch from helpers repo
  • [x] Commons - update scm tag
  • [x] Commons - remove master branch from helpers repo
  • [x] Commons - Readme - Check & Update required Java version
  • [x] Commons - Package typed-values with commons-bom
  • [x] Commons - Update Docs: Packaged typed-values with commons-bom
  • [x] Commons - Use the platform defined version variable for Logback
  • [x] (created task) Connect uses wiremock 1.58 while platform uses 2.27
  • [x] (created task) Connect uses mockito 1.9.5 while platform uses 5.10.0
  • [x] Freemarket-template-engine uses assertj-core 3.20.2 while platform uses 2.9.1

venetrius avatar May 28 '24 07:05 venetrius

Commons - [Readme] - Check & Update required Java version (Java 11?)

Required version is correct, it was specified in .ci.cambpm but file has been removed during reconciliation.

Based on docs: https://docs.camunda.org/manual/7.20/introduction/supported-environments/ supported minimum is Java 11, updated the Readme file.

venetrius avatar May 28 '24 07:05 venetrius

Spin - could we use the existing jakarta.jakartaee-bom versioning instead of jakarta.xml.bind-api

PR follow up: spin/dataformat-xml-dom-jakarta/pom.xml

  <dependency>
      <groupId>jakarta.xml.bind</groupId>
      <artifactId>jakarta.xml.bind-api</artifactId>
      <version>${version.jakarta.xml.bind-api}</version>

❓Instead of a separate versioning, could we maybe also use the jakarta.jakartaee-bom here? That's already defined and used at several other places so it could be easier for future updates.

Platform declares <version.xml.bind-api>2.3.3</version.xml.bind-api> but camunda-spin-dataformat-xml-dom-jakarta uses 4.0.2.

Created a follow up task to have a single version of the lib: https://github.com/camunda/camunda-bpm-platform/issues/4383

venetrius avatar May 28 '24 10:05 venetrius

+1 for keeping the declared versions in sync (e.g. include them in the camunda-bom) -1 for using jakartaee-bom - as a spring boot user, this might conflict with versions declared in spring-boot-bom

jangalinski avatar May 28 '24 12:05 jangalinski

Freemarket-template-engine uses assertj-core 3.20.2 while platform uses 2.9.1

Bumping platform assertj-core up to 3.20.2 (or to latest 3.26.0) fails with:

/engine-dmn/engine/src/test/java/org/camunda/bpm/dmn/engine/test/asserts/DmnDecisionTableResultAssert.java:[23,8]
org.camunda.bpm.dmn.engine.test.asserts.DmnDecisionTableResultAssert is not abstract and 
does not override abstract method 
newAbstractIterableAssert(java.lang.Iterable<?
 extends org.camunda.bpm.dmn.engine.DmnDecisionRuleResult>) 
in org.assertj.core.api.AbstractIterableAssert

Downgrading Freemarket-template-engine to 2.9.1 (that is used by platform) build work pushed changes to branch

venetrius avatar May 28 '24 13:05 venetrius

Connect uses wiremock 1.58 while platform uses 2.27 & uses mockito 1.9.5 while platform uses 5.10.0

Neither updating wiremock or mockito passing the CI, created new issues: https://github.com/camunda/camunda-bpm-platform/issues/4384 https://github.com/camunda/camunda-bpm-platform/issues/4385

venetrius avatar May 28 '24 21:05 venetrius

Hi @venetrius, I reviewed the linked PRs. All the changes you made make sense to me. 👍

❌However, I find it hard to understand the exact goals of this issue. I can not understand what needs to be done from the issue description and why, making it impossible to assess if all tasks are completed. It would help to give the issue description more structure. For example, for each task, you can link sources (review feedback, test failures, etc.), describe the problem, and document your decision.

❓ I looked at this list. Does this list include all the TODOs for this issue? The PRs seem to tick all the boxes. ❓ However, there is one open question in the list: Package typed-values with commons-bom—migration guide? Do we need to change the migration guide?

❌You created follow-up issues (#4384 and #4385). When you create new issues, you should ideally pick one of the templates (bug report, task, etc.) and provide the information requested by the template. This would also have helped create a good description for this issue.

mboskamp avatar May 31 '24 12:05 mboskamp

Hi @mboskamp, Thanks for your review. I update the issue description to make it easier to understand. I think my learning here is that I shouldn't put too many goals in a single issue. In a future I will break issues down smaller tasks even is the code changes are relatively small.

Resolved questions about the follow up tasks

Created PR to update migration guide regarding managing typed-values version via camunda-commons-bom

venetrius avatar Jun 03 '24 09:06 venetrius

All subtask are done, closing this issue. @gbetances089 no functional changes were introduced in this issue.

venetrius avatar Jun 07 '24 07:06 venetrius