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

chore(catalog-generator): Lower the required Java version to 17 #1520

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

lordrip
Copy link
Member

@lordrip lordrip commented Oct 2, 2024

Context

Currently, the catalog-generator requires Java 21.

While that enables modern syntax, not all users have it installed in their machines, so in order to lower the contribution requirements, we're lowering the required version to be Java 17.

By default, in RHEL 9.0, Java 17 is preinstalled.

Here is an extract of the available Java version per RHEL version

RHEL Version Java version Release date
RHEL 7 Java 8 June 10, 2014
RHEL 8 Java 11 May 7, 2019
RHEL 9 👈 we're here Java 17 May 17, 2022

fix: #1476

@lordrip lordrip requested review from apupier and igarashitm October 2, 2024 13:35
Copy link
Member

@apupier apupier left a comment

Choose a reason for hiding this comment

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

usually a good practice to align the version used for the build on CI with the minimal one, for instance here

java-version: "21"

<configuration>
<rules>
<requireJavaVersion>
<version>[17.0.0,17.0.999],[21.0.0,21.0.999]</version>
Copy link
Member

Choose a reason for hiding this comment

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

This will prevent users with Java 22+ to build. or even 21.1
I do no tthink that it is what we want

Copy link
Member

Choose a reason for hiding this comment

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

By reading version range rule defintion https://maven.apache.org/enforcer/enforcer-rules/versionRanges.html, it seems like we are forbiding Java 17.1, 18, 19, 20 too

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 we should go with only 17

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, didn't like it

[ERROR] Rule 0: org.apache.maven.enforcer.rules.version.RequireJavaVersion failed with message:
[ERROR] Detected JDK version 17.0.12 (JAVA_HOME=/home/rmartinez/.sdkman/candidates/java/17.0.12-tem) is not in the allowed range [17,17].

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be [1.0,)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, 17 didn't work, I tried that one. I ended up using
image

Copy link
Member

Choose a reason for hiding this comment

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

strange, just 17 is working locally for me:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

😕

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the problem was [17] getting extrapolated to [17,17]. I missed the 17 alone.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was it, using 17 alone works with both 17 and 21

packages/catalog-generator/pom.xml Show resolved Hide resolved
@apupier
Copy link
Member

apupier commented Oct 2, 2024

Shouldn't it be updated in the other Maven modules projects?

<version.java>21</version.java>

to avoid having a mix of minimal version of JDK

@apupier
Copy link
Member

apupier commented Oct 2, 2024

readme to update too

- Java 21+

@lordrip
Copy link
Member Author

lordrip commented Oct 2, 2024

Shouldn't it be updated in the other Maven modules projects?

<version.java>21</version.java>

to avoid having a mix of minimal version of JDK

We don't use camel-catalog package anymore. We still have it because we're in the process of migrating the tests.

@lordrip lordrip force-pushed the chore/lower2java17 branch from b225bc7 to 5b82976 Compare October 2, 2024 13:59
<configuration>
<rules>
<requireJavaVersion>
<version>[17]</version>
Copy link
Member

Choose a reason for hiding this comment

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

not [17] but 17

Copy link
Member

Choose a reason for hiding this comment

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

or [17,) if you prefer to be more explicit and do no tuse the short notation
image

@lordrip lordrip force-pushed the chore/lower2java17 branch from 5b82976 to 8bd3058 Compare October 2, 2024 14:13
Currently, the `catalog-generator` requires Java 21.

While that enable modern syntax, not all users have it installed in
their machines, so in order to lower the contribution requirements,
we're lowering the required version to be Java 17.

By default, in RHEL 9.0, Java 17 is preinstalled.

Here is an extract of the available Java version per RHEL version

| RHEL Version | Java version | Release date |
| --- | --- | --- |
| RHEL 7 | Java 8  | June 10, 2014 |
| RHEL 8 | Java 11 | May 7, 2019   |
| RHEL 9 | Java 17 | May 17, 2022  |

fix: KaotoIO#1476
@lordrip lordrip force-pushed the chore/lower2java17 branch from 8bd3058 to 015a433 Compare October 2, 2024 14:14
Copy link

sonarcloud bot commented Oct 2, 2024

@@ -59,7 +59,7 @@ void testGeneratorCalledWithCorrectParameters() {
})) {
generateCommand.run();

CatalogGeneratorBuilder builder = mockedBuilder.constructed().getFirst();
CatalogGeneratorBuilder builder = mockedBuilder.constructed().get(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

😢

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@3907213). Learn more about missing BASE report.

Files with missing lines Patch % Lines
.../camelcatalog/generator/CamelCatalogProcessor.java 0.00% 15 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1520   +/-   ##
=======================================
  Coverage        ?   69.90%           
  Complexity      ?       62           
=======================================
  Files           ?      276           
  Lines           ?     7733           
  Branches        ?     1503           
=======================================
  Hits            ?     5406           
  Misses          ?     2277           
  Partials        ?       50           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lordrip lordrip merged commit 5f7e7f3 into KaotoIO:main Oct 2, 2024
12 checks passed
@lordrip lordrip deleted the chore/lower2java17 branch October 2, 2024 14:36
@apupier
Copy link
Member

apupier commented Oct 2, 2024

usually a good practice to align the version used for the build on CI with the minimal one, for instance here

java-version: "21"

I guess this part was forgotten.
It also ensures that when using Java 17 is really working.

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

Successfully merging this pull request may close these issues.

Downgrade the required Java Version to build Kaoto
3 participants