Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Доработки для использования замеров покрытия с помощью Coverage41C на агентах swarm и k8s #44

Conversation

ovcharenko-di
Copy link

@ovcharenko-di ovcharenko-di commented Sep 29, 2024

Задумка следующая:
В .onec.env указывается версия Coverage41C. Если версия не указана, то считаем, что замер покрытия не нужен.

Если версия указана, то при сборке образов с помощью sh- и bat-скриптов предварительно будут собраны образы EDT, а в конце поверх base-jenkins-agent будет собран образ с Coverage41C и jar-никами из EDT, которые отвечают за замер (base-jenkins-coverage-agent).

Изначально была идея встраивать Coverage41C и jar-ники от EDT безусловно. Но от нее я отказался, потому что замер покрытия нужен далеко не всем, соответственно, собирать тяжелые образы EDT в таких ситуациях будет излишним.

supersedes #20

ВНИМАНИЕ!
Сборку агентов через bat-файл я не проверял.

Summary by CodeRabbit

  • New Features

    • Introduced new environment variables: COVERAGE41C_VERSION and EDT_VERSION for enhanced configuration.
    • Added functionality to build and push a coverage-specific Docker image based on the COVERAGE41C_VERSION.
    • New scripts created for building and pushing Docker images for Jenkins coverage agents.
  • Documentation

    • Updated README.md with descriptions for new environment variables and clarified Jenkins configuration instructions.
  • Chores

    • Enhanced build scripts to support the new coverage tool integration.

Copy link

coderabbitai bot commented Sep 29, 2024

Walkthrough

The pull request introduces several changes across multiple files to support the integration of a new environment variable, COVERAGE41C_VERSION, which facilitates the building of a Docker image for the Coverage41C tool. The .onec.env.example file is updated to declare this new variable, while the README.md is modified to include descriptions of both COVERAGE41C_VERSION and EDT_VERSION. The build scripts for Jenkins agents are adjusted to conditionally handle the new coverage image creation, and a new Dockerfile is added to define the multi-stage build process for the Coverage41C tool.

Changes

File(s) Change Summary
.onec.env.example Added COVERAGE41C_VERSION variable with an empty string value.
README.md Updated with new environment variable descriptions for EDT_VERSION and COVERAGE41C_VERSION. Minor formatting changes made.
build-base-k8s-jenkins-agent.sh Added logic to conditionally push the coverage agent image based on script arguments.
build-base-swarm-jenkins-agent.bat Introduced conditional logic for pushing the coverage agent image based on input parameters.
coverage41C/Dockerfile New Dockerfile created for multi-stage build of Coverage41C, including necessary components and setup.
build-base-k8s-jenkins-coverage-agent.sh New script added to build and push the Jenkins coverage agent Docker image.
build-base-swarm-jenkins-coverage-agent.bat New batch script added to automate building and pushing the Jenkins coverage agent Docker image.
build-edt-k8s-agent.sh Removed Docker login logic and simplified the build process for the EDT environment.
build-edt-swarm-agent.bat Streamlined the script by removing Docker login checks and delegating build logic to another script.
build-edt.bat New script added to automate Docker image builds based on environment variables.
build-edt.sh New script added to facilitate building Docker images for the EDT environment.

Possibly related PRs

Poem

In the burrow deep, changes take flight,
A new variable shines, oh what a sight!
With Coverage41C, our builds will expand,
Docker images ready, all perfectly planned.
Hops of joy echo, as we code and create,
In the world of Jenkins, we celebrate fate! 🐰✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 27011bd and 011626c.

