feat: support jdk 17&21 workflow #367
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow configuration for a Maven-based Java project. The change expands the Java version matrix used for testing, adding support for Java 17 and Java 21 alongside the existing Java 8 and 11 versions. Additionally, it updates the JDK setup action and specifies the distribution as 'temurin'. A new profile is introduced in the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 actionlint (1.7.4).github/workflows/maven.yml38-38: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) 40-40: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue (action) ⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/maven.yml (1)
35-35: Consider using Java version-specific build profiles.Different Java versions might require different compiler settings or dependencies. Consider using Maven profiles to handle version-specific configurations.
Would you like me to help create version-specific Maven profiles for Java 17 & 21 compatibility?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/maven.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (21)
- GitHub Check: build (17)
- GitHub Check: build (11)
- GitHub Check: build (8)
🔇 Additional comments (1)
.github/workflows/maven.yml (1)
35-35: Verify Maven wrapper compatibility with Java 17 & 21.The workflow uses Maven wrapper (
./mvnw) which needs to be compatible with the new Java versions.Run this script to check the Maven wrapper version:
| @@ -32,7 +32,7 @@ jobs: | |||
|
|
|||
| strategy: | |||
| matrix: | |||
| java-version: [8, 11] | |||
| java-version: [8, 11, 17, 21] | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions/setup-java to v3 for better Java 17 & 21 support.
The workflow uses an outdated version of actions/setup-java@v1 which may not fully support Java 17 & 21. Additionally, for newer Java versions, it's important to specify the distribution type.
Apply this diff to update the action and specify the distribution:
java-version: [8, 11, 17, 21]
steps:
- uses: actions/checkout@v2
- name: Set up JDK ${{ matrix.java-version }}
- uses: actions/setup-java@v1
+ uses: actions/setup-java@v3
with:
java-version: ${{ matrix.java-version }}
+ distribution: 'temurin' # or 'zulu', depending on your preferenceCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/maven.yml (1)
Line range hint
35-52: Consider workflow optimizations for better reliability and performance.The workflow could benefit from the following improvements:
- Add matrix exclusions for unsupported Java/dependency combinations
- Enable Maven dependency caching
- Add continue-on-error for experimental Java versions
Example implementation:
strategy: matrix: java-version: [8, 11, 17, 21] fail-fast: false # Mark Java 21 as experimental include: - java-version: 21 experimental: true continue-on-error: ${{ matrix.experimental || false }} steps: # ... existing checkout and setup steps ... - name: Cache Maven packages uses: actions/cache@v3 with: path: ~/.m2 key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }} restore-keys: ${{ runner.os }}-m2🧰 Tools
🪛 actionlint (1.7.4)
44-44: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
44-44: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
44-44: shellcheck reported issue in this script: SC1010:warning:1:387: Use semicolon or linefeed before 'fi' (or quote to make it literal)
(shellcheck)
44-44: shellcheck reported issue in this script: SC1047:error:2:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
44-44: shellcheck reported issue in this script: SC1072:error:2:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
49-49: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
49-49: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
49-49: shellcheck reported issue in this script: SC1010:warning:1:387: Use semicolon or linefeed before 'fi' (or quote to make it literal)
(shellcheck)
49-49: shellcheck reported issue in this script: SC1047:error:2:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
49-49: shellcheck reported issue in this script: SC1072:error:2:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
54-54: the runner of "codecov/codecov-action@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/maven.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/maven.yml
44-44: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
44-44: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
44-44: shellcheck reported issue in this script: SC1010:warning:1:387: Use semicolon or linefeed before 'fi' (or quote to make it literal)
(shellcheck)
44-44: shellcheck reported issue in this script: SC1047:error:2:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
44-44: shellcheck reported issue in this script: SC1072:error:2:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
49-49: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
49-49: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
49-49: shellcheck reported issue in this script: SC1010:warning:1:387: Use semicolon or linefeed before 'fi' (or quote to make it literal)
(shellcheck)
49-49: shellcheck reported issue in this script: SC1047:error:2:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
49-49: shellcheck reported issue in this script: SC1072:error:2:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
🔇 Additional comments (1)
.github/workflows/maven.yml (1)
35-35: Update actions/setup-java to v3 for better Java 17 & 21 support.The workflow uses an outdated version of
actions/setup-java@v1which may not fully support Java 17 & 21. Additionally, for newer Java versions, it's important to specify the distribution type.
.github/workflows/maven.yml
Outdated
| run: if [[ ${{ matrix.java-version }} -gt 17 ]]; then | ||
| MAVEN_OPTS="--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.regex=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED" | ||
| fi | ||
| ./mvnw clean install -Dmaven.test.skip=true -B -U -e && bash ./.middleware-common/check_format.sh |
There was a problem hiding this comment.
Fix shell script syntax and improve readability.
The current shell script has syntax issues and could be more maintainable.
Apply this diff to fix the syntax and improve readability:
- run: if [[ ${{ matrix.java-version }} -gt 17 ]]; then
- MAVEN_OPTS="--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.regex=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED"
- fi
- ./mvnw clean install -Dmaven.test.skip=true -B -U -e && bash ./.middleware-common/check_format.sh
+ env:
+ MAVEN_OPTS: ${{ matrix.java-version > 17 && '--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.regex=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED' || '' }}
+ run: |
+ ./mvnw clean install -Dmaven.test.skip=true -B -U -e && bash ./.middleware-common/check_format.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: if [[ ${{ matrix.java-version }} -gt 17 ]]; then | |
| MAVEN_OPTS="--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.regex=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED" | |
| fi | |
| ./mvnw clean install -Dmaven.test.skip=true -B -U -e && bash ./.middleware-common/check_format.sh | |
| env: | |
| MAVEN_OPTS: ${{ matrix.java-version > 17 && '--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.regex=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED' || '' }} | |
| run: | | |
| ./mvnw clean install -Dmaven.test.skip=true -B -U -e && bash ./.middleware-common/check_format.sh |
🧰 Tools
🪛 actionlint (1.7.4)
44-44: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
44-44: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
44-44: shellcheck reported issue in this script: SC1010:warning:1:387: Use semicolon or linefeed before 'fi' (or quote to make it literal)
(shellcheck)
44-44: shellcheck reported issue in this script: SC1047:error:2:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
44-44: shellcheck reported issue in this script: SC1072:error:2:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pom.xml (1)
Line range hint
1-471: Fix pipeline failure: Format pom.xml.The pipeline indicates uncommitted changes in pom.xml. This is likely due to the
sortpom-maven-pluginwhich enforces consistent formatting.Run the following command to fix the formatting:
mvn sortpom:sort🧰 Tools
🪛 GitHub Actions: build
[error] Uncommitted changes detected in pom.xml. Changes must be committed before running the build.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maven.yml(1 hunks)pom.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/maven.yml
🧰 Additional context used
🪛 GitHub Actions: build
pom.xml
[error] Uncommitted changes detected in pom.xml. Changes must be committed before running the build.
🔇 Additional comments (2)
pom.xml (2)
456-464: Consider adding module access for testing frameworks.While the current module access configuration covers Java base modules, consider adding access for testing frameworks if needed (e.g., JUnit, Mockito).
🧰 Tools
🪛 GitHub Actions: build
[error] Uncommitted changes detected in pom.xml. Changes must be committed before running the build.
450-471: Profile configuration for JDK 17+ looks good.The profile correctly configures the necessary module access for testing with JDK 17 and above. The
--add-opensdirectives grant appropriate access to internal APIs that are encapsulated by the module system.However, let's verify if all required dependencies support JDK 17 and 21.
✅ Verification successful
Dependencies are compatible with JDK 17+ configuration
The key dependencies (Netty 4.1.42.Final and Hessian 3.5.5) support JDK 17, and the profile's module system configuration is correctly set up with all necessary
--add-opensdirectives. The ByteBuddy experimental mode is properly enabled for modern JDK compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if key dependencies support JDK 17 and 21 # Check Maven Central for Netty compatibility echo "Checking Netty version compatibility..." curl -s "https://search.maven.org/solrsearch/select?q=g:io.netty+AND+a:netty-all+AND+v:${netty.version}&rows=20&wt=json" | jq . # Check Maven Central for Hessian compatibility echo "Checking Hessian version compatibility..." curl -s "https://search.maven.org/solrsearch/select?q=g:com.alipay.sofa+AND+a:hessian+AND+v:${hessian.version}&rows=20&wt=json" | jq . # Check for any explicit JDK version requirements in dependencies echo "Checking for explicit JDK requirements in dependencies..." rg -l "jdk|java.version" --type xmlLength of output: 774
Script:
#!/bin/bash # Extract dependency versions and JDK requirements echo "Extracting dependency information..." echo "Properties section:" rg -A 5 "<properties>" pom.xml echo "\nDependency versions:" rg "<version>" pom.xml | grep -E "netty|hessian|byte-buddy" echo "\nJDK/Java version requirements:" rg "jdk|java.version" pom.xmlLength of output: 1605
Script:
#!/bin/bash # Get complete dependency information echo "Netty version property:" rg "netty.version" pom.xml echo "\nFull Netty dependency:" rg -A 3 "<artifactId>netty" pom.xml echo "\nBytebuddy dependencies:" rg -A 3 "byte-buddy" pom.xmlLength of output: 508
🧰 Tools
🪛 GitHub Actions: build
[error] Uncommitted changes detected in pom.xml. Changes must be committed before running the build.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/maven.yml(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/maven.yml
38-38: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
40-40: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (2)
.github/workflows/maven.yml (2)
35-35: LGTM! Matrix updated to support newer Java versions.The Java version matrix has been correctly expanded to include versions 17 and 21, aligning with the PR objective.
40-43: LGTM! JDK setup action properly updated.The action has been correctly updated to v3 with Temurin distribution, which provides better support for Java 17 & 21.
🧰 Tools
🪛 actionlint (1.7.4)
40-40: the runner of "actions/setup-java@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| @@ -32,14 +32,15 @@ jobs: | |||
|
|
|||
| strategy: | |||
| matrix: | |||
| java-version: [8, 11] | |||
| java-version: [8, 11, 17, 21] | |||
|
|
|||
| steps: | |||
| - uses: actions/checkout@v2 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions/checkout to v3 or v4.
The checkout action version is outdated. Update it to maintain consistency with other actions and ensure better compatibility.
- - uses: actions/checkout@v2
+ - uses: actions/checkout@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v2 | |
| - uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.4)
38-38: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| <profile> | ||
| <id>args-for-test-by-jdk17-and-above</id> | ||
| <activation> | ||
| <jdk>[17,)</jdk> | ||
| </activation> | ||
| <properties> | ||
| <maven.surefire.argLine>--add-opens java.base/java.lang=ALL-UNNAMED | ||
| --add-opens java.base/java.net=ALL-UNNAMED | ||
| --add-opens java.base/java.math=ALL-UNNAMED | ||
| --add-opens java.base/java.text=ALL-UNNAMED | ||
| --add-opens java.base/java.util=ALL-UNNAMED | ||
| --add-opens java.base/java.util.regex=ALL-UNNAMED | ||
| --add-opens java.base/java.util.concurrent=ALL-UNNAMED | ||
| --add-opens java.sql/java.sql=ALL-UNNAMED | ||
| --add-opens java.sql.rowset/javax.sql.rowset.serial=ALL-UNNAMED</maven.surefire.argLine> | ||
| </properties> | ||
| </profile> |
There was a problem hiding this comment.
💡 Codebase verification
Remove unnecessary SQL-related module opens
The SQL-related module opens can be safely removed as there is no SQL usage in the codebase:
- No SQL-related imports or dependencies
- No SQL usage in test files
- No JDBC or database-related code
🔗 Analysis chain
Consider removing unnecessary SQL-related module opens.
The profile is well-structured and correctly configured for JDK 17+. However, it includes SQL-related module opens that might not be necessary for this project:
--add-opens java.sql/java.sql=ALL-UNNAMED--add-opens java.sql.rowset/javax.sql.rowset.serial=ALL-UNNAMED
<maven.surefire.argLine>--add-opens java.base/java.lang=ALL-UNNAMED
--add-opens java.base/java.net=ALL-UNNAMED
--add-opens java.base/java.math=ALL-UNNAMED
--add-opens java.base/java.text=ALL-UNNAMED
--add-opens java.base/java.util=ALL-UNNAMED
--add-opens java.base/java.util.regex=ALL-UNNAMED
- --add-opens java.base/java.util.concurrent=ALL-UNNAMED
- --add-opens java.sql/java.sql=ALL-UNNAMED
- --add-opens java.sql.rowset/javax.sql.rowset.serial=ALL-UNNAMED</maven.surefire.argLine>
+ --add-opens java.base/java.util.concurrent=ALL-UNNAMED</maven.surefire.argLine>Let's verify if SQL-related modules are actually needed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for SQL-related code or dependencies
echo "Checking for SQL usage..."
rg -l "java\.sql" --type java
echo "Checking for SQL-related dependencies..."
rg "sql" pom.xml
Length of output: 371
Script:
#!/bin/bash
# Check for SQL-related code in test files
echo "Checking for SQL-related test files..."
fd -e java -t f . src/test --exec rg -l "(?i)(jdbc|datasource|sql|rowset)"
echo "Checking for SQL-related test dependencies..."
rg -A 5 -B 5 "(?i)(jdbc|h2|mysql|postgresql|oracle|sqlite)" pom.xml
echo "Checking for specific SQL imports..."
rg "^import.*\b(javax\.sql|java\.sql)" --type java
Length of output: 470
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pom.xml (1)
452-468: 🛠️ Refactor suggestionRemove unnecessary SQL-related module opens.
The profile is well-structured for JDK 17+, but includes SQL-related module opens that aren't needed as there's no SQL usage in the codebase.
Apply this diff to remove unnecessary module opens:
<maven.surefire.argLine>--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.net=ALL-UNNAMED --add-opens java.base/java.math=ALL-UNNAMED --add-opens java.base/java.text=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.regex=ALL-UNNAMED - --add-opens java.base/java.util.concurrent=ALL-UNNAMED - --add-opens java.sql/java.sql=ALL-UNNAMED - --add-opens java.sql.rowset/javax.sql.rowset.serial=ALL-UNNAMED</maven.surefire.argLine> + --add-opens java.base/java.util.concurrent=ALL-UNNAMED</maven.surefire.argLine>
🧹 Nitpick comments (1)
src/main/java/com/alipay/remoting/util/RemotingUtil.java (1)
196-213: LGTM! The refactored parsing logic is clearer and more maintainable.The new implementation improves readability by:
- Using explicit position checks instead of loops
- Handling edge cases more clearly
- Adding descriptive comments
Consider adding input validation for the port format:
if (port.contains(":")) { + // Ensure port is in valid format + String[] parts = port.split(":"); + if (parts.length != 2) { + return addr; + } port = port.split(":")[1]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pom.xml(3 hunks)src/main/java/com/alipay/remoting/util/RemotingUtil.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (21)
- GitHub Check: build (17)
- GitHub Check: build (11)
- GitHub Check: build (8)
🔇 Additional comments (2)
pom.xml (2)
83-84: LGTM! Well-structured test configuration properties.The new properties provide flexibility for configuring test execution arguments and exclusions.
279-280: LGTM! Proper integration of test configuration properties.The properties are correctly referenced in the surefire plugin configuration.
Summary by CodeRabbit