Сборка образов 1с #45
Conversation
WalkthroughThe pull request introduces modifications to the GitHub Actions workflow and multiple shell scripts related to Docker image building. Key changes include the addition of environment variables for Docker configuration, conditional checks for the Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant DockerLogin
participant DockerBuild
Workflow->>DockerLogin: Check if DOCKER_LOGIN is not empty
alt Login Credentials Provided
DockerLogin->>DockerLogin: Execute docker login
else No Credentials
DockerLogin->>DockerLogin: Skip login
end
Workflow->>DockerBuild: Execute Docker build commands
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: 7
🧹 Outside diff range and nitpick comments (17)
build-oscript-k8s-agent.sh (2)
4-6: Approve the login check, but suggest improvements for robustness.The addition of the DOCKER_LOGIN check is a good improvement to prevent unintended login attempts. However, we can further enhance the script's robustness:
- Use the
-ntest for non-empty strings, which is more idiomatic in bash.- Check all required variables for the docker login command.
Consider refactoring the conditional check as follows:
-if [ $DOCKER_LOGIN != '' ] ; then - docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL +if [ -n "$DOCKER_LOGIN" ] && [ -n "$DOCKER_PASSWORD" ] && [ -n "$DOCKER_REGISTRY_URL" ]; then + docker login -u "$DOCKER_LOGIN" -p "$DOCKER_PASSWORD" "$DOCKER_REGISTRY_URL" fiThis change ensures all required variables are non-empty before attempting to log in and uses proper quoting for variable expansion.
Line range hint
1-38: Suggest improvements for overall script robustness and maintainability.While the main change in this PR is good, there are several areas where we can improve the overall script:
- Add checks for required environment variables.
- Use proper quoting for variable expansions.
- Add error handling for docker commands.
- Consider using arrays for build arguments to improve readability.
Here's a refactored version of the script incorporating these suggestions:
#!/bin/bash set -euo pipefail # Check required environment variables required_vars=("DOCKER_REGISTRY_URL" "DOCKER_SYSTEM_PRUNE" "NO_CACHE") for var in "${required_vars[@]}"; do if [ -z "${!var}" ]; then echo "Error: $var is not set" >&2 exit 1 fi done # Docker login if [ -n "${DOCKER_LOGIN:-}" ] && [ -n "${DOCKER_PASSWORD:-}" ]; then if ! docker login -u "$DOCKER_LOGIN" -p "$DOCKER_PASSWORD" "$DOCKER_REGISTRY_URL"; then echo "Error: Docker login failed" >&2 exit 1 fi fi # Docker system prune if [ "$DOCKER_SYSTEM_PRUNE" = 'true' ]; then docker system prune -af fi # Set build arguments build_args=( "--pull" "--build-arg" "DOCKER_REGISTRY_URL=library" "--build-arg" "BASE_IMAGE=adoptopenjdk" "--build-arg" "BASE_TAG=14-hotspot" ) if [ "$NO_CACHE" = 'true' ]; then build_args+=("--no-cache") fi # Build oscript-jdk image if ! docker build "${build_args[@]}" -t "$DOCKER_REGISTRY_URL/oscript-jdk:latest" -f oscript/Dockerfile .; then echo "Error: Failed to build oscript-jdk image" >&2 exit 1 fi # Update build arguments for oscript-agent build_args=( "--build-arg" "DOCKER_REGISTRY_URL=$DOCKER_REGISTRY_URL" "--build-arg" "BASE_IMAGE=oscript-jdk" "--build-arg" "BASE_TAG=latest" ) if [ "$NO_CACHE" = 'true' ]; then build_args+=("--no-cache") fi # Build oscript-agent image if ! docker build "${build_args[@]}" -t "$DOCKER_REGISTRY_URL/oscript-agent:latest" -f k8s-jenkins-agent/Dockerfile .; then echo "Error: Failed to build oscript-agent image" >&2 exit 1 fi # Push oscript-agent image if ! docker push "$DOCKER_REGISTRY_URL/oscript-agent:latest"; then echo "Error: Failed to push oscript-agent image" >&2 exit 1 fiThis refactored version improves error handling, variable usage, and overall script structure. It also uses arrays for build arguments, which improves readability and maintainability.
build-oscript-swarm-agent.sh (3)
4-6: Approve login check, but suggest security improvementsThe addition of the conditional check for
$DOCKER_LOGINis a good improvement. It prevents attempting to log in with empty credentials, which could cause errors. However, there are a few suggestions to enhance security and robustness:
- Consider checking all required variables (
DOCKER_LOGIN,DOCKER_PASSWORD,DOCKER_REGISTRY_URL) before attempting to log in.- Use double quotes around variable expansions to prevent word splitting and glob expansion.
- Consider using
${DOCKER_LOGIN:+x}to check if the variable is set and not empty.Here's a suggested improvement:
if [ -n "${DOCKER_LOGIN}" ] && [ -n "${DOCKER_PASSWORD}" ] && [ -n "${DOCKER_REGISTRY_URL}" ]; then docker login -u "${DOCKER_LOGIN}" -p "${DOCKER_PASSWORD}" "${DOCKER_REGISTRY_URL}" else echo "Skipping Docker login due to missing credentials" fiThis change ensures all required variables are set, uses proper quoting, and provides feedback when login is skipped.
Line range hint
1-43: Overall script review summaryThe changes to this script improve its robustness by adding a check before Docker login. However, there are opportunities for further enhancements:
- Improve the security and completeness of the Docker login check.
- Resolve the potential issue with the undefined
$no_cache_argvariable.- Consider adding more error handling and logging throughout the script.
These improvements will make the script more reliable and maintainable.
Consider breaking down this script into functions for better readability and maintainability. For example:
#!/bin/bash set -e perform_docker_login() { # Docker login logic here } handle_system_prune() { # System prune logic here } build_and_push_images() { # Docker build and push logic here } main() { perform_docker_login handle_system_prune build_and_push_images } mainThis structure would make the script easier to understand and modify in the future.
Potential issue with undefined
no_cache_argvariableThe variable
$no_cache_argis used in multiple scripts but is not defined anywhere in the repository. This may lead to unexpected behavior during the build process.
build-server.sh:19build-edt-swarm-agent.sh:22build-edt-k8s-agent.sh:22build-oscript-k8s-agent.sh:19build-oscript-k8s-agent.sh:28build-oscript-swarm-agent.sh:19build-oscript-swarm-agent.sh:28build-base-swarm-jenkins-agent.sh:19build-base-k8s-jenkins-agent.sh:19Consider defining the
no_cache_argvariable or replacing$no_cache_argwith$last_argif they serve the same purpose.🔗 Analysis chain
Line range hint
1-43: Potential issue with undefined variableWhile reviewing the entire file, I noticed that the
$no_cache_argvariable is used in thedocker buildcommands, but it's not defined in the visible part of the script. This could lead to unexpected behavior.To verify this, please run the following script:
If the variable is indeed undefined, consider replacing
$no_cache_argwith$last_argin thedocker buildcommands, aslast_argseems to serve the same purpose.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the definition and usage of no_cache_arg variable # Test 1: Search for the definition of no_cache_arg echo "Searching for no_cache_arg definition:" rg --type bash -n 'no_cache_arg=' # Test 2: Search for the usage of no_cache_arg echo "Searching for no_cache_arg usage:" rg --type bash -n '\$no_cache_arg' # Expected results: # - If no_cache_arg is properly defined, Test 1 should show its definition. # - Test 2 should show the lines where it's used in docker build commands. # - If Test 1 shows no results but Test 2 does, it indicates the variable is used without being defined.Length of output: 292
Script:
#!/bin/bash # Description: Check for the definition and usage of no_cache_arg variable # Test 1: Search for the definition of no_cache_arg echo "Searching for no_cache_arg definition:" rg --type sh -n 'no_cache_arg=' # Test 2: Search for the usage of no_cache_arg echo "Searching for no_cache_arg usage:" rg --type sh -n '\$no_cache_arg' # Expected results: # - If no_cache_arg is properly defined, Test 1 should show its definition. # - Test 2 should show the lines where it's used in docker build commands. # - If Test 1 shows no results but Test 2 does, it indicates the variable is used without being defined.Length of output: 679
build-crs.sh (1)
Line range hint
1-38: Overall assessment of changesThe addition of the Docker login check is a step in the right direction, improving the script's robustness. However, there are several areas where the script can be further enhanced:
- Improve the Docker login conditional check for better error handling and security.
- Enhance the security of the Docker login process by using more secure methods for password handling.
- Adopt additional shell scripting best practices for increased reliability and maintainability.
These improvements will make the script more secure, robust, and easier to maintain. Despite these suggestions, the core functionality of building and pushing Docker images remains intact and should work as intended.
Consider breaking down this script into smaller, focused scripts or functions for better modularity and easier testing. For example, you could have separate functions for Docker login, system pruning, and building/pushing each image.
build-server.sh (3)
4-6: Improve the condition for Docker loginThe addition of a check before executing the Docker login command is a good practice. However, the current condition can be improved for better robustness.
Consider modifying the condition as follows:
-if [ $DOCKER_LOGIN != '' ] ; then +if [ -n "$DOCKER_LOGIN" ] ; then docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL fiThis change will correctly handle cases where
$DOCKER_LOGINis unset or set to an empty string.
Line range hint
21-21: Fix potential variable name mismatchThere's a potential issue with the variable name used in the Docker build command.
The script uses
$no_cache_arg, which is not defined. It seems this should be$last_arg, which is set earlier in the script based on the$NO_CACHEvalue. Please verify and correct this:docker build \ --pull \ - $no_cache_arg \ + $last_arg \ --build-arg DOCKER_REGISTRY_URL=library \ --build-arg BASE_IMAGE=ubuntu \ --build-arg BASE_TAG=20.04 \ --build-arg ONESCRIPT_PACKAGES="yard" \ -t $DOCKER_REGISTRY_URL/oscript-downloader:latest \ -f oscript/Dockerfile \ $last_argNote that
$last_argis also used at the end of the command. You may want to review if this is the intended behavior or if it should be removed from one of these positions.
Line range hint
1-38: Enhance script robustness and flexibilityWhile the script functions as intended, there are a few areas where it could be improved:
- Add checks for required environment variables at the beginning of the script. For example:
required_vars="DOCKER_LOGIN DOCKER_PASSWORD DOCKER_REGISTRY_URL ONEC_USERNAME ONEC_PASSWORD ONEC_VERSION" for var in $required_vars; do if [ -z "${!var}" ]; then echo "Error: $var is not set" exit 1 fi done
- Consider making some of the hardcoded values configurable via environment variables or command-line arguments. For instance:
BASE_IMAGE=${BASE_IMAGE:-ubuntu} BASE_TAG=${BASE_TAG:-20.04} ONESCRIPT_PACKAGES=${ONESCRIPT_PACKAGES:-"yard"}These changes would make the script more robust and flexible, allowing for easier configuration and reducing the risk of running with missing or incorrect values.
.github/workflows/build.yml (1)
Line range hint
1-9: Consider expanding trigger events for the workflowThe workflow name "Build Docker Images" accurately describes its purpose, which is good. The current trigger events are appropriate for a feature branch. However, consider the following suggestions:
- Include the main branch in the trigger events if these Docker images should be built for main branch changes as well.
- Add a trigger for tags or releases if you want to build images for specific versions.
You might want to update the trigger events like this:
on: push: branches: [ main, feature/first-bit ] pull_request: branches: [ main, feature/first-bit ] release: types: [published] workflow_dispatch:This would ensure the images are built for main branch changes, pull requests to main, and when a new release is published, while still allowing manual triggers.
🧰 Tools
🪛 actionlint
16-16: duplicate value "build-base-k8s-jenkins-agent.sh" is found in matrix "script". the same value is at line:16,col:19
(matrix)
build-edt-swarm-agent.sh (3)
4-6: Approve with suggestions: Improve robustness of DOCKER_LOGIN checkThe addition of the DOCKER_LOGIN check is a good improvement to prevent empty login attempts. However, we can further enhance its robustness:
- Use double quotes around variable expansions to handle potential spaces or special characters.
- Use the
-ntest to check for non-empty strings, which also accounts for unset variables.Consider applying this diff for improved robustness:
-if [ $DOCKER_LOGIN != '' ] ; then - docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL +if [ -n "$DOCKER_LOGIN" ] ; then + docker login -u "$DOCKER_LOGIN" -p "$DOCKER_PASSWORD" "$DOCKER_REGISTRY_URL" fi
Line range hint
23-23: Fix inconsistent variable naming for NO_CACHE optionThere's an inconsistency between the variable definition and its usage for the NO_CACHE option. The variable is defined as
last_argbut used asno_cache_argin the docker build command. This will cause the NO_CACHE option to not work as intended.Apply this diff to fix the inconsistency:
- $no_cache_arg \ + $last_arg \Alternatively, if the intention was to use a separate variable for the no-cache option, update the variable definition earlier in the script:
-last_arg='.' -if [ $NO_CACHE = 'true' ] ; then - last_arg='--no-cache .' +no_cache_arg='' +if [ "$NO_CACHE" = 'true' ] ; then + no_cache_arg='--no-cache' fiThis second option allows for more flexibility in how the no-cache option is applied.
Line range hint
1-58: Improve overall script robustnessWhile reviewing the entire script, I noticed a few areas where we can improve robustness and consistency:
- Quote environment variables to prevent word splitting and glob expansion issues.
- Use consistent indentation (spaces vs tabs).
- Remove unnecessary $last_arg usage at the end of docker build commands.
Consider applying these changes throughout the script:
- Quote environment variables:
-if [ $DOCKER_SYSTEM_PRUNE = 'true' ] ; then +if [ "$DOCKER_SYSTEM_PRUNE" = 'true' ] ; then
- Use consistent indentation (preferably spaces):
- last_arg='--no-cache .' + last_arg='--no-cache .'
- Remove unnecessary $last_arg usage:
docker build \ --build-arg DOCKER_REGISTRY_URL=$DOCKER_REGISTRY_URL \ --build-arg BASE_IMAGE=edt \ --build-arg BASE_TAG=$edt_escaped \ - -t $DOCKER_REGISTRY_URL/edt-agent:$edt_escaped \ - -f swarm-jenkins-agent/Dockerfile \ - $last_arg + -t "$DOCKER_REGISTRY_URL/edt-agent:$edt_escaped" \ + -f swarm-jenkins-agent/Dockerfile .Apply similar changes to other docker build commands in the script.
build-edt-k8s-agent.sh (1)
4-6: Good addition, but consider improving robustnessThe new conditional block for Docker login is a good practice to prevent unnecessary login attempts. However, consider the following improvements:
- Use
[[ -n "${DOCKER_LOGIN}" ]]instead of$DOCKER_LOGIN != ''to properly handle cases where the variable is unset.- Add error handling for the login command.
Here's a suggested improvement:
-if [ $DOCKER_LOGIN != '' ] ; then - docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL +if [[ -n "${DOCKER_LOGIN}" ]]; then + if ! docker login -u "${DOCKER_LOGIN}" -p "${DOCKER_PASSWORD}" "${DOCKER_REGISTRY_URL}"; then + echo "Docker login failed" >&2 + exit 1 + fi fiThis change will:
- Properly handle cases where DOCKER_LOGIN is unset
- Exit the script if the login fails, preventing further errors
- Use double quotes around variables to prevent word splitting and globbing
build-base-swarm-jenkins-agent.sh (3)
4-6: Approve conditional Docker login with suggestion for error handlingThe addition of the conditional check for
DOCKER_LOGINis a good improvement. It prevents unnecessary login attempts when no credentials are provided.Consider adding error handling for the Docker login process. Here's a suggested improvement:
if [ $DOCKER_LOGIN != '' ] ; then - docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL + if ! docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL; then + echo "Docker login failed" + exit 1 + fi fiThis change will cause the script to exit if the Docker login fails, preventing further errors in subsequent steps.
Line range hint
13-16: Improve variable naming and definition for cache handlingThe conditional handling of the
NO_CACHEoption is a good addition. However, there are a few improvements that can be made:
- The variable name
last_argis not descriptive of its purpose.- The
$no_cache_argvariable is used in Docker build commands but is not defined in this script.Consider the following improvements:
-last_arg='.' -if [ $NO_CACHE = 'true' ] ; then - last_arg='--no-cache .' +build_args='.' +no_cache_arg='' +if [ "$NO_CACHE" = 'true' ] ; then + no_cache_arg='--no-cache' + build_args="$no_cache_arg ." fiThen, update all
docker buildcommands to use$build_argsinstead of$last_arg:docker build \ --pull \ - $no_cache_arg \ --build-arg DOCKER_REGISTRY_URL=library \ --build-arg BASE_IMAGE=ubuntu \ --build-arg BASE_TAG=20.04 \ --build-arg ONESCRIPT_PACKAGES="yard" \ -t $DOCKER_REGISTRY_URL/oscript-downloader:latest \ -f oscript/Dockerfile \ - $last_arg + $build_argsThese changes improve clarity and ensure that all variables used are properly defined.
Line range hint
1-93: Suggest overall improvements to enhance script robustness and maintainabilityWhile the script functions well for building and pushing multiple Docker images, there are several improvements that could enhance its robustness and maintainability:
- Add comprehensive error handling and logging.
- Consider breaking down the script into functions for better readability and maintainability.
- Implement a dry-run mode for testing purposes.
Here's a high-level suggestion for restructuring the script:
#!/bin/bash set -euo pipefail log() { echo "[$(date +'%Y-%m-%d %H:%M:%S')] $*" >&2 } docker_login() { if [ "${DOCKER_LOGIN:-}" != '' ] ; then log "Logging into Docker registry" if ! docker login -u "$DOCKER_LOGIN" -p "$DOCKER_PASSWORD" "$DOCKER_REGISTRY_URL"; then log "Docker login failed" exit 1 fi else log "Skipping Docker login" fi } docker_system_prune() { if [ "${DOCKER_SYSTEM_PRUNE:-false}" = 'true' ] ; then log "Pruning Docker system" docker system prune -af fi } build_and_push_image() { local image_name="$1" shift log "Building image: $image_name" if ! docker build "$@" -t "$image_name"; then log "Failed to build image: $image_name" return 1 fi if [ "${DRY_RUN:-false}" != 'true' ]; then log "Pushing image: $image_name" if ! docker push "$image_name"; then log "Failed to push image: $image_name" return 1 fi else log "[DRY RUN] Would push image: $image_name" fi } main() { docker_login docker_system_prune local build_args='.' local no_cache_arg='' if [ "${NO_CACHE:-false}" = 'true' ] ; then no_cache_arg='--no-cache' build_args="$no_cache_arg ." fi # Build and push each image build_and_push_image "$DOCKER_REGISTRY_URL/oscript-downloader:latest" \ --pull \ --build-arg DOCKER_REGISTRY_URL=library \ --build-arg BASE_IMAGE=ubuntu \ --build-arg BASE_TAG=20.04 \ --build-arg ONESCRIPT_PACKAGES="yard" \ -f oscript/Dockerfile \ $build_args # ... repeat for other images ... } main "$@"This restructured script:
- Uses functions for better organization.
- Implements proper logging.
- Adds error handling for critical operations.
- Introduces a dry-run mode for testing.
- Uses
set -euo pipefailfor stricter error checking.Remember to adapt this structure to fit all your image building and pushing operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/build.yml (2 hunks)
- build-base-k8s-jenkins-agent.sh (1 hunks)
- build-base-swarm-jenkins-agent.sh (1 hunks)
- build-crs.sh (1 hunks)
- build-edt-k8s-agent.sh (2 hunks)
- build-edt-swarm-agent.sh (1 hunks)
- build-oscript-k8s-agent.sh (1 hunks)
- build-oscript-swarm-agent.sh (1 hunks)
- build-server.sh (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yml
16-16: duplicate value "build-base-k8s-jenkins-agent.sh" is found in matrix "script". the same value is at line:16,col:19
(matrix)
🪛 yamllint
.github/workflows/build.yml
[error] 40-40: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
.github/workflows/build.yml (1)
31-34:⚠️ Potential issueImprove handling of sensitive data
The use of secrets for ONEC_USERNAME and ONEC_PASSWORD is a good security practice. However, the handling of Docker credentials needs improvement:
- DOCKER_LOGIN and DOCKER_PASSWORD are set to empty strings. This could lead to unintended behavior or security issues if these variables are used in scripts without proper checks.
Consider the following options:
If Docker authentication is required, use GitHub secrets:
DOCKER_LOGIN: ${{ secrets.DOCKER_LOGIN }} DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }}If Docker authentication is not needed, remove these variables entirely.
If these variables are used conditionally, ensure all scripts using them have proper checks to handle empty values securely.
To ensure proper handling of these variables, let's check their usage in the scripts:
Review the results to ensure these variables are used safely throughout the codebase.
✅ Verification successful
Sensitive Data Handling Verified
The usage of
DOCKER_LOGINandDOCKER_PASSWORDin the scripts is properly handled:
- Each script checks if
DOCKER_LOGINis not empty before attempting to perform Docker login using these credentials.- Docker credentials are only used when provided, ensuring that empty strings do not lead to unintended behavior or security risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DOCKER_LOGIN and DOCKER_PASSWORD usage in scripts rg -i 'DOCKER_LOGIN|DOCKER_PASSWORD' *.shLength of output: 1507
build-edt-k8s-agent.sh (2)
Line range hint
1-50: Overall, good improvements to the scriptThe changes to this script enhance its functionality and clarity. The addition of the Docker login check and the more specific build argument name are positive improvements. The suggestions provided in the previous comments will further increase the robustness and maintainability of the script.
35-35: Approve the change, but verify impact and consider new env varThe change from
DOCKER_REGISTRY_URLtoDOWNLOADER_REGISTRY_URLas a build argument name is good for clarity. However:
- Ensure this change doesn't break any expectations in the Dockerfile or downstream processes.
- Consider introducing a new environment variable
DOWNLOADER_REGISTRY_URLfor consistency between the argument name and its value.To verify the impact of this change, please run the following script:
This will help identify any potential conflicts or inconsistencies in the usage of these variables across the project.
Consider updating the environment variable usage:
- --build-arg DOWNLOADER_REGISTRY_URL=$DOCKER_REGISTRY_URL \ + --build-arg DOWNLOADER_REGISTRY_URL=${DOWNLOADER_REGISTRY_URL:-$DOCKER_REGISTRY_URL} \This change allows for a separate
DOWNLOADER_REGISTRY_URLenvironment variable while falling back toDOCKER_REGISTRY_URLif not set, maintaining backwards compatibility.✅ Verification successful
Verified: Impact of changing build argument name is acceptable
The addition of
DOWNLOADER_REGISTRY_URLas a new build argument, while retaining the usage ofDOCKER_REGISTRY_URL, does not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for uses of DOCKER_REGISTRY_URL and DOWNLOADER_REGISTRY_URL in Dockerfiles and scripts echo "Checking for DOCKER_REGISTRY_URL usage:" rg --type dockerfile --type shell 'DOCKER_REGISTRY_URL' echo "\nChecking for DOWNLOADER_REGISTRY_URL usage:" rg --type dockerfile --type shell 'DOWNLOADER_REGISTRY_URL'Length of output: 375
Script:
#!/bin/bash # Description: Corrected script to check for uses of DOCKER_REGISTRY_URL and DOWNLOADER_REGISTRY_URL in Dockerfiles and scripts echo "Checking for DOCKER_REGISTRY_URL usage:" rg --type Dockerfile --type shell 'DOCKER_REGISTRY_URL' echo "\nChecking for DOWNLOADER_REGISTRY_URL usage:" rg --type Dockerfile --type shell 'DOWNLOADER_REGISTRY_URL'Length of output: 375
Script:
#!/bin/bash # Description: Search for DOCKER_REGISTRY_URL and DOWNLOADER_REGISTRY_URL in Dockerfiles and shell scripts echo "Finding Dockerfiles and searching for DOCKER_REGISTRY_URL:" fd --type f --exact 'Dockerfile' | xargs rg 'DOCKER_REGISTRY_URL' echo "\nFinding Dockerfiles and searching for DOWNLOADER_REGISTRY_URL:" fd --type f --exact 'Dockerfile' | xargs rg 'DOWNLOADER_REGISTRY_URL' echo "\nSearching all shell scripts for DOCKER_REGISTRY_URL:" rg --type sh 'DOCKER_REGISTRY_URL' echo "\nSearching all shell scripts for DOWNLOADER_REGISTRY_URL:" rg --type sh 'DOWNLOADER_REGISTRY_URL'Length of output: 24698
build-base-k8s-jenkins-agent.sh (2)
Line range hint
1-93: Summary of review for build-base-k8s-jenkins-agent.shThe addition of the DOCKER_LOGIN check is a positive change that improves the script's robustness. However, there are several areas where the script could be further improved:
- Enhance the DOCKER_LOGIN check with better error handling and security practices.
- Apply bash best practices throughout the script (quoting variables, improving error handling).
- Make the script more configurable by using environment variables for currently hardcoded values.
- Improve version management for all Docker images built by the script.
These changes will make the script more reliable, secure, and maintainable. Please consider implementing the suggested improvements in a future iteration.
Line range hint
1-93: General script improvements and best practicesWhile the main change in this PR is the addition of the DOCKER_LOGIN check, there are several areas where the script could be improved:
- Quote variables to prevent word splitting and globbing issues.
- Define or remove the
$no_cache_argvariable.- Make hardcoded values configurable via environment variables.
- Add version tagging for the
oscript-downloaderimage.- Improve error handling for build and push commands.
Here are some suggested improvements:
Quote variables:
if [ "${DOCKER_SYSTEM_PRUNE}" = 'true' ] ; then docker system prune -af fiDefine
no_cache_arg:no_cache_arg="" if [ "${NO_CACHE}" = 'true' ] ; then no_cache_arg="--no-cache" fiMake hardcoded values configurable:
UBUNTU_VERSION="${UBUNTU_VERSION:-20.04}" ONESCRIPT_PACKAGES="${ONESCRIPT_PACKAGES:-yard}"Add version tagging for
oscript-downloader:OSCRIPT_VERSION="${OSCRIPT_VERSION:-latest}" docker build \ # ... other args ... -t "${DOCKER_REGISTRY_URL}/oscript-downloader:${OSCRIPT_VERSION}" \ -t "${DOCKER_REGISTRY_URL}/oscript-downloader:latest" \ # ... rest of command ...Improve error handling:
docker build \ # ... build args ... || { echo "Build failed for ${DOCKER_REGISTRY_URL}/onec-client:${ONEC_VERSION}"; exit 1; } docker push "${DOCKER_REGISTRY_URL}/onec-client:${ONEC_VERSION}" \ || { echo "Push failed for ${DOCKER_REGISTRY_URL}/onec-client:${ONEC_VERSION}"; exit 1; }To ensure these changes don't introduce any regressions, please run the following verification script:
#!/bin/bash # Check if required environment variables are set echo "Checking required environment variables..." required_vars=("DOCKER_REGISTRY_URL" "ONEC_USERNAME" "ONEC_PASSWORD" "ONEC_VERSION") for var in "${required_vars[@]}"; do if [[ -z "${!var}" ]]; then echo "Error: $var is not set" else echo "$var is set" fi done # Check if Docker is available echo "Checking Docker availability..." if command -v docker &> /dev/null; then echo "Docker is available" else echo "Error: Docker is not available" fi
build-crs.sh
Outdated
| @@ -1,7 +1,9 @@ | |||
| #!/bin/bash | |||
| set -e | |||
|
|
|||
| docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL | |||
| if [ $DOCKER_LOGIN != '' ] ; then | |||
| docker login -u $DOCKER_LOGIN -p $DOCKER_PASSWORD $DOCKER_REGISTRY_URL | |||
There was a problem hiding this comment.
Improve security of Docker login
Passing the Docker password directly in the command line can be a security risk as it may be visible in process lists or logs.
Consider using Docker's built-in credential helpers or passing the password through stdin. Here's an example using stdin:
echo "${DOCKER_PASSWORD}" | docker login -u "${DOCKER_LOGIN}" --password-stdin "${DOCKER_REGISTRY_URL}"This method is more secure as it doesn't expose the password in the command line.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
build-edt-k8s-agent.sh (2)
4-11: Improved Docker login process with better error handling.The addition of credential checks before attempting Docker login is a good improvement. It prevents unnecessary login attempts and provides clear feedback.
Consider adding more specific error messages to indicate which credentials are missing. This could help in troubleshooting. For example:
if [ -z "${DOCKER_LOGIN}" ]; then echo "DOCKER_LOGIN is not set" fi if [ -z "${DOCKER_PASSWORD}" ]; then echo "DOCKER_PASSWORD is not set" fi if [ -z "${DOCKER_REGISTRY_URL}" ]; then echo "DOCKER_REGISTRY_URL is not set" fi
Line range hint
1-56: Overall improvements with some points to addressThe changes to this script have generally improved its robustness and clarity. The main improvements are:
- Better error handling for Docker login.
- More specific naming for the downloader registry URL.
However, there are a few points that need attention:
- Consider adding more specific error messages for missing Docker credentials.
- Verify the consistency of
DOCKER_REGISTRY_URLandDOWNLOADER_REGISTRY_URLusage throughout the script.- Ensure that
DOWNLOADER_REGISTRY_URLis properly set if it's a new variable.Once these points are addressed, the script will be more maintainable and less prone to errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/build.yml (2 hunks)
- build-base-k8s-jenkins-agent.sh (1 hunks)
- build-base-swarm-jenkins-agent.sh (1 hunks)
- build-crs.sh (1 hunks)
- build-edt-k8s-agent.sh (2 hunks)
- build-edt-swarm-agent.sh (1 hunks)
- build-oscript-k8s-agent.sh (1 hunks)
- build-oscript-swarm-agent.sh (1 hunks)
- build-server.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/build.yml
- build-base-k8s-jenkins-agent.sh
- build-base-swarm-jenkins-agent.sh
- build-crs.sh
- build-edt-swarm-agent.sh
- build-oscript-k8s-agent.sh
- build-oscript-swarm-agent.sh
- build-server.sh
🔇 Additional comments (2)
build-edt-k8s-agent.sh (2)
Line range hint
34-45: Improved clarity with DOWNLOADER_REGISTRY_URL, but verify consistency.The change from
DOCKER_REGISTRY_URLtoDOWNLOADER_REGISTRY_URLimproves clarity by specifying the purpose of this registry URL. This is a good practice.However, please verify that this change doesn't introduce inconsistencies with other parts of the script. Consider updating all occurrences of
DOCKER_REGISTRY_URLto more specific names where appropriate, or ensure thatDOWNLOADER_REGISTRY_URLis properly set earlier in the script.Run the following script to check all occurrences of both variables:
#!/bin/bash echo "Occurrences of DOCKER_REGISTRY_URL:" grep -n "DOCKER_REGISTRY_URL" build-edt-k8s-agent.sh echo -e "\nOccurrences of DOWNLOADER_REGISTRY_URL:" grep -n "DOWNLOADER_REGISTRY_URL" build-edt-k8s-agent.shBased on the results, decide if further changes are needed for consistency.
Line range hint
13-33: Verify consistency of DOCKER_REGISTRY_URL usage.While this segment hasn't changed, it's worth noting that
DOCKER_REGISTRY_URLis used here as a build argument. However, in the second Docker build command, this has been changed toDOWNLOADER_REGISTRY_URL.Please confirm if the usage of
DOCKER_REGISTRY_URLin this build command is correct or if it should also be updated toDOWNLOADER_REGISTRY_URLfor consistency.✅ Verification successful
Verification Successful:
DOCKER_REGISTRY_URLis used consistently as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of DOCKER_REGISTRY_URL across the script grep -n "DOCKER_REGISTRY_URL" build-edt-k8s-agent.shLength of output: 951
Summary by CodeRabbit
New Features
Bug Fixes
Chores