Поддержка 1cedtcli и переход на базовый образ с JDK 17 для EDT 2024.1.0+ #53
Conversation
WalkthroughThe pull request introduces modifications to several scripts and a Dockerfile to implement conditional logic for selecting base images and tags based on the Changes
Possibly related PRs
Poem
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
build-edt-k8s-agent.sh(2 hunks)build-edt-swarm-agent.bat(2 hunks)build-edt-swarm-agent.sh(2 hunks)edt/Dockerfile(2 hunks)
🔇 Additional comments (6)
build-edt-swarm-agent.bat (2)
20-21: LGTM: Error handling is properly implemented.
The error check is consistent with the script's error handling pattern.
41-42: Verify availability of the base images.
The script now uses dynamic base images (azul/zulu-openjdk:17.0.13-jdk or eclipse-temurin:11). Please ensure these images are:
- Publicly available
- Compatible with your build environment
- Stable and maintained
build-edt-k8s-agent.sh (1)
49-50: Verify base image availability and k8s compatibility.
The addition of dynamic base image selection requires verification:
- Ensure both base images are accessible in all deployment environments
- Test k8s agent builds with both JDK versions
Let's verify the image availability:
Consider adding:
- Image availability checks before the build process
- A test matrix in CI to verify k8s agent functionality with both JDK versions
build-edt-swarm-agent.sh (1)
49-50: Verify the impact of base image changes across all builds.
The new base image configuration is only applied to the EDT image build but not to the oscript-downloader or edt-agent builds. This might lead to version incompatibility issues.
Let's verify the Dockerfile configurations:
Consider:
- Adding validation for BASE_IMAGE and BASE_TAG variables
- Documenting the JDK version requirements for each component
- Ensuring consistent base image usage across all interdependent containers
edt/Dockerfile (2)
77-77: LGTM! PATH environment variable is correctly configured
The PATH update properly includes both components while preserving the existing PATH value.
41-41: Consider adding base image validation for EDT 2024.1.0+
While the flexible base image configuration is good, the requirement to use azul/zulu-openjdk 17 for EDT 2024.1.0+ isn't enforced in the Dockerfile. Consider adding a build-time validation.
Here's a script to verify the base image settings in the build scripts:
build-edt-swarm-agent.bat
Outdated
| for /f "delims=." %%a in ("%EDT_VERSION%") do set EDT_MAJOR_VERSION=%aa | ||
| if %EDT_MAJOR_VERSION% GEQ "2024" ( | ||
| set BASE_IMAGE="azul/zulu-openjdk" | ||
| set BASE_TAG="17.0.13-jdk" | ||
| ) else ( | ||
| set BASE_IMAGE="eclipse-temurin" | ||
| set BASE_TAG="11" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling for version extraction.
The version extraction logic needs additional safeguards:
- EDT_VERSION validation is missing
- No error handling for the version extraction
- The comparison assumes proper version format
Consider this improved implementation:
-for /f "delims=." %%a in ("%EDT_VERSION%") do set EDT_MAJOR_VERSION=%aa
-if %EDT_MAJOR_VERSION% GEQ "2024" (
+if "%EDT_VERSION%"=="" (
+ echo Error: EDT_VERSION is not set
+ goto end
+)
+
+for /f "delims=." %%a in ("%EDT_VERSION%") do (
+ set EDT_MAJOR_VERSION=%%a
+ if "%%a"=="" (
+ echo Error: Failed to extract major version from %EDT_VERSION%
+ goto end
+ )
+)
+
+if %EDT_MAJOR_VERSION% GEQ "2024" (📝 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.
| for /f "delims=." %%a in ("%EDT_VERSION%") do set EDT_MAJOR_VERSION=%aa | |
| if %EDT_MAJOR_VERSION% GEQ "2024" ( | |
| set BASE_IMAGE="azul/zulu-openjdk" | |
| set BASE_TAG="17.0.13-jdk" | |
| ) else ( | |
| set BASE_IMAGE="eclipse-temurin" | |
| set BASE_TAG="11" | |
| ) | |
| if "%EDT_VERSION%"=="" ( | |
| echo Error: EDT_VERSION is not set | |
| goto end | |
| ) | |
| for /f "delims=." %%a in ("%EDT_VERSION%") do ( | |
| set EDT_MAJOR_VERSION=%%a | |
| if "%%a"=="" ( | |
| echo Error: Failed to extract major version from %EDT_VERSION% | |
| goto end | |
| ) | |
| ) | |
| if %EDT_MAJOR_VERSION% GEQ "2024" ( | |
| set BASE_IMAGE="azul/zulu-openjdk" | |
| set BASE_TAG="17.0.13-jdk" | |
| ) else ( | |
| set BASE_IMAGE="eclipse-temurin" | |
| set BASE_TAG="11" | |
| ) |
build-edt-k8s-agent.sh
Outdated
| #Если версия 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.0.13-jdk" | ||
| else | ||
| BASE_IMAGE="eclipse-temurin" | ||
| BASE_TAG="11" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve version check implementation and add validation.
The current version check has several areas for improvement:
- The check
"$EDT_VERSION" "2024"will match any version starting with "2024", not specifically 2024.1.0+ - There's no validation that EDT_VERSION is set
Consider this improved implementation:
-#Если версия EDT >= 2024.1.0, использовать JDK 17
-if [[ "$(printf "%s\n" "$EDT_VERSION" "2024" | sort -V | head -n 1)" == "2024" ]]; then
+# Use JDK 17 for EDT version >= 2024.1.0
+if [ -z "$EDT_VERSION" ]; then
+ echo "EDT_VERSION is not set"
+ exit 1
+fi
+
+if [[ "$(printf "%s\n" "$EDT_VERSION" "2024.1.0" | sort -V | head -n 1)" == "2024.1.0" ]]; then
BASE_IMAGE="azul/zulu-openjdk"
BASE_TAG="17.0.13-jdk"
else
BASE_IMAGE="eclipse-temurin"
BASE_TAG="11"
fi📝 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.
| #Если версия 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.0.13-jdk" | |
| else | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="11" | |
| fi | |
| # Use JDK 17 for EDT version >= 2024.1.0 | |
| if [ -z "$EDT_VERSION" ]; then | |
| echo "EDT_VERSION is not set" | |
| exit 1 | |
| fi | |
| if [[ "$(printf "%s\n" "$EDT_VERSION" "2024.1.0" | sort -V | head -n 1)" == "2024.1.0" ]]; then | |
| BASE_IMAGE="azul/zulu-openjdk" | |
| BASE_TAG="17.0.13-jdk" | |
| else | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="11" | |
| fi |
build-edt-swarm-agent.sh
Outdated
| #Если версия 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.0.13-jdk" | ||
| else | ||
| BASE_IMAGE="eclipse-temurin" | ||
| BASE_TAG="11" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve version comparison logic and documentation.
The current version comparison has several issues:
- It only checks the year part (2024) instead of the full version (2024.1.0)
- The comment is in Russian which may affect maintainability
Consider this improved implementation:
-#Если версия EDT >= 2024.1.0, использовать JDK 17
-if [[ "$(printf "%s\n" "$EDT_VERSION" "2024" | sort -V | head -n 1)" == "2024" ]]; then
+# Use JDK 17 for EDT version >= 2024.1.0
+if [[ "$(printf "%s\n" "$EDT_VERSION" "2024.1.0" | sort -V | head -n 1)" == "2024.1.0" ]]; then
BASE_IMAGE="azul/zulu-openjdk"
BASE_TAG="17.0.13-jdk"
else
BASE_IMAGE="eclipse-temurin"
BASE_TAG="11"
fi📝 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.
| #Если версия 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.0.13-jdk" | |
| else | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="11" | |
| fi | |
| # Use JDK 17 for EDT version >= 2024.1.0 | |
| if [[ "$(printf "%s\n" "$EDT_VERSION" "2024.1.0" | sort -V | head -n 1)" == "2024.1.0" ]]; then | |
| BASE_IMAGE="azul/zulu-openjdk" | |
| BASE_TAG="17.0.13-jdk" | |
| else | |
| BASE_IMAGE="eclipse-temurin" | |
| BASE_TAG="11" | |
| fi |
| && mv $(dirname $(find /opt/1C/1CE -name ring)) /opt/1C/1CE/components/1c-enterprise-ring \ | ||
| #1cedtcli отсутствует в EDT версии <2023.1.0 | ||
| && if [ -n "$(find /opt/1C/1CE -name 1cedtcli)" ]; then mv $(dirname $(find /opt/1C/1CE -name 1cedtcli)) /opt/1C/1CE/components/1cedtcli; fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize component installation and add error handling
The current implementation has several areas for improvement:
- The
findcommand is executed multiple times - No error handling for the ring component move
- Silent failures possible in the conditional move
Consider this more robust implementation:
- && mv $(dirname $(find /opt/1C/1CE -name ring)) /opt/1C/1CE/components/1c-enterprise-ring \
- #1cedtcli отсутствует в EDT версии <2023.1.0
- && if [ -n "$(find /opt/1C/1CE -name 1cedtcli)" ]; then mv $(dirname $(find /opt/1C/1CE -name 1cedtcli)) /opt/1C/1CE/components/1cedtcli; fi
+ && RING_PATH=$(find /opt/1C/1CE -name ring) \
+ && if [ -z "$RING_PATH" ]; then echo "Error: ring component not found" && exit 1; fi \
+ && mv "$(dirname "$RING_PATH")" /opt/1C/1CE/components/1c-enterprise-ring \
+ # 1cedtcli is only available in EDT version 2023.1.0 and above \
+ && CEDTCLI_PATH=$(find /opt/1C/1CE -name 1cedtcli) \
+ && if [ -n "$CEDTCLI_PATH" ]; then \
+ mv "$(dirname "$CEDTCLI_PATH")" /opt/1C/1CE/components/1cedtcli \
+ || { echo "Error moving 1cedtcli component"; exit 1; }; \
+ fi📝 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.
| && mv $(dirname $(find /opt/1C/1CE -name ring)) /opt/1C/1CE/components/1c-enterprise-ring \ | |
| #1cedtcli отсутствует в EDT версии <2023.1.0 | |
| && if [ -n "$(find /opt/1C/1CE -name 1cedtcli)" ]; then mv $(dirname $(find /opt/1C/1CE -name 1cedtcli)) /opt/1C/1CE/components/1cedtcli; fi | |
| && RING_PATH=$(find /opt/1C/1CE -name ring) \ | |
| && if [ -z "$RING_PATH" ]; then echo "Error: ring component not found" && exit 1; fi \ | |
| && mv "$(dirname "$RING_PATH")" /opt/1C/1CE/components/1c-enterprise-ring \ | |
| # 1cedtcli is only available in EDT version 2023.1.0 and above \ | |
| && CEDTCLI_PATH=$(find /opt/1C/1CE -name 1cedtcli) \ | |
| && if [ -n "$CEDTCLI_PATH" ]; then \ | |
| mv "$(dirname "$CEDTCLI_PATH")" /opt/1C/1CE/components/1cedtcli \ | |
| || { echo "Error moving 1cedtcli component"; exit 1; }; \ | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
build-edt-swarm-agent.bat (1)
Line range hint
11-42: Consider operational impacts of base image changes.The switch to different base images (
azul/zulu-openjdkfor EDT 2024+ andeclipse-temurinfor earlier versions) has several implications:
- As mentioned in the PR objectives, k8s compatibility needs testing
- Different base images might have different security implications
- Image size and layer caching efficiency might be affected
- CI/CD pipelines might need updates to handle both base images
Consider:
- Adding documentation about the base image requirements
- Creating a test matrix for different environments
- Implementing image size monitoring
build-edt-swarm-agent.sh (1)
49-50: Consider using distinct variable names for different images.The variables
BASE_IMAGEandBASE_TAGare used for both the downloader image (lines 36-37) and the EDT image. This could lead to confusion and maintenance issues. Consider using more specific variable names likeEDT_BASE_IMAGEandEDT_BASE_TAGto clearly distinguish their purpose.- --build-arg BASE_IMAGE=$BASE_IMAGE \ - --build-arg BASE_TAG=$BASE_TAG \ + --build-arg BASE_IMAGE=$EDT_BASE_IMAGE \ + --build-arg BASE_TAG=$EDT_BASE_TAG \And update the conditional block accordingly:
if [[ "$(printf "%s\n" "$EDT_VERSION" "2024" | sort -V | head -n 1)" == "2024" ]]; then - BASE_IMAGE="azul/zulu-openjdk" - BASE_TAG="17" + EDT_BASE_IMAGE="azul/zulu-openjdk" + EDT_BASE_TAG="17" else - BASE_IMAGE="eclipse-temurin" - BASE_TAG="11" + EDT_BASE_IMAGE="eclipse-temurin" + EDT_BASE_TAG="11" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
build-edt-k8s-agent.sh(2 hunks)build-edt-swarm-agent.bat(2 hunks)build-edt-swarm-agent.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build-edt-k8s-agent.sh
🔇 Additional comments (4)
build-edt-swarm-agent.bat (2)
11-18:
Add input validation and error handling for version extraction.
The version extraction and comparison logic needs additional safeguards:
- No validation if EDT_VERSION is set or empty
- No error handling for version extraction failures
- String comparison with "2024" could be problematic with different version formats
The previous review comment about adding input validation remains valid. Additionally, consider adding logging to help with troubleshooting:
+if "%EDT_VERSION%"=="" (
+ echo Error: EDT_VERSION environment variable is not set
+ goto end
+)
+
+echo Processing EDT version: %EDT_VERSION%
for /f "delims=." %%a in ("%EDT_VERSION%") do set EDT_MAJOR_VERSION=%%a
+if "%EDT_MAJOR_VERSION%"=="" (
+ echo Error: Failed to extract major version from %EDT_VERSION%
+ goto end
+)
+
+echo Detected major version: %EDT_MAJOR_VERSION%
if %EDT_MAJOR_VERSION% GEQ "2024" (
+ echo Using JDK 17 base image for EDT 2024+
set BASE_IMAGE="azul/zulu-openjdk"
set BASE_TAG="17"
) else (
+ echo Using JDK 11 base image for EDT pre-2024
set BASE_IMAGE="eclipse-temurin"
set BASE_TAG="11"
)41-42: 🛠️ Refactor suggestion
Improve Docker build argument handling and verification.
The build argument handling needs improvements:
- Variables should be quoted to handle potential spaces
- Base image existence should be verified before building
- --build-arg BASE_IMAGE=%BASE_IMAGE% ^
- --build-arg BASE_TAG=%BASE_TAG% ^
+ --build-arg BASE_IMAGE="%BASE_IMAGE%" ^
+ --build-arg BASE_TAG="%BASE_TAG%" ^Add verification before building:
build-edt-swarm-agent.sh (2)
17-24: Previous review comment is still applicable.
The version comparison logic and documentation issues highlighted in the previous review remain unaddressed.
Line range hint 1-1: Verify 1cedtcli implementation in Dockerfile.
The PR objective mentions adding 1cedtcli to PATH, but this implementation is not visible in the build script. Let's verify the implementation in the Dockerfile.
✅ Verification successful
1cedtcli is properly implemented in the Dockerfile
The Dockerfile correctly handles the 1cedtcli tool by:
- Moving it to
/opt/1C/1CE/components/1cedtcliif found in EDT installation - Adding it to the PATH environment variable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how 1cedtcli is handled in the Dockerfile
# Test: Look for 1cedtcli-related changes in the Dockerfile
rg -A 5 "1cedtcli" edt/Dockerfile
Length of output: 315
|
Спасибо! |
nixel2007
merged commit e7a928f
into
firstBitMarksistskaya:feature/first-bit
Summary by CodeRabbit
New Features
1cedtclicomponent within the Dockerfile.Bug Fixes
Documentation