Бампнул версию джавы для агента оскрипта и добавил локаль в образ edt #57
Conversation
WalkthroughThe pull request introduces updates to various scripts and documentation related to the deployment of the Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (2)
edt/Dockerfile (2)
Line range hint
29-31: Remove redundant locale environment variables from the downloader stageThese environment variables are set in both the downloader and final stages. Since the downloader is a temporary build stage, these variables are not needed here and can be safely removed.
Apply this diff to remove the redundant environment variables:
# Установка необходимых пакетов и генерация локали RUN apt-get update \ && apt-get install -y \ locales \ p7zip-rar \ p7zip-full \ && rm -rf /var/lib/apt/lists/* \ && locale-gen ru_RU.UTF-8 \ && localedef -i ru_RU -c -f UTF-8 -A /usr/share/locale/locale.alias ru_RU.UTF-8 -# Установка переменных окружения для корректной работы локали -ENV LANG ru_RU.UTF-8 -ENV LANGUAGE ru_RU:ru -ENV LC_ALL ru_RU.UTF-8
Line range hint
45-51: Consider additional cleanup steps to reduce image sizeWhile the current cleanup is good, consider adding more paths to reduce the image size further.
Apply this diff to enhance cleanup:
&& rm -rf \ /var/lib/apt/lists/* \ /var/cache/debconf \ - /tmp/* \ + /tmp/* \ + /var/tmp/* \ + /usr/share/doc/* \ + /usr/share/man/* \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Layers.md(1 hunks)build-oscript-k8s-agent.sh(1 hunks)build-oscript-swarm-agent.bat(1 hunks)build-oscript-swarm-agent.sh(1 hunks)edt/Dockerfile(1 hunks)
🔇 Additional comments (7)
Layers.md (2)
58-58: Consider documenting locale changes for EDT image.
The PR title mentions locale changes for the EDT image, but these changes are not reflected in the documentation. Consider if these changes should be documented in the EDT section of this file.
Let's check for EDT-related changes in the codebase:
58-58: LGTM! Verify build script consistency.
The documentation update correctly reflects the base image change from adoptopenjdk:14-hotspot to eclipse-temurin:17 for the oscript layer.
Let's verify that this change is consistent with the related build scripts:
✅ Verification successful
Documentation correctly reflects the Java version update
The documentation update in Layers.md showing oscript поверх library/eclipse-temurin:17 is accurate and consistent with the implementation. The build scripts (build-oscript-swarm-agent.sh and build-oscript-k8s-agent.sh) confirm that the oscript image is built using eclipse-temurin:17 as the base image through the build arguments:
--build-arg BASE_IMAGE=eclipse-temurin
--build-arg BASE_TAG=17
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the base image is consistently defined across build scripts
# Expected: All occurrences should use eclipse-temurin:17
rg -l "adoptopenjdk:14-hotspot|eclipse-temurin:17" --type sh --type bat
Length of output: 101
Script:
#!/bin/bash
# Let's try a broader search for Docker base images in all files
rg -l "FROM.*adoptopenjdk|FROM.*eclipse-temurin"
# Also search for any Dockerfile that might contain these base images
fd Dockerfile --type f --exec cat {}
Length of output: 30848
Script:
#!/bin/bash
# Let's search for any Dockerfile or configuration that defines the base image for oscript
rg -l "FROM.*oscript" --type dockerfile
# Also search for any direct references to Java versions in the codebase
rg -l "adoptopenjdk|eclipse-temurin" --type dockerfile
Length of output: 168
Script:
#!/bin/bash
# Search for any files containing Java version references without file type restriction
rg -l "adoptopenjdk|eclipse-temurin"
# Search for any files containing oscript base image references
rg -l "FROM.*oscript"
# Also look for any Dockerfile files in the repository
fd Dockerfile
Length of output: 628
Script:
#!/bin/bash
# Let's check the content of the oscript Dockerfile and build scripts to verify the Java version change
cat oscript/Dockerfile
cat build-oscript-swarm-agent.sh
cat build-oscript-k8s-agent.sh
Length of output: 4633
build-oscript-k8s-agent.sh (2)
26-27: Good choice of base image and Java version!
The upgrade to Eclipse Temurin 17 (LTS) from AdoptOpenJDK 14 is a positive change as:
- Eclipse Temurin is the successor to AdoptOpenJDK
- Java 17 is an LTS release with long-term support
Let's verify the Java version requirements and dependencies:
✅ Verification successful
Base image upgrade to Eclipse Temurin 17 appears safe to proceed
Based on the verification results:
- The repository contains another Dockerfile that specifies Java 17 (temurin-17-jdk)
- No Java version-specific features (sealed classes, records) were found that could cause compatibility issues
- No build configuration files with strict Java version requirements were found
The change from AdoptOpenJDK 14 to Eclipse Temurin 17 aligns with the existing Java usage in the codebase and represents a move to a more stable, long-term support version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Java version requirements in project files
# Look for Java version specifications in build files and documentation
rg -g '!{*.sh,*.bat}' -i '(java|jdk|jre).*(version|1[1-9])'
# Check for potential Java version sensitive code patterns
ast-grep --pattern 'sealed class' # Java 17 feature
ast-grep --pattern 'record class' # Java 16 feature
ast-grep --pattern 'var =' # Java 11+ feature
Length of output: 350
Script:
#!/bin/bash
# Let's check for any Java-specific build configurations or dependency files
fd -e gradle -e pom.xml -e xml -e properties . -x cat {}
# Also check for any Java version requirements in Dockerfiles
fd Dockerfile -x grep -i "FROM.*\(temurin\|openjdk\|adoptopenjdk\)" {}
Length of output: 389
26-27: Verify application compatibility with Java 17
Since this is a major version upgrade from Java 14 to 17, ensure that:
- The oscript application is compatible with Java 17
- All dependencies support Java 17
- No deprecated APIs are in use
Let's check for potential compatibility issues:
✅ Verification successful
Java 17 upgrade appears safe for this context
The verification shows that:
- The oscript container is primarily running OneScript (a 1C:Enterprise scripting engine) on Mono runtime
- Java/JDK is only used as a base image (eclipse-temurin) and not directly by the application
- No Java-specific code or dependencies are present that could be affected by the Java version upgrade
Since Java is only used as a base container image and the application itself runs on Mono runtime, the upgrade from Java 14 to 17 should not impact the application's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for potential Java compatibility issues
# Search for common Java version-sensitive imports and APIs
rg -g '*.java' 'import.*?(javax\.xml|com\.sun|sun\.|jdk\.|oracle\.)'
# Look for deprecated API usage warnings in build logs or comments
rg -g '!{*.sh,*.bat}' '(?i)(deprecated|removal|unsupported|compatibility)'
Length of output: 296
Script:
#!/bin/bash
# Let's first check what Java/class files exist in the repository
fd -e java -e class
# Check for any build configuration files that might indicate Java version
fd -e gradle -e pom.xml -e build.gradle
# Look for any Java version specifications in configuration files
rg -g '*.{xml,gradle,properties,yaml,yml}' -i 'sourceCompatibility|targetCompatibility|java.version|javac|jdk'
# Check for any documentation about Java requirements
fd README -t f -X cat
Length of output: 7386
Script:
#!/bin/bash
# Let's check the Dockerfile and related files for oscript
fd -e Dockerfile -e sh -p oscript -X cat
# Check for any Java-related configuration in the oscript directory
fd -p oscript -e xml -e json -e conf -X cat
# Look for any version or compatibility specifications in oscript directory
rg -g 'oscript/*' -i 'java|jdk|version'
Length of output: 2700
build-oscript-swarm-agent.sh (1)
26-27: Excellent choice of base image and Java version!
The changes bring several improvements:
- Eclipse Temurin is the official successor to AdoptOpenJDK, now backed by the Eclipse Foundation
- Java 17 is an LTS release with support until September 2029, offering better stability and security compared to Java 14
Let's verify if the codebase is compatible with Java 17:
✅ Verification successful
Base image and Java version upgrade is safe to proceed
The verification shows that:
- The project appears to be a containerized application without direct Java source files
- The Java runtime is only used as a base image for running applications
- The Dockerfile in
jdk/already supports Temurin JDK 17 through an argument - No Java version-specific code or dependencies were found that could be impacted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential Java version compatibility issues
# Look for Java version-specific APIs or syntax that might need updates
# Search for Java version declarations in build files
echo "Checking build files for Java version declarations..."
rg -g "*.{gradle,xml,properties}" "sourceCompatibility|targetCompatibility|java.version|release"
# Look for potentially incompatible Java features
echo "Checking for Java 14-specific features that might need updates..."
rg -g "*.java" "sealed|record|instanceof\s+\w+\s+\w+" -A 2
# Check for dependencies that might have Java version constraints
echo "Checking for dependency version constraints..."
fd -e gradle -e xml --exec grep -l "compile\|implementation\|runtime" {} \;
Length of output: 750
Script:
#!/bin/bash
# Let's try a different approach to check Java-related files and configurations
# First, let's find all Java source files to confirm if the project has Java code
echo "Checking for Java source files..."
fd -e java
# Look for build configuration files
echo -e "\nChecking for build configuration files..."
fd -e gradle -e xml -e properties
# If we find any files, let's examine their content for Java version info
echo -e "\nChecking content of potential build files..."
fd -e gradle -e xml -e properties --exec cat {} \;
# Look for Dockerfile references to Java
echo -e "\nChecking Dockerfile references to Java..."
rg -g "Dockerfile*" "FROM.*adoptopenjdk|FROM.*temurin|JAVA|JDK"
Length of output: 961
edt/Dockerfile (2)
63-66: LGTM! Locale configuration is properly implemented
The locale environment variables are correctly set in the final image after generating the locale. This ensures proper character encoding and localization support for the application.
Line range hint 6-7: Verify Java version compatibility
The base image has been changed to eclipse-temurin:11. While this is a good choice for long-term support, please ensure that EDT version ${EDT_VERSION} is compatible with Java 11, as the PR title mentions a Java version bump.
| --build-arg BASE_IMAGE=onec-client-vnc ^ | ||
| --build-arg BASE_TAG=%ONEC_VERSION% ^ | ||
| -t %DOCKER_REGISTRY_URL%/onec-client-vnc-oscript:%ONEC_VERSION% ^ | ||
| --build-arg DOCKER_REGISTRY_URL=library ^ |
There was a problem hiding this comment.
Consider keeping registry URL configurable
Hardcoding the registry URL to library reduces script flexibility and might break existing workflows that expect to use a custom registry URL. Consider keeping this configurable via the %DOCKER_REGISTRY_URL% variable.
- --build-arg DOCKER_REGISTRY_URL=library ^
+ --build-arg DOCKER_REGISTRY_URL=%DOCKER_REGISTRY_URL% ^📝 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.
| --build-arg DOCKER_REGISTRY_URL=library ^ | |
| --build-arg DOCKER_REGISTRY_URL=%DOCKER_REGISTRY_URL% ^ |
| --build-arg DOCKER_REGISTRY_URL=library ^ | ||
| --build-arg BASE_IMAGE=eclipse-temurin ^ | ||
| --build-arg BASE_TAG=17 ^ | ||
| -t %DOCKER_REGISTRY_URL%/oscript-jdk:latest ^ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a specific version tag instead of 'latest'
Using the latest tag can lead to reproducibility issues as the image content may change over time. Consider using a specific version tag that matches your Java version or application version.
- -t %DOCKER_REGISTRY_URL%/oscript-jdk:latest ^
+ -t %DOCKER_REGISTRY_URL%/oscript-jdk:17 ^This change would:
- Make builds more reproducible
- Make it easier to track which Java version is being used
- Allow for easier rollback if needed
Committable suggestion skipped: line range outside the PR's diff.
nixel2007
merged commit 9cf9607
into
firstBitMarksistskaya:feature/first-bit
Summary by CodeRabbit
Documentation
oscriptlayer in the Jenkins agent documentation.New Features
eclipse-temurin:17base image for improved performance and compatibility.Improvements