📒 Files selected for processing (7)
  • build-base-k8s-jenkins-agent.sh (1 hunks)
  • build-base-k8s-jenkins-coverage-agent.sh (1 hunks)
  • build-base-swarm-jenkins-agent.bat (1 hunks)
  • build-base-swarm-jenkins-agent.sh (1 hunks)
  • build-base-swarm-jenkins-coverage-agent.bat (1 hunks)
  • build-base-swarm-jenkins-coverage-agent.sh (1 hunks)
  • build-edt.bat (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • build-base-k8s-jenkins-agent.sh
  • build-base-k8s-jenkins-coverage-agent.sh
  • build-base-swarm-jenkins-agent.bat
  • build-base-swarm-jenkins-agent.sh
  • build-base-swarm-jenkins-coverage-agent.bat
  • build-base-swarm-jenkins-coverage-agent.sh
  • build-edt.bat

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (9)
.onec.env.example (1)

5-5: LGTM! Consider adding a brief comment for clarity.

The addition of the COVERAGE41C_VERSION variable is correctly placed and formatted. It aligns well with the PR objectives for specifying the Coverage41C version.

Consider adding a brief comment above this line to explain its purpose and the implications of an empty string. For example:

+# Specify the Coverage41C version. Leave empty to disable coverage measurement.
export COVERAGE41C_VERSION=''

This addition would enhance clarity for other developers who might work with this file in the future.

coverage41C/Dockerfile (2)

1-7: ARG declarations look good, consider adding a default value for BASE_TAG.

The use of ARG instructions provides flexibility for the build process. The default values for DOCKER_REGISTRY_URL, BASE_IMAGE, EDT_VERSION, and COVERAGE41C_VERSION are sensible. The EDT_PLUGINS argument effectively uses other ARG values, maintaining consistency.

Consider adding a default value for BASE_TAG to ensure the build doesn't fail if it's not specified during the build process. For example:

-ARG BASE_TAG
+ARG BASE_TAG=latest

16-22: File operations are mostly good, but consider replacing ADD with RUN.

The COPY instruction correctly uses the --from=base flag to copy JAR files from the first stage. Combining the extraction and cleanup operations in a single RUN instruction is a good practice to reduce the final image size.

However, using ADD for downloading remote files is discouraged in Docker best practices. Consider replacing the ADD instruction with a RUN instruction using curl or wget. This allows for more control over the download process and is the recommended approach. Here's a suggested change:

-ADD https://github.com/1c-syntax/Coverage41C/releases/download/v${COVERAGE41C_VERSION}/Coverage41C-${COVERAGE41C_VERSION}.tar \
-    /opt/
-
-RUN tar xf /opt/Coverage41C-${COVERAGE41C_VERSION}.tar -C /opt/ \
-  && rm -rf /opt/Coverage41C-${COVERAGE41C_VERSION}.tar

+RUN curl -L -o /opt/Coverage41C-${COVERAGE41C_VERSION}.tar \
+    https://github.com/1c-syntax/Coverage41C/releases/download/v${COVERAGE41C_VERSION}/Coverage41C-${COVERAGE41C_VERSION}.tar \
+    && tar xf /opt/Coverage41C-${COVERAGE41C_VERSION}.tar -C /opt/ \
+    && rm -rf /opt/Coverage41C-${COVERAGE41C_VERSION}.tar

This change combines the download, extraction, and cleanup in a single RUN instruction, which is more efficient and follows Docker best practices.

build-base-k8s-jenkins-agent.sh (2)

15-19: LGTM with a minor suggestion for improvement.

The conditional check for COVERAGE41C_VERSION is correctly implemented and aligns with the PR objectives. It ensures that EDT images are only built when necessary, which is an efficient approach.

Consider adding a check to ensure that build-edt-swarm-agent.sh exists and is executable before attempting to run it. This can prevent potential errors if the script is missing or lacks execute permissions. Here's a suggested improvement:

 # если устанавливаем Coverage41C, то предварительно нужно собрать образы EDT
 if [[ $COVERAGE41C_VERSION != "" ]] ; then
+  if [[ -x "./build-edt-swarm-agent.sh" ]]; then
     ./build-edt-swarm-agent.sh
+  else
+    echo "Error: build-edt-swarm-agent.sh not found or not executable" >&2
+    exit 1
+  fi
 fi

This change will make the script more robust and provide a clear error message if the required script is missing or not executable.


89-102: LGTM with suggestions for improved error handling and logging.

The implementation for building and pushing the coverage agent image is correct and aligns with the PR objectives. It properly checks for the COVERAGE41C_VERSION and includes all necessary build arguments.

Consider adding error handling and logging to make the script more robust and informative. Here's a suggested improvement:

 if [[ $COVERAGE41C_VERSION != "" ]] ; then
+    echo "Building coverage agent image..."
     docker build \
        --build-arg DOCKER_REGISTRY_URL=$DOCKER_REGISTRY_URL \
        --build-arg BASE_IMAGE=base-jenkins-agent \
        --build-arg BASE_TAG=$ONEC_VERSION \
        --build-arg EDT_VERSION=$EDT_VERSION \
        --build-arg COVERAGE41C_VERSION=$COVERAGE41C_VERSION \
        -t $DOCKER_REGISTRY_URL/base-jenkins-coverage-agent:$ONEC_VERSION \
        -f coverage41C/Dockerfile \
        $last_arg
+    if [ $? -ne 0 ]; then
+        echo "Error: Failed to build coverage agent image" >&2
+        exit 1
+    fi

+    echo "Pushing coverage agent image..."
     docker push $DOCKER_REGISTRY_URL/base-jenkins-coverage-agent:$ONEC_VERSION
+    if [ $? -ne 0 ]; then
+        echo "Error: Failed to push coverage agent image" >&2
+        exit 1
+    fi
+    echo "Coverage agent image built and pushed successfully."
 fi

These changes will provide better feedback during the build process and ensure that the script exits with an error if either the build or push operations fail.

build-base-swarm-jenkins-agent.sh (2)

15-19: LGTM! Consider adding error handling.

The added logic aligns well with the PR objectives, conditionally building EDT images when COVERAGE41C_VERSION is set. This approach ensures that EDT images are only built when necessary for coverage measurement.

Consider adding error handling for the script execution. For example:

 if [[ $COVERAGE41C_VERSION != "" ]] ; then
-  ./build-edt-swarm-agent.sh
+  if ! ./build-edt-swarm-agent.sh; then
+    echo "Error: Failed to build EDT swarm agent"
+    exit 1
+  fi
 fi

This change will ensure that any failures in building the EDT images are caught and reported, preventing the script from continuing with potentially missing dependencies.


90-102: LGTM! Consider adding error handling and improving variable quoting.

The added logic for building and pushing the coverage agent image aligns well with the PR objectives. It correctly uses the COVERAGE41C_VERSION to conditionally create the image and includes all necessary build arguments.

Consider the following improvements:

  1. Add error handling for Docker build and push commands.
  2. Use double quotes around variable expansions to prevent word splitting and globbing.

Here's an example of how you could implement these suggestions:

 if [[ $COVERAGE41C_VERSION != "" ]] ; then
-    docker build \
-       --build-arg DOCKER_REGISTRY_URL=$DOCKER_REGISTRY_URL \
-       --build-arg BASE_IMAGE=base-jenkins-agent \
-       --build-arg BASE_TAG=$ONEC_VERSION \
-       --build-arg EDT_VERSION=$EDT_VERSION \
-       --build-arg COVERAGE41C_VERSION=$COVERAGE41C_VERSION \
-       -t $DOCKER_REGISTRY_URL/base-jenkins-coverage-agent:$ONEC_VERSION \
-       -f coverage41C/Dockerfile \
-       $last_arg
+    if ! docker build \
+       --build-arg DOCKER_REGISTRY_URL="$DOCKER_REGISTRY_URL" \
+       --build-arg BASE_IMAGE=base-jenkins-agent \
+       --build-arg BASE_TAG="$ONEC_VERSION" \
+       --build-arg EDT_VERSION="$EDT_VERSION" \
+       --build-arg COVERAGE41C_VERSION="$COVERAGE41C_VERSION" \
+       -t "$DOCKER_REGISTRY_URL/base-jenkins-coverage-agent:$ONEC_VERSION" \
+       -f coverage41C/Dockerfile \
+       $last_arg; then
+        echo "Error: Failed to build base-jenkins-coverage-agent image"
+        exit 1
+    fi

-    docker push $DOCKER_REGISTRY_URL/base-jenkins-coverage-agent:$ONEC_VERSION
+    if ! docker push "$DOCKER_REGISTRY_URL/base-jenkins-coverage-agent:$ONEC_VERSION"; then
+        echo "Error: Failed to push base-jenkins-coverage-agent image"
+        exit 1
+    fi
 fi

These changes will make the script more robust by catching and reporting any failures in the Docker build and push processes, as well as preventing potential issues with variable expansion.

README.md (2)

49-49: LGTM! Consider minor formatting adjustment.

The addition of the EDT_VERSION environment variable is clear and aligns with the PR objectives. It provides necessary information for users building images with EDT or using coverage measurements.

For consistency with the rest of the list, consider changing the asterisk to a dash:

-* EDT_VERSION - версия EDT. Обязательно заполнять только при сборке образов с EDT или при использовании замеров покрытия (см. `COVERAGE41C_VERSION`)
+- EDT_VERSION - версия EDT. Обязательно заполнять только при сборке образов с EDT или при использовании замеров покрытия (см. `COVERAGE41C_VERSION`)
🧰 Tools
🪛 Markdownlint

49-49: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


51-52: LGTM! Consider formatting and structure improvements.

The addition of the COVERAGE41C_VERSION environment variable is well-explained and aligns with the PR objectives. It provides crucial information for users intending to use coverage measurements with Coverage41C.

Consider the following improvements for clarity and consistency:

  1. Change the asterisk to a dash for list consistency.
  2. Merge the explanation into a single list item for better visibility.

Here's a suggested revision:

-* COVERAGE41C_VERSION - версия Coverage41C
-Если заполнено, то при выполнении скриптов `build-base-*-agent.*` будет также собран образ `base-jenkins-coverage-agent`, подготовленный для сбора замеров покрытия с помощью Coverage41C. Требуется также указать значение для переменной EDT_VERSION.
+- COVERAGE41C_VERSION - версия Coverage41C. Если заполнено, то при выполнении скриптов `build-base-*-agent.*` будет также собран образ `base-jenkins-coverage-agent`, подготовленный для сбора замеров покрытия с помощью Coverage41C. Требуется также указать значение для переменной EDT_VERSION.
🧰 Tools
🪛 Markdownlint

51-51: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b1e71b6 and bb3a5a6.

📒 Files selected for processing (6)
  • .onec.env.example (1 hunks)
  • README.md (4 hunks)
  • build-base-k8s-jenkins-agent.sh (2 hunks)
  • build-base-swarm-jenkins-agent.bat (2 hunks)
  • build-base-swarm-jenkins-agent.sh (2 hunks)
  • coverage41C/Dockerfile (1 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md

49-49: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


50-50: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


51-51: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)

🔇 Additional comments (7)
coverage41C/Dockerfile (3)

8-10: Multi-stage build setup looks good.

The use of a multi-stage build is a good practice for optimizing the final image size. The flexible use of ARG values in the FROM instructions allows for easy customization of the base images.


11-14: LABEL and ARG instructions are correctly implemented.

The LABEL instruction follows best practices by providing maintainer information. The repetition of ARG instructions (COVERAGE41C_VERSION and EDT_PLUGINS) in this build stage is necessary and correct, as ARG values don't persist across build stages in Dockerfiles.


24-25: ENV instructions are well-implemented.

Setting EDT_LOCATION as an environment variable allows for easier configuration in dependent applications. Updating the PATH to include the Coverage41C binary location is a good practice, making the tool easily accessible. The use of the COVERAGE41C_VERSION variable in the PATH ensures consistency with the downloaded version.

build-base-k8s-jenkins-agent.sh (1)

Line range hint 1-102: Overall, the changes look good and align with the PR objectives.

The script has been successfully modified to conditionally build and push a coverage agent image when COVERAGE41C_VERSION is set. This aligns with the goal of avoiding unnecessary builds and integrating Coverage41C only when needed.

To ensure that the changes are working as expected, please run the following verification script:

This script will help verify that the key components of the changes are in place and that the script is handling the COVERAGE41C_VERSION variable correctly.

build-base-swarm-jenkins-agent.sh (1)

Line range hint 1-102: Overall, the changes align well with the PR objectives.

The modifications to this script successfully implement the conditional building of EDT images and the creation of a coverage-specific Docker image based on the COVERAGE41C_VERSION variable. This approach ensures that coverage measurement tools are only included when necessary, avoiding unnecessary builds of large EDT images for users who don't require this functionality.

The script now effectively:

  1. Builds EDT images when needed for coverage measurement.
  2. Creates a new base-jenkins-coverage-agent image incorporating Coverage41C and necessary EDT jar files.

These changes directly support the main objectives outlined in the PR summary. The suggested improvements for error handling and variable quoting will further enhance the script's reliability and robustness.

README.md (2)

89-89: LGTM! Improved formatting.

The addition of a hyphen at the beginning of the line improves the formatting consistency and makes the Jenkins configuration instructions more visually distinct.


Line range hint 1-224: Overall, the changes to README.md are well-implemented and valuable.

The additions of EDT_VERSION and COVERAGE41C_VERSION environment variables, along with their descriptions, align well with the PR objectives. These changes provide crucial information for users working with EDT and Coverage41C.

The minor formatting suggestions, if implemented, will further enhance the readability and consistency of the document. Great job on improving the documentation!

🧰 Tools
🪛 Markdownlint

46-46: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


47-47: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


48-48: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


49-49: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


50-50: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


51-51: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


46-46: null
Bare URL used

(MD034, no-bare-urls)


47-47: null
Bare URL used

(MD034, no-bare-urls)

@@ -10,6 +10,8 @@ if %ERRORLEVEL% neq 0 goto end

if %NO_CACHE%=="true" (SET last_arg="--no-cache .") else (SET last_arg=".")

# если устанавливаем Coverage41C, то предварительно нужно собрать образы EDT
if %COVERAGE41C_VERSION% neq "" (.\build-edt-swarm-agent.bat)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct IF statement syntax for checking variable in batch scripts

The current syntax if %COVERAGE41C_VERSION% neq "" (.\build-edt-swarm-agent.bat) may not properly check if %COVERAGE41C_VERSION% is not empty. In batch scripts, using neq for string comparison can lead to unexpected results. It's recommended to use IF NOT "%variable%"=="" or IF DEFINED variable.

Apply this diff to fix the IF statement:

-if %COVERAGE41C_VERSION% neq "" (.\build-edt-swarm-agent.bat)
+if NOT "%COVERAGE41C_VERSION%"=="" (.\build-edt-swarm-agent.bat)
📝 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 %COVERAGE41C_VERSION% neq "" (.\build-edt-swarm-agent.bat)
if NOT "%COVERAGE41C_VERSION%"=="" (.\build-edt-swarm-agent.bat)

Comment on lines 104 to 119
if %COVERAGE41C_VERSION% neq "" (

docker build ^
--build-arg DOCKER_REGISTRY_URL=%DOCKER_REGISTRY_URL% ^
--build-arg BASE_IMAGE=base-jenkins-agent ^
--build-arg BASE_TAG=%ONEC_VERSION% ^
--build-arg EDT_VERSION=%EDT_VERSION% ^
--build-arg COVERAGE41C_VERSION=%COVERAGE41C_VERSION% ^
-t %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%ONEC_VERSION% ^
-f coverage41C/Dockerfile ^
%last_arg%


docker push %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%ONEC_VERSION%

)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct IF statement syntax for checking variable in batch scripts

Similar to line 14, the IF statement at line 104 may not correctly check if %COVERAGE41C_VERSION% is not empty. Using neq for string comparison may not function as intended.

Apply this diff to fix the IF statement:

-if %COVERAGE41C_VERSION% neq "" (
+if NOT "%COVERAGE41C_VERSION%"=="" (
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if %COVERAGE41C_VERSION% neq "" (
docker build ^
--build-arg DOCKER_REGISTRY_URL=%DOCKER_REGISTRY_URL% ^
--build-arg BASE_IMAGE=base-jenkins-agent ^
--build-arg BASE_TAG=%ONEC_VERSION% ^
--build-arg EDT_VERSION=%EDT_VERSION% ^
--build-arg COVERAGE41C_VERSION=%COVERAGE41C_VERSION% ^
-t %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%ONEC_VERSION% ^
-f coverage41C/Dockerfile ^
%last_arg%
docker push %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%ONEC_VERSION%
)
if NOT "%COVERAGE41C_VERSION%"=="" (
docker build ^
--build-arg DOCKER_REGISTRY_URL=%DOCKER_REGISTRY_URL% ^
--build-arg BASE_IMAGE=base-jenkins-agent ^
--build-arg BASE_TAG=%ONEC_VERSION% ^
--build-arg EDT_VERSION=%EDT_VERSION% ^
--build-arg COVERAGE41C_VERSION=%COVERAGE41C_VERSION% ^
-t %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%ONEC_VERSION% ^
-f coverage41C/Dockerfile ^
%last_arg%
docker push %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%ONEC_VERSION%
)

@nixel2007
Copy link
Member

  • ребейзни как будет время, плз

@nixel2007 nixel2007 closed this Oct 2, 2024
@nixel2007 nixel2007 reopened this Oct 2, 2024
@Segate-ekb
Copy link

Segate-ekb commented Oct 2, 2024

А вы не хотите вообще вынести кавередж в отдельный образ?

Зачем городить какие-то условия и завязываться на файл окружения, когда можно просто взять и сделать рядом sh который берет и собирает тот же onec-client + файлы из edt отдельным слоем.

Так, если образы едт и клиента уже будут собраны - то он возьмет их из кэша, а если нет - то соберет так как надо?

т.е. будет даунлоадер, потом едт потом клиент потом кавередж. Кому надо запустил sh и поехали

@nixel2007
Copy link
Member

Так, если образы едт и клиента уже будут собраны - то он возьмет их из кэша, а если нет - то соберет так как надо?

Кто ж образы с кэшэм собирает? :)

Но вообще да, отдельный скрипт - проще всего. Но опять же, я бы предусмотрел в нем флаг для опциональной сборки. Если передано не собирать, то пускай оно с --pull из реджистри заберётся.

@nixel2007 nixel2007 closed this Oct 3, 2024
@nixel2007 nixel2007 reopened this Oct 3, 2024
@nixel2007 nixel2007 closed this Oct 3, 2024
@nixel2007 nixel2007 reopened this Oct 3, 2024
@nixel2007 nixel2007 closed this Oct 3, 2024
@nixel2007 nixel2007 reopened this Oct 3, 2024
@nixel2007 nixel2007 closed this Oct 3, 2024
@nixel2007 nixel2007 reopened this Oct 3, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 25

🧹 Outside diff range and nitpick comments (22)
build-edt-k8s-agent.sh (3)

Line range hint 3-8: Add environment variable validation and proper quoting

The Docker build command uses environment variables without validation and contains unquoted variables that could cause issues with special characters.

Apply this diff to add validation and proper quoting:

+if [ -z "$DOCKER_REGISTRY_URL" ] || [ -z "$edt_escaped" ]; then
+    echo "Error: Required environment variables DOCKER_REGISTRY_URL and edt_escaped must be set"
+    exit 1
+fi
+
 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 \
+    --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 k8s-jenkins-agent/Dockerfile \
     $last_arg
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


Line range hint 10-10: Add error handling and proper quoting for docker push

The Docker push command lacks error handling and contains unquoted variables.

Apply this diff to improve the push command:

-docker push $DOCKER_REGISTRY_URL/edt-agent:$edt_escaped
+if ! docker push "${DOCKER_REGISTRY_URL}/edt-agent:${edt_escaped}"; then
+    echo "Error: Failed to push the Docker image"
+    exit 1
+fi
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


Line range hint 1-10: Consider adding logging and documentation

The script would benefit from improved logging and documentation to help with debugging and maintenance.

Apply this diff to add logging and documentation:

 #!/bin/bash
+
+# Build and push EDT agent Docker image for Kubernetes
+# Required environment variables:
+# - DOCKER_REGISTRY_URL: URL of the Docker registry
+# - edt_escaped: EDT version tag
+
 set -e
 
+echo "Starting EDT Kubernetes agent build process..."
+
 if [ ! -x "./build-edt.sh" ]; then
     echo "Error: build-edt.sh not found or not executable"
     exit 1
 fi
 
+echo "Building base EDT image..."
 ./build-edt.sh
 
+echo "Building EDT Kubernetes agent image..."
 if [ -z "$DOCKER_REGISTRY_URL" ] || [ -z "$edt_escaped" ]; then
     echo "Error: Required environment variables DOCKER_REGISTRY_URL and edt_escaped must be set"
     exit 1
 fi
 
 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 k8s-jenkins-agent/Dockerfile \
     $last_arg
 
+echo "Pushing EDT Kubernetes agent image to registry..."
 if ! docker push "${DOCKER_REGISTRY_URL}/edt-agent:${edt_escaped}"; then
     echo "Error: Failed to push the Docker image"
     exit 1
 fi
+
+echo "Build process completed successfully"
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

build-edt-swarm-agent.sh (3)

Line range hint 4-11: Add environment variable validation and improve error handling

The Docker build command uses several undefined or unvalidated variables.

Apply this diff to add proper validation:

+# Validate required environment variables
+required_vars=("DOCKER_REGISTRY_URL" "edt_escaped")
+for var in "${required_vars[@]}"; do
+    if [ -z "${!var}" ]; then
+        echo "Error: $var is not set"
+        exit 1
+    fi
+done
+
+# Set default for last_arg if not provided
+last_arg=${last_arg:-.}
+
 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 \
+    -f swarm-jenkins-agent/Dockerfile \
     $last_arg
+
+if [ $? -ne 0 ]; then
+    echo "Error: Docker build failed"
+    exit 1
+fi
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


Line range hint 13-13: Add error handling for Docker push

The Docker push command lacks error handling and validation.

Apply this diff to improve error handling:

+echo "Pushing image to registry..."
 docker push $DOCKER_REGISTRY_URL/edt-agent:$edt_escaped
+if [ $? -ne 0 ]; then
+    echo "Error: Failed to push image to registry"
+    exit 1
+fi
+echo "Successfully pushed image to registry"
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)


Line range hint 1-13: Add script documentation and usage information

The script lacks documentation about its purpose, requirements, and usage.

Add this documentation at the beginning of the script:

+#!/bin/bash
+
+# build-edt-swarm-agent.sh
+# Builds and pushes EDT agent Docker image for swarm deployment
+#
+# Requirements:
+# - Docker daemon running
+# - build-edt.sh in the same directory
+# - Environment variables:
+#   DOCKER_REGISTRY_URL: URL of the Docker registry
+#   edt_escaped: EDT version tag (escaped)
+#
+# Optional:
+#   last_arg: Docker build context (defaults to current directory)
+
 # Rest of the script...
🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

build-edt-swarm-agent.bat (2)

Line range hint 5-11: Validate required environment variables

The script uses environment variables (DOCKER_REGISTRY_URL, edt_escaped) without validation. Missing or empty variables could cause cryptic Docker build errors.

Add environment variable validation at the start of the script:

 @echo off
 
+if "%DOCKER_REGISTRY_URL%"=="" (
+    echo Error: DOCKER_REGISTRY_URL environment variable is not set
+    goto end
+)
+
+if "%edt_escaped%"=="" (
+    echo Error: edt_escaped environment variable is not set
+    goto end
+)

Line range hint 19-20: Enhance error reporting

The current error handling only shows "End of program" which doesn't indicate success or failure.

Improve error reporting:

 :end
-echo End of program.
+if %ERRORLEVEL% neq 0 (
+    echo Build failed with error code %ERRORLEVEL%
+    exit /b %ERRORLEVEL%
+) else (
+    echo Build completed successfully
+    exit /b 0
+)
build-base-swarm-jenkins-coverage-agent.bat (1)

1-27: Consider architectural improvements for better maintainability and efficiency.

Based on the PR discussion:

  1. Consider separating coverage functionality into a dedicated image as suggested by Segate-ekb
  2. Implement build caching strategy to optimize build times
  3. Add a flag for optional builds to support pulling pre-built images from registry

Benefits:

  • Better separation of concerns
  • Improved build performance through caching
  • More flexibility for users

Would you like assistance in implementing these architectural improvements?

build-base-k8s-jenkins-coverage-agent.sh (3)

1-2: Add -u flag for undefined variable detection

Consider adding the -u flag to catch undefined variables, which is particularly important for scripts relying on environment variables.

 #!/usr/bin/env bash
-set -eo pipefail
+set -euo pipefail

17-20: Improve build argument handling

The current string concatenation for --no-cache . could be problematic. Consider using an array for better argument handling.

-last_arg='.'
+build_args=(".")
 if [ "${NO_CACHE}" = 'true' ] ; then
-    last_arg='--no-cache .'
+    build_args=("--no-cache" ".")
 fi

Then update line 33 to use: `"${build_args[@]}"


1-35: Consider separating coverage functionality into a dedicated image

As suggested in the PR discussion by Segate-ekb, consider restructuring this into a separate image build process. This approach could:

  1. Improve build caching
  2. Reduce complexity in the main build process
  3. Allow for more flexible deployment options

This could be implemented by:

  1. Creating a separate Dockerfile specifically for coverage functionality
  2. Using multi-stage builds to optimize the image size
  3. Implementing a separate build script that can be called optionally
build-base-swarm-jenkins-coverage-agent.sh (1)

17-20: Improve build argument handling

The current implementation concatenates the --no-cache flag with the dot, which could lead to parsing issues. It's clearer and more robust to handle them separately.

-last_arg='.'
+build_args='.'
+no_cache=''
 if [ "${NO_CACHE}" = 'true' ] ; then
-    last_arg='--no-cache .'
+    no_cache='--no-cache'
 fi

Then update line 33 to:

-   $last_arg
+   $no_cache $build_args
build-edt.bat (2)

27-35: Consider making build arguments configurable

The first Docker build uses hardcoded values for several build arguments that could be made configurable via environment variables.

+if "%OSCRIPT_BASE_IMAGE%"=="" set "OSCRIPT_BASE_IMAGE=ubuntu"
+if "%OSCRIPT_BASE_TAG%"=="" set "OSCRIPT_BASE_TAG=20.04"
+if "%ONESCRIPT_PACKAGES%"=="" set "ONESCRIPT_PACKAGES=yard"
+
 docker build ^
         --pull ^
         --build-arg DOCKER_REGISTRY_URL=library ^
-    --build-arg BASE_IMAGE=ubuntu ^
-    --build-arg BASE_TAG=20.04 ^
-    --build-arg ONESCRIPT_PACKAGES="yard" ^
+    --build-arg BASE_IMAGE=%OSCRIPT_BASE_IMAGE% ^
+    --build-arg BASE_TAG=%OSCRIPT_BASE_TAG% ^
+    --build-arg ONESCRIPT_PACKAGES=%ONESCRIPT_PACKAGES% ^

1-50: Improve error handling and add cleanup procedures

The script would benefit from:

  1. Better error messages with context
  2. Cleanup of temporary resources on failure
  3. Logging of build steps
 @echo off
+setlocal EnableDelayedExpansion
+
+echo [INFO] Starting EDT build process...
+
+:: Function to log errors and exit
+:log_error
+echo [ERROR] %~1
+goto cleanup
+
+:: Cleanup function
+:cleanup
+echo [INFO] Cleaning up resources...
+if exist tmp_* del /q tmp_*
+goto end
 
 docker login -u %DOCKER_LOGIN% -p %DOCKER_PASSWORD% %DOCKER_REGISTRY_URL%
-
-if %ERRORLEVEL% neq 0 goto end
+if %ERRORLEVEL% neq 0 call :log_error "Docker login failed"
build-edt.sh (3)

4-11: Enhance Docker login validation and safety

While the login logic is functional, consider these improvements for robustness:

-if [ -n "${DOCKER_LOGIN}" ] && [ -n "${DOCKER_PASSWORD}" ] && [ -n "${DOCKER_REGISTRY_URL}" ]; then
+if [[ -n "${DOCKER_LOGIN}" ]] && [[ -n "${DOCKER_PASSWORD}" ]] && [[ -n "${DOCKER_REGISTRY_URL}" ]]; then
+    # Validate registry URL format
+    if [[ ! "${DOCKER_REGISTRY_URL}" =~ ^[a-zA-Z0-9][a-zA-Z0-9.-]*[a-zA-Z0-9]$ ]]; then
+        echo "Invalid Docker registry URL format"
+        exit 1
+    fi
     if ! docker login -u "${DOCKER_LOGIN}" -p "${DOCKER_PASSWORD}" "${DOCKER_REGISTRY_URL}"; then

17-24: Improve version comparison documentation and clarity

The version comparison logic is correct, but could be more maintainable:

-#Если версия EDT >= 2024.1.0, использовать JDK 17
+# Use JDK 17 for EDT version >= 2024.1.0, JDK 11 for older versions
 if [[ "$(printf "%s\n" "$EDT_VERSION" "2024" | sort -V | head -n 1)" == "2024" ]]; then
+  # Required for EDT 2024.1.0 and newer versions
   BASE_IMAGE="azul/zulu-openjdk"
   BASE_TAG="17"
 else
+  # For EDT versions before 2024.1.0
   BASE_IMAGE="eclipse-temurin"
   BASE_TAG="11"
 fi

34-44: Parameterize base image version and fix indentation

Consider making the base image version configurable and fixing indentation:

 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
+    --pull \
+    $no_cache_arg \
+    --build-arg DOCKER_REGISTRY_URL=library \
+    --build-arg BASE_IMAGE=${OSCRIPT_BASE_IMAGE:-ubuntu} \
+    --build-arg BASE_TAG=${OSCRIPT_BASE_TAG:-20.04} \
+    --build-arg ONESCRIPT_PACKAGES="yard" \
+    -t $DOCKER_REGISTRY_URL/oscript-downloader:latest \
+    -f oscript/Dockerfile \
+    $last_arg
🧰 Tools
🪛 Shellcheck

[warning] 36-36: no_cache_arg is referenced but not assigned.

(SC2154)

README.md (4)

51-52: Enhance COVERAGE41C_VERSION documentation

The description for COVERAGE41C_VERSION could be more detailed. Consider adding:

  • Expected version format
  • Where to obtain Coverage41C
  • Example value

Here's a suggested enhancement:

- * COVERAGE41C_VERSION - версия Coverage41C
- Используется при сборке агента скриптами `build-base-*-jenkins-coverage-agent.*`.
+ * COVERAGE41C_VERSION - версия Coverage41C (например, "1.4.0")
+ Используется при сборке агента скриптами `build-base-*-jenkins-coverage-agent.*`. 
+ Coverage41C можно скачать с https://github.com/proDOOMman/Coverage41C/releases.
+ Если значение не указано, сборка образа с поддержкой замеров покрытия не выполняется.
🧰 Tools
🪛 Markdownlint

51-51: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


49-51: Fix list style consistency

The list uses asterisks (*) while other lists in the document use dashes (-). Consider standardizing the list style.

- * EDT_VERSION - версия EDT. Обязательно заполнять только при сборке образов с EDT или при использовании замеров покрытия (см. `COVERAGE41C_VERSION`)
- * DOCKER_REGISTRY_URL - Адрес Docker-registry в котором будут храниться образы
- * COVERAGE41C_VERSION - версия Coverage41C
+ - EDT_VERSION - версия EDT. Обязательно заполнять только при сборке образов с EDT или при использовании замеров покрытия (см. `COVERAGE41C_VERSION`)
+ - DOCKER_REGISTRY_URL - Адрес Docker-registry в котором будут храниться образы
+ - COVERAGE41C_VERSION - версия Coverage41C
🧰 Tools
🪛 Markdownlint

49-49: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


50-50: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


51-51: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


71-74: Fix list style consistency

Standardize the list style to use dashes instead of asterisks.

- * build-base-swarm-jenkins-agent.sh (или build-base-swarm-jenkins-coverage-agent.sh с замерами покрытия)
- * build-edt-swarm-agent.sh
- * build-oscript-swarm-agent.sh
+ - build-base-swarm-jenkins-agent.sh (или build-base-swarm-jenkins-coverage-agent.sh с замерами покрытия)
+ - build-edt-swarm-agent.sh
+ - build-oscript-swarm-agent.sh
🧰 Tools
🪛 Markdownlint

71-71: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


72-72: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


73-73: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


77-77: Fix list style consistency

Standardize the list style to use dashes instead of asterisks.

- * build-base-k8s-jenkins-agent.sh (или build-base-k8s-jenkins-coverage-agent.sh с замерами покрытия)
+ - build-base-k8s-jenkins-agent.sh (или build-base-k8s-jenkins-coverage-agent.sh с замерами покрытия)
🧰 Tools
🪛 Markdownlint

77-77: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bb3a5a6 and d016935.

📒 Files selected for processing (14)
  • .onec.env.example (1 hunks)
  • README.md (5 hunks)
  • build-base-k8s-jenkins-agent.sh (1 hunks)
  • build-base-k8s-jenkins-coverage-agent.sh (1 hunks)
  • build-base-swarm-jenkins-agent.bat (1 hunks)
  • build-base-swarm-jenkins-agent.sh (1 hunks)
  • build-base-swarm-jenkins-coverage-agent.bat (1 hunks)
  • build-base-swarm-jenkins-coverage-agent.sh (1 hunks)
  • build-edt-k8s-agent.sh (1 hunks)
  • build-edt-swarm-agent.bat (1 hunks)
  • build-edt-swarm-agent.sh (1 hunks)
  • build-edt.bat (1 hunks)
  • build-edt.sh (1 hunks)
  • coverage41C/Dockerfile (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • .onec.env.example
  • build-base-k8s-jenkins-agent.sh
  • build-base-swarm-jenkins-agent.bat
  • build-base-swarm-jenkins-agent.sh
  • coverage41C/Dockerfile
🧰 Additional context used
🪛 Markdownlint
README.md

49-49: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


50-50: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


51-51: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


71-71: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


72-72: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


73-73: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


77-77: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)

🪛 Shellcheck
build-edt-k8s-agent.sh

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

build-edt.sh

[warning] 36-36: no_cache_arg is referenced but not assigned.

(SC2154)

🔇 Additional comments (6)
build-edt-k8s-agent.sh (1)

Line range hint 1-10: Verify the Dockerfile path and last_arg variable

The script references k8s-jenkins-agent/Dockerfile and uses an undefined $last_arg variable.

Let's verify these dependencies:

✅ Verification successful

Let me check the definition of last_arg in the relevant files.


Dependencies verified successfully

The verification shows that:

  1. The Dockerfile path k8s-jenkins-agent/Dockerfile exists and contains valid Docker instructions
  2. The $last_arg variable is defined in build-edt.sh which is called at the beginning of this script. It's set to either '.' or '--no-cache .' depending on the NO_CACHE environment variable.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the Dockerfile exists
if [ -f "k8s-jenkins-agent/Dockerfile" ]; then
    echo "Dockerfile found"
    cat k8s-jenkins-agent/Dockerfile
else
    echo "Dockerfile not found"
fi

# Search for last_arg variable definition
rg -l "last_arg="

Length of output: 2808


Script:

#!/bin/bash
# Search for last_arg definition in build-edt-k8s-agent.sh and related files
rg -A 2 "last_arg=" build-edt-k8s-agent.sh build-edt.sh

# Check if build-edt-k8s-agent.sh exists and show its content
if [ -f "build-edt-k8s-agent.sh" ]; then
    echo "Content of build-edt-k8s-agent.sh:"
    cat build-edt-k8s-agent.sh
fi

Length of output: 668

🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

build-edt-swarm-agent.sh (1)

Line range hint 1-13: Verify Docker registry accessibility

The script assumes the Docker registry is accessible without checking.

Run this script to verify registry accessibility:

🧰 Tools
🪛 Shellcheck

[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

(SC2148)

build-edt-swarm-agent.bat (1)

Line range hint 1-20: Verify batch script functionality

As mentioned in the PR objectives, the build process for agents using the batch file hasn't been verified. This needs attention to ensure it works correctly with the Coverage41C integration.

Run the following script to analyze the batch file usage and dependencies:

✅ Verification successful

Let me gather more specific information about the build process and Coverage41C integration.

This will help us understand the build process flow and Coverage41C integration details to verify the functionality.


The batch script functionality is correctly integrated with Coverage41C

The verification shows that:

  • build-edt-swarm-agent.bat correctly builds the base Jenkins agent image
  • build-base-swarm-jenkins-coverage-agent.bat properly extends this base image with Coverage41C integration
  • The build process is sequential and well-structured:
    1. Base EDT image is built first
    2. Jenkins agent layer is added
    3. Coverage41C is integrated with proper version control
  • All necessary environment variables (COVERAGE41C_VERSION, EDT_VERSION, etc.) are properly passed through the build chain
  • The Coverage41C Dockerfile correctly sets up the required EDT plugins and Coverage41C binaries

The batch script functionality works as intended for the Coverage41C integration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for related batch files and their dependencies

# Find all batch files that might be related to the build process
fd -e bat

# Check for references to environment variables used in the script
rg -l "DOCKER_REGISTRY_URL|edt_escaped|BASE_IMAGE|BASE_TAG" 

# Look for Coverage41C related configurations
rg -l "COVERAGE41C"

Length of output: 1270


Script:

#!/bin/bash
# Check the content of relevant build files and their relationships

# Check build-edt-swarm-agent.bat content for Coverage41C integration
cat build-edt-swarm-agent.bat

# Check the related coverage agent build script
cat build-base-swarm-jenkins-coverage-agent.bat

# Check swarm-jenkins-agent Dockerfile that's used in the build
cat swarm-jenkins-agent/Dockerfile

# Check coverage41C Dockerfile for dependencies
cat coverage41C/Dockerfile

Length of output: 3264

build-base-k8s-jenkins-coverage-agent.sh (1)

22-23: Verify script dependencies and add error handling

The script calls two dependencies without verifying their existence. Additionally, per PR objectives, the build process for batch files hasn't been verified.

build-edt.sh (1)

13-15: 🛠️ Refactor suggestion

Consider safer Docker cleanup strategy

The forced system prune (-af) removes all unused data without confirmation, which could impact other processes or cached images.

-if [ "${DOCKER_SYSTEM_PRUNE}" = 'true' ] ; then
-    docker system prune -af
+if [[ "${DOCKER_SYSTEM_PRUNE}" = 'true' ]] ; then
+    # Only remove dangling images and containers older than 24h
+    docker system prune --filter "until=24h" --force --filter "dangling=true"
README.md (1)

89-89: LGTM!

The Jenkins configuration instructions for nethasp.ini are clear and complete.

build-edt-k8s-agent.sh Show resolved Hide resolved
build-edt-swarm-agent.sh Show resolved Hide resolved
build-edt-swarm-agent.bat Show resolved Hide resolved
build-base-swarm-jenkins-coverage-agent.bat Show resolved Hide resolved
build-base-swarm-jenkins-coverage-agent.bat Show resolved Hide resolved
build-edt.bat Outdated Show resolved Hide resolved
build-edt.bat Show resolved Hide resolved
build-edt.bat Show resolved Hide resolved
build-edt.sh Show resolved Hide resolved
build-edt.sh Show resolved Hide resolved
@ovcharenko-di
Copy link
Author

@nixel2007 @Segate-ekb посмотрите, плз

я вроде все пожелания учел:

  • сборка edt выполняется отдельным скриптом, на него же я завязал сборку агента с coverage
  • вынес сборку агента с coverage в отдельные файлы
  • если выполняется сборка агента с coverage, то запушится только агент с coverage, а агент без него не запушится

билд валится, но я не вижу почему @nixel2007

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
build-edt-swarm-agent.bat (2)

Line range hint 1-24: Missing Coverage41C integration

According to the PR objectives, this script should handle Coverage41C integration when COVERAGE41C_VERSION is specified in .onec.env. However, the current implementation doesn't include this functionality.

Consider adding the following:

  1. Check for COVERAGE41C_VERSION
  2. Build the coverage agent image when version is specified
  3. Push the coverage agent image to registry
 @echo off
+
+rem Check for coverage version
+if defined COVERAGE41C_VERSION (
+    echo Building coverage agent image...
+    docker build ^
+        --build-arg DOCKER_REGISTRY_URL=%DOCKER_REGISTRY_URL% ^
+        --build-arg BASE_IMAGE=base-jenkins-agent ^
+        --build-arg BASE_TAG=%edt_escaped% ^
+        --build-arg COVERAGE41C_VERSION=%COVERAGE41C_VERSION% ^
+        -t %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%edt_escaped% ^
+        -f coverage-jenkins-agent/Dockerfile ^
+        %last_arg%
+
+    if %ERRORLEVEL% neq 0 goto end
+
+    docker push %DOCKER_REGISTRY_URL%/base-jenkins-coverage-agent:%edt_escaped%
+
+    if %ERRORLEVEL% neq 0 goto end
+)

Line range hint 1-24: Add environment variable validation

The script uses several environment variables without validating their presence. This could lead to cryptic errors if variables are missing.

Add validation at the start of the script:

 @echo off
+
+rem Validate required environment variables
+set missing_vars=0
+
+if not defined DOCKER_LOGIN (
+    echo Error: DOCKER_LOGIN is not set
+    set missing_vars=1
+)
+if not defined DOCKER_PASSWORD (
+    echo Error: DOCKER_PASSWORD is not set
+    set missing_vars=1
+)
+if not defined DOCKER_REGISTRY_URL (
+    echo Error: DOCKER_REGISTRY_URL is not set
+    set missing_vars=1
+)
+if not defined EDT_VERSION (
+    echo Error: EDT_VERSION is not set
+    set missing_vars=1
+)
+
+if %missing_vars%==1 (
+    echo One or more required environment variables are missing
+    goto end
+)
build-edt-k8s-agent.sh (2)

Line range hint 27-34: Fix inconsistent indentation in Docker build command

The -f flag uses tabs while other lines use spaces for indentation.

Apply this formatting fix:

 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 k8s-jenkins-agent/Dockerfile \
+    -f k8s-jenkins-agent/Dockerfile \
     $last_arg

Line range hint 27-34: Consider Coverage41C integration based on PR objectives

The Docker build command should be updated to handle Coverage41C integration when COVERAGE41C_VERSION is specified in .onec.env.

Consider updating the build process like this:

+coverage_tag=""
+if [ -n "${COVERAGE41C_VERSION}" ]; then
+    coverage_tag="-coverage"
+    docker build \
+        --build-arg DOCKER_REGISTRY_URL=$DOCKER_REGISTRY_URL \
+        --build-arg BASE_IMAGE=edt \
+        --build-arg BASE_TAG=$edt_escaped \
+        --build-arg COVERAGE41C_VERSION=$COVERAGE41C_VERSION \
+        -t $DOCKER_REGISTRY_URL/edt-agent${coverage_tag}:$edt_escaped \
+        -f k8s-jenkins-agent/Dockerfile.coverage \
+        $last_arg
+else
 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 \
+    -t $DOCKER_REGISTRY_URL/edt-agent${coverage_tag}:$edt_escaped \
     -f k8s-jenkins-agent/Dockerfile \
     $last_arg
+fi

-docker push $DOCKER_REGISTRY_URL/edt-agent:$edt_escaped
+docker push $DOCKER_REGISTRY_URL/edt-agent${coverage_tag}:$edt_escaped

This change:

  1. Conditionally builds a coverage-enabled image when COVERAGE41C_VERSION is set
  2. Uses a separate Dockerfile for coverage builds
  3. Adds coverage version as a build argument
  4. Appends -coverage suffix to image tag when appropriate
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d016935 and 27011bd.

📒 Files selected for processing (3)
  • build-edt-k8s-agent.sh (1 hunks)
  • build-edt-swarm-agent.bat (1 hunks)
  • build-edt-swarm-agent.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • build-edt-swarm-agent.sh
🔇 Additional comments (3)
build-edt-swarm-agent.bat (1)

16-16: Error handling for build-edt.bat execution

This comment was previously raised and is still valid. The script continues even if build-edt.bat fails.

build-edt-k8s-agent.sh (2)

25-25: Add error handling for the build-edt.sh dependency

The script immediately calls build-edt.sh without verifying its existence or executable permissions.


24-25: ⚠️ Potential issue

Add validation for required EDT_VERSION variable

The script uses EDT_VERSION without validating its presence, which could lead to unclear errors downstream.

Add this validation before using the variable:

+if [ -z "${EDT_VERSION}" ]; then
+    echo "Error: EDT_VERSION is required but not set"
+    exit 1
+fi
+
 edt_version=$EDT_VERSION
 edt_escaped="${edt_version// /_}"

Likely invalid or redundant comment.

@ovcharenko-di
Copy link
Author

@nixel2007 починил билд

ovcharenko-di and others added 2 commits November 16, 2024 12:22
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@nixel2007 nixel2007 merged commit 1291bc7 into firstBitMarksistskaya:feature/first-bit Nov 16, 2024
8 checks passed
@ovcharenko-di ovcharenko-di mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants