Изменения для запуска EDT 2025.2.3 и платформы 8.3.27.1989#95
Open
AronMav wants to merge 1 commit intofirstBitMarksistskaya:feature/first-bitfrom
Open
Изменения для запуска EDT 2025.2.3 и платформы 8.3.27.1989#95AronMav wants to merge 1 commit intofirstBitMarksistskaya:feature/first-bitfrom
AronMav wants to merge 1 commit intofirstBitMarksistskaya:feature/first-bitfrom
Conversation
📝 WalkthroughWalkthroughSwitch EDT Java base selection to direct prefix checks for Changes
Sequence Diagram(s)(Skipped — changes are build-time and small control-flow edits that don't require multi-component runtime sequencing visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
build-edt.sh (1)
33-34: Version information could be logged for debugging.As per coding guidelines, version information should be included in Docker build outputs for debugging. Consider adding an echo statement.
Add version logging
edt_version=$EDT_VERSION edt_escaped="${edt_version// /_}" + +echo "Building EDT $edt_version with base image: ${BASE_IMAGE}:${BASE_TAG}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-edt.sh` around lines 33 - 34, Add a simple log line that echoes the EDT version before the Docker build so the build output includes the version; update the script around the edt_version and edt_escaped variables (edt_version=$EDT_VERSION and edt_escaped="${edt_version// /_}") to print them (e.g., echo "Building EDT version: $edt_version (escaped: $edt_escaped)") so CI/build logs contain the version information for debugging.swarm-jenkins-agent/docker-entrypoint.sh (1)
7-11: Consider adding Xvfb readiness check.Two sequential
sleep 1commands are used to wait for Xvfb and/init. Fixed sleeps can be unreliable; the display might not be ready in time on slower systems or be unnecessarily delayed on faster ones.Optional: Add a simple readiness check
Xvfb ${DISPLAY:-:99} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 -nolisten tcp & -sleep 1 +# Дождаться готовности виртуального дисплея +for i in $(seq 1 10); do + xdpyinfo -display ${DISPLAY:-:99} >/dev/null 2>&1 && break + sleep 0.5 +done if [ -f "/init" ]; then /init & fi - -sleep 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swarm-jenkins-agent/docker-entrypoint.sh` around lines 7 - 11, The current entrypoint uses fixed sleeps to wait for /init and Xvfb which is flaky; replace the second fixed sleep with a readiness loop that polls for X display and /init completion: after launching /init in background (the existing "/init &"), capture its PID, then loop with a short sleep until either the X server is ready (e.g., test with xdpyinfo -display "$DISPLAY" or check /tmp/.X11-unix/X${DISPLAY#*:}) and the /init process is still running (or has created its expected socket/file), or a timeout is reached; if readiness succeeds continue, otherwise exit non-zero. Update the script around the existing "/init &" and "sleep 1" lines to implement this polling and timeout logic.edt/Dockerfile (2)
70-70: Overly broadchmod -R +x .on distribution files.Making all files recursively executable is broader than necessary. While it works, it could mask issues or affect file permissions unexpectedly.
Consider targeting only the installer binary
-RUN chmod -R +x . \ +RUN chmod +x ./1ce-installer-cli \ && ./1ce-installer-cli install all --ignore-hardware-checks --ignore-signature-warnings \If other scripts in the distribution also need execute permissions, a more targeted approach like
find . -name "*.sh" -exec chmod +x {} \;might be preferable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@edt/Dockerfile` at line 70, The Dockerfile currently uses the overly broad RUN chmod -R +x . which makes every file executable; instead, change that step to only set execute permission on the installer binary (the packaged executable produced by your build) and, if necessary, on shell scripts by using a targeted find for files ending in .sh; update the RUN chmod -R +x . line to two focused operations: one that marks the installer binary executable and one optional find-based operation that marks only *.sh scripts executable so you avoid mass-changing permissions.
51-62: Document the reason EDT installer requires thegetconfstub for Ubuntu 24.04 compatibility.The shadowing of
/usr/bin/getconf(fromcoreutils) by the stub at/usr/local/bin/getconfis intentional, as indicated by the inline comment "создаем заглушку для getconf" and the adjacent comment about bypassing Ubuntu 24.04 restrictions. However, the code lacks explanation of why the realgetconffromcoreutilsis insufficient for the EDT installer. Consider adding a clearer comment documenting the specific EDT installer check or version that requires this workaround.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@edt/Dockerfile` around lines 51 - 62, Add a concise comment above the getconf stub in the Dockerfile explaining that the stub at /usr/local/bin/getconf intentionally shadows the coreutils getconf to satisfy the EDT installer’s compatibility check on Ubuntu 24.04 (the installer queries getconf LONG_BIT and fails against the shipped coreutils behavior/version), include the specific installer check or version string (e.g., the EDT installer’s LONG_BIT check or installer version that triggers the workaround), and cite Ubuntu 24.04 as the affected target so future maintainers understand why the replacement is necessary and safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build-edt.sh`:
- Around line 18-26: Update the version check to be future-proof and keep
Windows and Bash scripts consistent: extract the major version from EDT_VERSION
(e.g., EDT_MAJOR=${EDT_VERSION%%.*}) and do a numeric comparison (if [[
"$EDT_MAJOR" -ge 2024 ]] ) to select BASE_IMAGE/BASE_TAG (set
BASE_IMAGE="eclipse-temurin" and BASE_TAG="17-jdk-jammy" for 2024+ otherwise
"eclipse-temurin" "11-jdk-focal"); mirror the same numeric-major logic in
build-edt.bat (use EDT_MAJOR and GEQ 2024) and update its base-image names to
match the bash script so both platforms use the same image choices.
In `@swarm-jenkins-agent/docker-entrypoint.sh`:
- Around line 3-5: The script starts Xvfb with a default DISPLAY of :99 and
references DISPLAY_HEIGHT which isn't guaranteed by the base image; update the
Xvfb invocation to use the same defaults as the base image (use ${DISPLAY:-:0}
to match the client-vnc ENV and set an explicit default for DISPLAY_HEIGHT, e.g.
${DISPLAY_HEIGHT:-900}), or ensure the base image/compose sets DISPLAY,
DISPLAY_WIDTH and DISPLAY_HEIGHT consistently; modify the Xvfb command (Xvfb ...
${DISPLAY:-:0} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 ...)
and add/document the required env defaults so Xvfb, DISPLAY, DISPLAY_WIDTH and
DISPLAY_HEIGHT are aligned with client-vnc's configuration.
- Around line 13-18: Validate that required environment variables
DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL,
DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL, and
DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET are set at the top of
docker-entrypoint.sh and exit with a clear error if any are missing; after that,
run wget to fetch agent.jar but check its exit status and print a descriptive
error (including the URL) on failure; then run java -jar agent.jar and capture
its exit code, printing an explanatory error message and exiting with that code
if it fails (also consider trapping signals or using set -o pipefail to avoid
silent failures).
---
Nitpick comments:
In `@build-edt.sh`:
- Around line 33-34: Add a simple log line that echoes the EDT version before
the Docker build so the build output includes the version; update the script
around the edt_version and edt_escaped variables (edt_version=$EDT_VERSION and
edt_escaped="${edt_version// /_}") to print them (e.g., echo "Building EDT
version: $edt_version (escaped: $edt_escaped)") so CI/build logs contain the
version information for debugging.
In `@edt/Dockerfile`:
- Line 70: The Dockerfile currently uses the overly broad RUN chmod -R +x .
which makes every file executable; instead, change that step to only set execute
permission on the installer binary (the packaged executable produced by your
build) and, if necessary, on shell scripts by using a targeted find for files
ending in .sh; update the RUN chmod -R +x . line to two focused operations: one
that marks the installer binary executable and one optional find-based operation
that marks only *.sh scripts executable so you avoid mass-changing permissions.
- Around line 51-62: Add a concise comment above the getconf stub in the
Dockerfile explaining that the stub at /usr/local/bin/getconf intentionally
shadows the coreutils getconf to satisfy the EDT installer’s compatibility check
on Ubuntu 24.04 (the installer queries getconf LONG_BIT and fails against the
shipped coreutils behavior/version), include the specific installer check or
version string (e.g., the EDT installer’s LONG_BIT check or installer version
that triggers the workaround), and cite Ubuntu 24.04 as the affected target so
future maintainers understand why the replacement is necessary and safe.
In `@swarm-jenkins-agent/docker-entrypoint.sh`:
- Around line 7-11: The current entrypoint uses fixed sleeps to wait for /init
and Xvfb which is flaky; replace the second fixed sleep with a readiness loop
that polls for X display and /init completion: after launching /init in
background (the existing "/init &"), capture its PID, then loop with a short
sleep until either the X server is ready (e.g., test with xdpyinfo -display
"$DISPLAY" or check /tmp/.X11-unix/X${DISPLAY#*:}) and the /init process is
still running (or has created its expected socket/file), or a timeout is
reached; if readiness succeeds continue, otherwise exit non-zero. Update the
script around the existing "/init &" and "sleep 1" lines to implement this
polling and timeout logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eba9b520-1edf-4e58-b180-81beb07b51f3
📒 Files selected for processing (4)
build-edt.shclient-vnc/Dockerfileedt/Dockerfileswarm-jenkins-agent/docker-entrypoint.sh
Comment on lines
+18
to
26
| if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; then | ||
| # Для новых версий используем Ubuntu + Java 17 | ||
| # (Это надежнее, чем просто голая Java-платформа) | ||
| BASE_IMAGE="eclipse-temurin" | ||
| BASE_TAG="17-jdk-jammy" | ||
| else | ||
| BASE_IMAGE="eclipse-temurin" | ||
| BASE_TAG="11" | ||
| BASE_TAG="11-jdk-focal" | ||
| fi |
There was a problem hiding this comment.
Version detection updated to prefix matching - good improvement.
The pattern-based approach (2024*, 2025*) is cleaner and aligns with the Dockerfile changes. However, there are two concerns:
-
Divergence with Windows script:
build-edt.bat(context snippet) still uses numeric comparison (GEQ "2024") and the old base images (azul/zulu-openjdk:17,eclipse-temurin:11). This creates inconsistency between platforms. -
Future versions: The pattern explicitly matches 2024 and 2025, but future versions (2026+) will fall through to Java 11, which might not be correct if EDT continues requiring Java 17+.
Consider using a more future-proof pattern
-if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; then
+# EDT 2024+ требует Java 17
+if [[ "$EDT_VERSION" =~ ^202[4-9] ]] || [[ "$EDT_VERSION" =~ ^20[3-9] ]]; then
# Для новых версий используем Ubuntu + Java 17
# (Это надежнее, чем просто голая Java-платформа)
BASE_IMAGE="eclipse-temurin"
BASE_TAG="17-jdk-jammy" Or use numeric comparison like the .bat file for consistency:
EDT_MAJOR=${EDT_VERSION%%.*}
if [[ "$EDT_MAJOR" -ge 2024 ]]; then📝 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.
Suggested change
| if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; then | |
| # Для новых версий используем Ubuntu + Java 17 | |
| # (Это надежнее, чем просто голая Java-платформа) | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="17-jdk-jammy" | |
| else | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="11" | |
| BASE_TAG="11-jdk-focal" | |
| fi | |
| # EDT 2024+ требует Java 17 | |
| if [[ "$EDT_VERSION" =~ ^202[4-9] ]] || [[ "$EDT_VERSION" =~ ^20[3-9] ]]; then | |
| # Для новых версий используем Ubuntu + Java 17 | |
| # (Это надежнее, чем просто голая Java-платформа) | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="17-jdk-jammy" | |
| else | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="11-jdk-focal" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-edt.sh` around lines 18 - 26, Update the version check to be
future-proof and keep Windows and Bash scripts consistent: extract the major
version from EDT_VERSION (e.g., EDT_MAJOR=${EDT_VERSION%%.*}) and do a numeric
comparison (if [[ "$EDT_MAJOR" -ge 2024 ]] ) to select BASE_IMAGE/BASE_TAG (set
BASE_IMAGE="eclipse-temurin" and BASE_TAG="17-jdk-jammy" for 2024+ otherwise
"eclipse-temurin" "11-jdk-focal"); mirror the same numeric-major logic in
build-edt.bat (use EDT_MAJOR and GEQ 2024) and update its base-image names to
match the bash script so both platforms use the same image choices.
Comment on lines
+3
to
+5
| # Запустить виртуальный дисплей напрямую (не через s6, т.к. он не PID1) | ||
| Xvfb ${DISPLAY:-:99} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 -nolisten tcp & | ||
| sleep 1 |
There was a problem hiding this comment.
Potential DISPLAY mismatch with base image.
The base image (client-vnc/Dockerfile line 99) sets ENV DISPLAY=:0, but here the default is :99. This could cause issues if other components expect :0.
Also, DISPLAY_HEIGHT is referenced but not declared in the base image (only DISPLAY and DISPLAY_WIDTH are set per context snippet 2). Consider aligning defaults or documenting the expected environment variables.
Suggested fix to align with base image defaults
-# Запустить виртуальный дисплей напрямую (не через s6, т.к. он не PID1)
-Xvfb ${DISPLAY:-:99} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 -nolisten tcp &
+# Запустить виртуальный дисплей напрямую (не через s6, т.к. он не PID1)
+Xvfb ${DISPLAY:-:0} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 -nolisten tcp &📝 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.
Suggested change
| # Запустить виртуальный дисплей напрямую (не через s6, т.к. он не PID1) | |
| Xvfb ${DISPLAY:-:99} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 -nolisten tcp & | |
| sleep 1 | |
| # Запустить виртуальный дисплей напрямую (не через s6, т.к. он не PID1) | |
| Xvfb ${DISPLAY:-:0} -screen 0 ${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 -nolisten tcp & | |
| sleep 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@swarm-jenkins-agent/docker-entrypoint.sh` around lines 3 - 5, The script
starts Xvfb with a default DISPLAY of :99 and references DISPLAY_HEIGHT which
isn't guaranteed by the base image; update the Xvfb invocation to use the same
defaults as the base image (use ${DISPLAY:-:0} to match the client-vnc ENV and
set an explicit default for DISPLAY_HEIGHT, e.g. ${DISPLAY_HEIGHT:-900}), or
ensure the base image/compose sets DISPLAY, DISPLAY_WIDTH and DISPLAY_HEIGHT
consistently; modify the Xvfb command (Xvfb ... ${DISPLAY:-:0} -screen 0
${DISPLAY_WIDTH:-1440}x${DISPLAY_HEIGHT:-900}x24 ...) and add/document the
required env defaults so Xvfb, DISPLAY, DISPLAY_WIDTH and DISPLAY_HEIGHT are
aligned with client-vnc's configuration.
Comment on lines
+13
to
+18
| wget -O agent.jar $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL && \ | ||
| java -jar agent.jar \ | ||
| -jnlpUrl $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL \ | ||
| -secret $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET \ | ||
| -noReconnect \ | ||
| -workDir /home/jenkins |
There was a problem hiding this comment.
Missing error handling for agent startup.
The script lacks proper error handling:
- If
wgetfails, no informative error message is shown - If
java -jarfails, the container exits silently - Environment variables (
DOCKER_SWARM_PLUGIN_JENKINS_AGENT_*) are not validated before use
As per coding guidelines, build scripts should include proper error checking and validate required environment variables.
Suggested improvement with validation
-wget -O agent.jar $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL && \
- java -jar agent.jar \
+# Проверка обязательных переменных окружения
+: "${DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL:?Variable not set}"
+: "${DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL:?Variable not set}"
+: "${DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET:?Variable not set}"
+
+if ! wget -O agent.jar "$DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL"; then
+ echo "Failed to download Jenkins agent JAR"
+ exit 1
+fi
+
+exec java -jar agent.jar \
-jnlpUrl $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL \
-secret $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET \
-noReconnect \
-workDir /home/jenkins📝 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.
Suggested change
| wget -O agent.jar $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL && \ | |
| java -jar agent.jar \ | |
| -jnlpUrl $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL \ | |
| -secret $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET \ | |
| -noReconnect \ | |
| -workDir /home/jenkins | |
| # Проверка обязательных переменных окружения | |
| : "${DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL:?Variable not set}" | |
| : "${DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL:?Variable not set}" | |
| : "${DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET:?Variable not set}" | |
| if ! wget -O agent.jar "$DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL"; then | |
| echo "Failed to download Jenkins agent JAR" | |
| exit 1 | |
| fi | |
| exec java -jar agent.jar \ | |
| -jnlpUrl $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL \ | |
| -secret $DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET \ | |
| -noReconnect \ | |
| -workDir /home/jenkins |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@swarm-jenkins-agent/docker-entrypoint.sh` around lines 13 - 18, Validate that
required environment variables DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JAR_URL,
DOCKER_SWARM_PLUGIN_JENKINS_AGENT_JNLP_URL, and
DOCKER_SWARM_PLUGIN_JENKINS_AGENT_SECRET are set at the top of
docker-entrypoint.sh and exit with a clear error if any are missing; after that,
run wget to fetch agent.jar but check its exit status and print a descriptive
error (including the URL) on failure; then run java -jar agent.jar and capture
its exit code, printing an explanatory error message and exiting with that code
if it fails (also consider trapping signals or using set -o pipefail to avoid
silent failures).
Member
AronMav
force-pushed
the
feature/first-bit
branch
from
4ad3849 to
46e5830
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build-edt.sh (1)
21-22: Consider adding informative output about selected Java version.Adding a log message would help with debugging build issues, per coding guidelines.
📝 Proposed improvement
BASE_IMAGE="eclipse-temurin" BASE_TAG="17-jdk-jammy" + echo "EDT $EDT_VERSION detected: using Java 17 base image ($BASE_IMAGE:$BASE_TAG)" else BASE_IMAGE="eclipse-temurin" BASE_TAG="11-jdk-focal" + echo "EDT $EDT_VERSION detected: using Java 11 base image ($BASE_IMAGE:$BASE_TAG)" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-edt.sh` around lines 21 - 22, Add a short informative log/echo in build-edt.sh right after the BASE_IMAGE and BASE_TAG declarations that prints the selected Java base image and tag (referencing BASE_IMAGE and BASE_TAG) so build logs show which Java version is being used; ensure the message is clear and prefixed (e.g., "Using Java base image: ...") and appears early in the script before any build steps that depend on these variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build-edt.sh`:
- Around line 17-26: The script uses EDT_VERSION without validating it; add a
presence check at the top of build-edt.sh that fails fast (print a clear error
and exit non-zero) if EDT_VERSION is unset or empty, before the conditional that
assigns BASE_IMAGE and BASE_TAG; ensure the validation message references
EDT_VERSION so callers know which env var is missing and keep the existing
branching logic (the lines that set BASE_IMAGE and BASE_TAG) unchanged aside
from being protected by this pre-check.
---
Nitpick comments:
In `@build-edt.sh`:
- Around line 21-22: Add a short informative log/echo in build-edt.sh right
after the BASE_IMAGE and BASE_TAG declarations that prints the selected Java
base image and tag (referencing BASE_IMAGE and BASE_TAG) so build logs show
which Java version is being used; ensure the message is clear and prefixed
(e.g., "Using Java base image: ...") and appears early in the script before any
build steps that depend on these variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4a66236-9c9b-4a34-a726-5e76cd677e29
📒 Files selected for processing (3)
build-edt.shclient-vnc/Dockerfileedt/Dockerfile
✅ Files skipped from review due to trivial changes (1)
- edt/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (1)
- client-vnc/Dockerfile
Comment on lines
17
to
26
| #Если версия EDT >= 2024.1.0, использовать JDK 17 | ||
| if [[ "$(printf "%s\n" "$EDT_VERSION" "2024" | sort -V | head -n 1)" == "2024" ]]; then | ||
| BASE_IMAGE="azul/zulu-openjdk" | ||
| BASE_TAG="17" | ||
| if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; then | ||
| # Для новых версий используем Ubuntu + Java 17 | ||
| # (Это надежнее, чем просто голая Java-платформа) | ||
| BASE_IMAGE="eclipse-temurin" | ||
| BASE_TAG="17-jdk-jammy" | ||
| else | ||
| BASE_IMAGE="eclipse-temurin" | ||
| BASE_TAG="11" | ||
| BASE_TAG="11-jdk-focal" | ||
| fi |
There was a problem hiding this comment.
Missing validation for required EDT_VERSION environment variable.
The script uses EDT_VERSION at line 18 without validating that it's set. If undefined, the version check will silently fail and default to Java 11, which may cause confusing build failures for EDT 2024+/2025+ users.
🛡️ Proposed fix to add validation
+if [ -z "${EDT_VERSION}" ]; then
+ echo "Error: EDT_VERSION environment variable is required"
+ exit 1
+fi
+
`#Если` версия EDT >= 2024.1.0, использовать JDK 17
if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; thenAs per coding guidelines: "Validate required environment variables in build scripts"
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 18-18: Possible misspelling: EDT_VERSION may not be assigned. Did you mean edt_version?
(SC2153)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-edt.sh` around lines 17 - 26, The script uses EDT_VERSION without
validating it; add a presence check at the top of build-edt.sh that fails fast
(print a clear error and exit non-zero) if EDT_VERSION is unset or empty, before
the conditional that assigns BASE_IMAGE and BASE_TAG; ensure the validation
message references EDT_VERSION so callers know which env var is missing and keep
the existing branching logic (the lines that set BASE_IMAGE and BASE_TAG)
unchanged aside from being protected by this pre-check.
46e5830 to
48f3d5f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
build-edt.sh (2)
18-18:⚠️ Potential issue | 🟡 MinorПроверьте
EDT_VERSIONдо условия.Если переменная пустая или не задана, этот блок молча выбирает Java 11, и причина ошибки всплывёт только позже в
docker build. Лучше падать сразу с явным сообщением.🛡️ Возможный вариант
+if [ -z "${EDT_VERSION:-}" ]; then + echo "Error: EDT_VERSION environment variable is required" + exit 1 +fi + if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; thenAs per coding guidelines: "Validate required environment variables in build scripts"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-edt.sh` at line 18, Перед условием проверки версии EDT (строка с [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]) добавьте явную валидацию переменной EDT_VERSION: если она пустая или не задана, скрипт должен завершаться с понятным сообщением об ошибке и ненулевым кодом выхода; используйте проверку на пустоту (например -z) и выводьте четкий текст, объясняющий, что нужно задать EDT_VERSION и допустимые значения, чтобы избежать молчалого выбора Java 11 и последующих неочевидных ошибок при docker build.
18-25:⚠️ Potential issue | 🟠 MajorПереведите выбор JDK на числовое сравнение major-версии.
Комментарий над блоком говорит про
>= 2024.1.0, но текущее условие покрывает только2024*и2025*.2026.xздесь уже уйдёт в ветку Java 11, и логика при этом разойдётся сbuild-edt.bat:15-22, где сравнение идёт по major-версии.🧩 Возможный вариант
-if [[ "$EDT_VERSION" == 2024* ]] || [[ "$EDT_VERSION" == 2025* ]]; then +EDT_MAJOR=${EDT_VERSION%%.*} +if [[ "$EDT_MAJOR" -ge 2024 ]]; then BASE_IMAGE="eclipse-temurin" BASE_TAG="17-jdk-jammy" else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-edt.sh` around lines 18 - 25, The version check in build-edt.sh uses string globbing for EDT_VERSION (matching "2024*" or "2025*") which misroutes future majors (e.g. 2026) to the Java 11 branch; change the condition to a numeric major-version comparison by extracting the major portion from EDT_VERSION (e.g. EDT_MAJOR from EDT_VERSION by splitting at the first dot) and use a numeric test (EDT_MAJOR -ge 2024) to select BASE_IMAGE="eclipse-temurin" and BASE_TAG="17-jdk-jammy", otherwise fall back to the Java 11 tag; update all uses of EDT_VERSION comparison logic in this script so it aligns with the major-version logic used in build-edt.bat.
🧹 Nitpick comments (2)
edt/Dockerfile (2)
64-65: Сохраняйте поведение системногоgetconfдля остальных запросов.Стаб сейчас подменяет
getconfцеликом и для всего, кромеLONG_BIT, возвращаетunknown. Разlibc-binуже установлен, безопаснее перехватывать толькоLONG_BIT, а остальные вызовы делегировать системномуgetconf.♻️ Возможный вариант
- && echo '#!/bin/sh\nif [ "$1" = "LONG_BIT" ]; then echo "64"; else echo "unknown"; fi' > /usr/local/bin/getconf \ + && printf '%s\n' \ + '#!/bin/sh' \ + 'if [ "$1" = "LONG_BIT" ]; then' \ + ' echo "64"' \ + 'else' \ + ' exec /usr/bin/getconf "$@"' \ + 'fi' > /usr/local/bin/getconf \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@edt/Dockerfile` around lines 64 - 65, Скрипт /usr/local/bin/getconf, который вы создаёте, затирает поведение системного getconf для всех ключей кроме LONG_BIT; исправьте его на прокси-обёртку: при аргументе "LONG_BIT" вернуть "64", во всех остальных случаях делегировать вызов системному getconf из libc-bin (например /usr/bin/getconf или явный путь к системному двоичному файлу), и оставить права выполнения; обновите команду, которая пишет /usr/local/bin/getconf, чтобы реализовать эту логику вместо всегда возвращающего "unknown".
6-8: Синхронизируйте дефолтыEDT_VERSIONиBASE_TAG.
BASE_TAGтеперь по умолчанию ведёт на Java 17 независимо отEDT_VERSION, а этот Dockerfile всё ещё дефолтитEDT_VERSIONв2021.3. В итоге прямойdocker buildили сборка с переопределением толькоEDT_VERSIONлегко получает несогласованную связку. Либо обновите дефолтныйEDT_VERSION, либо уберите один из дефолтов, чтобы выбор версии задавался явно.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@edt/Dockerfile` around lines 6 - 8, The Dockerfile currently defaults EDT_VERSION=2021.3 while BASE_TAG=17-jdk-jammy (Java 17), causing mismatched defaults; update the Dockerfile to keep these defaults in sync by either (a) bumping the EDT_VERSION default to a release that requires Java 17 (e.g., set EDT_VERSION to the 2025.x default), or (b) removing one of the ARG defaults so callers must explicitly pass EDT_VERSION or BASE_TAG; adjust the ARG declarations for EDT_VERSION and BASE_TAG accordingly and ensure any downstream references (e.g., uses of EDT_VERSION, BASE_IMAGE, BASE_TAG) reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@build-edt.sh`:
- Line 18: Перед условием проверки версии EDT (строка с [[ "$EDT_VERSION" ==
2024* ]] || [[ "$EDT_VERSION" == 2025* ]]) добавьте явную валидацию переменной
EDT_VERSION: если она пустая или не задана, скрипт должен завершаться с понятным
сообщением об ошибке и ненулевым кодом выхода; используйте проверку на пустоту
(например -z) и выводьте четкий текст, объясняющий, что нужно задать EDT_VERSION
и допустимые значения, чтобы избежать молчалого выбора Java 11 и последующих
неочевидных ошибок при docker build.
- Around line 18-25: The version check in build-edt.sh uses string globbing for
EDT_VERSION (matching "2024*" or "2025*") which misroutes future majors (e.g.
2026) to the Java 11 branch; change the condition to a numeric major-version
comparison by extracting the major portion from EDT_VERSION (e.g. EDT_MAJOR from
EDT_VERSION by splitting at the first dot) and use a numeric test (EDT_MAJOR -ge
2024) to select BASE_IMAGE="eclipse-temurin" and BASE_TAG="17-jdk-jammy",
otherwise fall back to the Java 11 tag; update all uses of EDT_VERSION
comparison logic in this script so it aligns with the major-version logic used
in build-edt.bat.
---
Nitpick comments:
In `@edt/Dockerfile`:
- Around line 64-65: Скрипт /usr/local/bin/getconf, который вы создаёте,
затирает поведение системного getconf для всех ключей кроме LONG_BIT; исправьте
его на прокси-обёртку: при аргументе "LONG_BIT" вернуть "64", во всех остальных
случаях делегировать вызов системному getconf из libc-bin (например
/usr/bin/getconf или явный путь к системному двоичному файлу), и оставить права
выполнения; обновите команду, которая пишет /usr/local/bin/getconf, чтобы
реализовать эту логику вместо всегда возвращающего "unknown".
- Around line 6-8: The Dockerfile currently defaults EDT_VERSION=2021.3 while
BASE_TAG=17-jdk-jammy (Java 17), causing mismatched defaults; update the
Dockerfile to keep these defaults in sync by either (a) bumping the EDT_VERSION
default to a release that requires Java 17 (e.g., set EDT_VERSION to the 2025.x
default), or (b) removing one of the ARG defaults so callers must explicitly
pass EDT_VERSION or BASE_TAG; adjust the ARG declarations for EDT_VERSION and
BASE_TAG accordingly and ensure any downstream references (e.g., uses of
EDT_VERSION, BASE_IMAGE, BASE_TAG) reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: daeca18b-28c5-4dd1-abe1-eaae703c18b9
📒 Files selected for processing (3)
build-edt.shclient-vnc/Dockerfileedt/Dockerfile
✅ Files skipped from review due to trivial changes (1)
- client-vnc/Dockerfile
Author
|
@nixel2007 Сделал |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Improvements