-
Notifications
You must be signed in to change notification settings - Fork 130
Upgrade Java version to 17 in Dockerfile #1195
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
Conversation
Motivation: The latest Central Dogma docker image failes to start up on the `ubuntu-latest` image of GitHub Actions due to the following NPE: ``` Error: -20 09:22:41.518 [ERROR](c.l.c.s.Main) [main] Failed to start the Central Dogma: java.util.concurrent.ExecutionException: java.lang.NullPointerException at java.base/java.util.concurrent.CompletableFuture.reportGet(Unknown Source) at java.base/java.util.concurrent.CompletableFuture.get(Unknown Source) at com.linecorp.armeria.common.util.EventLoopCheckingFuture.get(EventLoopCheckingFuture.java:73) at com.linecorp.centraldogma.server.Main.start(Main.java:138) at com.linecorp.centraldogma.server.Main.main(Main.java:238) Caused by: java.lang.NullPointerException: null at java.base/jdk.internal.platform.cgroupv2.CgroupV2Subsystem.getInstance(Unknown Source) at java.base/jdk.internal.platform.CgroupSubsystemFactory.create(Unknown Source) at java.base/jdk.internal.platform.CgroupMetrics.getInstance(Unknown Source) at java.base/jdk.internal.platform.SystemMetrics.instance(Unknown Source) at java.base/jdk.internal.platform.Metrics.systemMetrics(Unknown Source) at java.base/jdk.internal.platform.Container.metrics(Unknown Source) at jdk.management/com.sun.management.internal.OperatingSystemImpl.<init>(Unknown Source) at jdk.management/com.sun.management.internal.PlatformMBeanProviderImpl.getOperatingSystemMXBean(Unknown Source) at jdk.management/com.sun.management.internal.PlatformMBeanProviderImpl$3.nameToMBeanMap(Unknown Source) at java.management/sun.management.spi.PlatformMBeanProvider$PlatformComponent.getMBeans(Unknown Source) at java.management/java.lang.management.ManagementFactory.getPlatformMXBean(Unknown Source) at java.management/java.lang.management.ManagementFactory.getOperatingSystemMXBean(Unknown Source) at io.micrometer.core.instrument.binder.system.FileDescriptorMetrics.<init>(FileDescriptorMetrics.java:78) at io.micrometer.core.instrument.binder.system.FileDescriptorMetrics.<init>(FileDescriptorMetrics.java:74) at com.linecorp.centraldogma.server.CentralDogma.configureMetrics(CentralDogma.java:1130) at com.linecorp.centraldogma.server.CentralDogma.startServer(CentralDogma.java:772) at com.linecorp.centraldogma.server.CentralDogma.doStart(CentralDogma.java:474) at com.linecorp.centraldogma.server.CentralDogma.access$800(CentralDogma.java:198) at com.linecorp.centraldogma.server.CentralDogma$CentralDogmaStartStop.lambda$doStart$0(CentralDogma.java:1306) at com.linecorp.centraldogma.server.CentralDogma$CentralDogmaStartStop.lambda$execute$2(CentralDogma.java:1345) at java.base/java.lang.Thread.run(Unknown Source) ``` The exception seems fixed by https://bugs.openjdk.org/browse/JDK-8287073 Related: line/centraldogma-python#70 Modifications: - Upgrade the JRE version of the Docker image to 17 - Add a CI test to check if Central Dogma Docker image starts successfully Result: Central Dogma Docker image now starts up propertly in GitHub Actions.
WalkthroughAdds a new GitHub Actions job Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Repo as Repository
participant SetupJava as actions/setup-java
participant GradleAction as gradle/gradle-build-action
participant QEMU as docker/setup-qemu-action
participant Buildx as docker/setup-buildx-action
participant Gradle as Gradle Wrapper
participant Docker as Docker Engine
participant Compose as docker-compose
GH->>Repo: trigger docker-build (if repository == line/centraldogma)
GH->>SetupJava: set up Temurin (BUILD_JDK_VERSION)
GH->>GradleAction: configure Gradle
GH->>QEMU: register QEMU
GH->>Buildx: set up Buildx builder
GH->>Gradle: ./gradlew :dist:docker -Pversion=test
Gradle->>Docker: build image(s)
GH->>Docker: docker images
GH->>Compose: docker-compose -f dist/docker-compose-test.yml up --detach --wait
Compose->>Compose: containers start & healthchecks
GH->>Compose: docker-compose -f ... down
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
minwoox
left a comment
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.
👍 👍 👍
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/actions_build.yml (1)
224-228: Ensure docker-compose cleanup on test failure.Consider adding error handling to ensure containers are cleaned up even if the test fails. Wrap the commands in a way that guarantees cleanup (e.g., using
finallyor a shell trap).Apply this diff to add robustness:
- name: Test DockerCompose # Make sure Central Dogma docker container can be started and healthy. run: | - docker compose -f dist/docker-compose-test.yml up --pull never --wait -d - docker compose -f dist/docker-compose-test.yml down + docker compose -f dist/docker-compose-test.yml up --pull never --wait -d + docker compose -f dist/docker-compose-test.yml down || docker compose -f dist/docker-compose-test.yml down -v shell: bashAlternatively, use a trap to ensure cleanup:
trap 'docker compose -f dist/docker-compose-test.yml down' EXIT docker compose -f dist/docker-compose-test.yml up --pull never --wait -d
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dist/build.gradleis excluded by!**/dist/**
📒 Files selected for processing (1)
.github/workflows/actions_build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/actions_build.yml
201-201: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
205-205: 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
- GitHub Check: flaky-tests
- GitHub Check: docker-build
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: lint
🔇 Additional comments (1)
.github/workflows/actions_build.yml (1)
194-229: Clarify scope: Java version in Docker image vs. build environment.The PR objectives mention upgrading the Dockerfile to Java 17, but this change adds a CI job that uses JDK 21 (from
BUILD_JDK_VERSION). The job appears to build the Docker image, but the actual Dockerfile is not provided in this review.Verify that: (1) the Dockerfile has been updated to Java 17, (2) Java 21 is intentionally used only for the build environment, and (3) the built image contains the correct Java 17 version.
| env: | ||
| GRADLE_OPTS: -Xmx1280m | ||
| steps: | ||
| - uses: actions/checkout@v3 |
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.
Update actions/checkout to v4.
The action version is outdated and should be upgraded to ensure compatibility with current GitHub Actions runners and security patches.
Apply this diff:
- - uses: actions/checkout@v3
+ - 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@v3 | |
| - uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.8)
201-201: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/actions_build.yml around line 201, the workflow uses
actions/checkout@v3 which is outdated; update the checkout step to use
actions/checkout@v4 by replacing the version tag so the action references v4
instead of v3, save the file, and run a CI workflow to verify the change.
| - id: setup-jdk | ||
| name: Set up JDK ${{ env.BUILD_JDK_VERSION }} | ||
| uses: actions/setup-java@v3 | ||
| with: | ||
| distribution: 'temurin' | ||
| java-version: ${{ env.BUILD_JDK_VERSION }} | ||
|
|
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.
Update actions/setup-java to v4 for consistency and security.
The setup-java action in this job uses v3, while the existing build job already uses v4 (line 58). Upgrade for consistency and to receive security updates.
Apply this diff:
- id: setup-jdk
name: Set up JDK ${{ env.BUILD_JDK_VERSION }}
- uses: actions/setup-java@v3
+ uses: actions/setup-java@v4
with:
distribution: 'temurin'
java-version: ${{ env.BUILD_JDK_VERSION }}📝 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.
| - id: setup-jdk | |
| name: Set up JDK ${{ env.BUILD_JDK_VERSION }} | |
| uses: actions/setup-java@v3 | |
| with: | |
| distribution: 'temurin' | |
| java-version: ${{ env.BUILD_JDK_VERSION }} | |
| - id: setup-jdk | |
| name: Set up JDK ${{ env.BUILD_JDK_VERSION }} | |
| uses: actions/setup-java@v4 | |
| with: | |
| distribution: 'temurin' | |
| java-version: ${{ env.BUILD_JDK_VERSION }} |
🧰 Tools
🪛 actionlint (1.7.8)
205-205: 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)
🤖 Prompt for AI Agents
.github/workflows/actions_build.yml around lines 203 to 209: the job currently
references actions/setup-java@v3; update the uses line to actions/setup-java@v4
to match the other build job and receive latest fixes and security updates,
keeping the existing with inputs (distribution and java-version) unchanged.
To address the comments by coderabbit. line#1195 (comment) line#1195 (comment) - actions/checkout@v3 -> actions/checkout@v4 - actions/setup-java@v3 -> actions/setup-java@v4 - gradle/gradle-build-action@v2 -> gradle/actions/setup-gradle@v3
Motivation:
The latest Central Dogma docker image failes to start up on the
ubuntu-latestimage of GitHub Actions due to the following NPE:The exception seems fixed by https://bugs.openjdk.org/browse/JDK-8287073, so upgrading Java version will solve the issue.
Related: line/centraldogma-python#70
Modifications:
Result:
Central Dogma Docker image now starts up properly in GitHub Actions.
Summary by CodeRabbit