Skip to content

[build]: use docker buildx #2229

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

Merged
merged 1 commit into from
Apr 26, 2024
Merged

[build]: use docker buildx #2229

merged 1 commit into from
Apr 26, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Apr 26, 2024

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Pre-steps to be ready with arm64 supports.
Make some common changes to reduce conflicts when syncing the downstream repo arm

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

Enhancement


Description

  • Added Docker Buildx and QEMU setup across various GitHub Actions workflows to enable multi-platform Docker image builds.
  • Updated Dockerfiles and Makefile to support architecture-specific builds, enhancing compatibility with AMD64 and ARM64 platforms.
  • Modified Dockerfile commands to conditionally execute based on the target architecture, ensuring appropriate environment setup.

Changes walkthrough

Relevant files
Enhancement
9 files
build-test.yml
Setup QEMU and Docker Buildx in GitHub Actions for build-test workflow

.github/workflows/build-test.yml

  • Added steps to set up QEMU and Docker Buildx with specific platforms
    for building Docker images.
  • +8/-0     
    deploy.yml
    Integrate Docker Buildx and QEMU in deploy workflow           

    .github/workflows/deploy.yml

  • Introduced steps to configure QEMU and Docker Buildx for
    multi-platform Docker image builds.
  • +7/-0     
    helm-chart-test.yml
    Configure QEMU and Docker Buildx for helm-chart-test workflow

    .github/workflows/helm-chart-test.yml

  • Configured QEMU and Docker Buildx setup for ARM64 and AMD64 platforms.

  • +7/-0     
    nightly.yaml
    Setup multi-platform builds in nightly workflow                   

    .github/workflows/nightly.yaml

  • Added Docker Buildx and QEMU setup for nightly builds across multiple
    platforms.
  • +7/-0     
    test-video.yml
    Implement Docker Buildx and QEMU in test-video workflow   

    .github/workflows/test-video.yml

  • Implemented Docker Buildx and QEMU for building Docker images on
    different architectures.
  • +7/-0     
    update-dev-beta-browser-images.yml
    Setup Docker Buildx and QEMU for browser image builds       

    .github/workflows/update-dev-beta-browser-images.yml

  • Setup Docker Buildx and QEMU for building browser images on AMD64 and
    ARM64.
  • +7/-0     
    Dockerfile
    Add architecture-specific conditions in Dockerfile             

    Base/Dockerfile

  • Conditional Docker commands based on target architecture for package
    installations.
  • Added architecture-specific conditional checks before running certain
    install commands.
  • +17/-11 
    Makefile
    Integrate Docker Buildx for multi-platform support in Makefile

    Makefile

  • Added TARGETARCH and PLATFORMS variables for architecture-specific
    builds.
  • Replaced docker build commands with docker buildx to support
    multi-platform builds.
  • +44/-37 
    Dockerfile
    Enhance Dockerfile with architecture-specific installations

    NodeBase/Dockerfile

  • Introduced TARGETARCH argument for architecture-specific operations.
  • Conditional installation of packages based on the architecture.
  • +7/-3     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 requested a review from diemol April 26, 2024 08:44
    Copy link

    PR Description updated to latest commit (5b14b76)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves multiple changes across various GitHub Actions workflows and Dockerfiles, including the introduction of Docker Buildx for multi-platform builds, and modifications to the Makefile and Dockerfile to support different architectures. This requires a detailed review to ensure compatibility and correctness across different platforms.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The condition if [ "${TARGETARCH}" = "amd64" ]; then in Dockerfiles might not cover all necessary cases for different architectures. It's important to ensure that all required packages are installed for other architectures as well.

    Compatibility Issue: The use of master branch in actions like docker/setup-qemu-action@master could lead to potential instability. It's generally safer to pin actions to a specific release or commit.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Improve architecture detection reliability in Docker build commands.

    Consider using a more robust conditional check for architecture compatibility in the
    Docker build commands. Using uname -m provides a more reliable output across different
    systems compared to arch.

    Makefile [113]

    -if [ `arch` = "aarch64" ] || [ `arch` = "x86_64" ]; then \
    +if [ `uname -m` = "aarch64" ] || [ `uname -m` = "x86_64" ]; then \
     
    Use the TARGETARCH environment variable consistently for effective multi-architecture support.

    Ensure that the TARGETARCH environment variable is used consistently across the Dockerfile
    to handle different architectures effectively.

    NodeBase/Dockerfile [18]

    -ARG TARGETARCH=amd64
    +ARG TARGETARCH
     
    Best practice
    Add a check to ensure docker-buildx-plugin is only installed if not already available.

    To avoid potential issues with sudo permissions and system configurations, consider
    checking if the docker-buildx-plugin is already installed before attempting to install it.
    This can prevent unnecessary system modifications and ensure smoother CI/CD operations.

    Makefile [47]

    -sudo apt-get install --upgrade docker-buildx-plugin
    +if ! docker buildx version; then sudo apt-get install --upgrade docker-buildx-plugin; fi
     
    Use specific tags instead of master for Docker actions to enhance stability and reproducibility.

    Consider using a more specific tag than master for the docker/setup-qemu-action and
    docker/setup-buildx-action to ensure reproducibility and to avoid potential breaking
    changes from the master branch.

    NodeBase/Dockerfile [27-29]

    -uses: docker/setup-qemu-action@master
    -uses: docker/setup-buildx-action@master
    +uses: docker/setup-qemu-action@v1
    +uses: docker/setup-buildx-action@v1
     
    Performance
    Add a condition to Docker build commands to only build if the image does not already exist.

    To ensure that the Docker images are built only if necessary, consider adding a dependency
    check to the build targets that verifies if the base images or configurations have changed
    before triggering a rebuild.

    Makefile [86]

    -cd ./NodeChrome && docker buildx build --platform $(PLATFORMS) $(BUILD_ARGS) $(FROM_IMAGE_ARGS) --load -t $(NAME)/node-chrome:$(TAG_VERSION) .
    +[ ! "$(docker images -q $(NAME)/node-chrome:$(TAG_VERSION) 2> /dev/null)" == "" ] || cd ./NodeChrome && docker buildx build --platform $(PLATFORMS) $(BUILD_ARGS) $(FROM_IMAGE_ARGS) --load -t $(NAME)/node-chrome:$(TAG_VERSION) .
     
    Maintainability
    Use the TARGETARCH variable for setting the PLATFORMS dynamically.

    Instead of hardcoding the platform 'amd64' in the Docker build arguments, use the
    TARGETARCH variable directly to allow for more flexible and dynamic builds across
    different architectures.

    Makefile [24]

    -PLATFORMS := $(or $(PLATFORMS),$(PLATFORMS),linux/amd64)
    +PLATFORMS := $(or $(PLATFORMS),$(PLATFORMS),linux/$(TARGETARCH))
     
    Refactor the addition of repository sources to the sources.list file to reduce redundancy and potential errors.

    To ensure that the Dockerfile is more maintainable and less error-prone, consider using a
    loop or a function to append the repository sources to the sources.list file instead of
    repeating the echo command multiple times.

    Base/Dockerfile [40-42]

    -echo "deb http://archive.ubuntu.com/ubuntu jammy main universe\n" > /etc/apt/sources.list \
    -&& echo "deb http://archive.ubuntu.com/ubuntu jammy-updates main universe\n" >> /etc/apt/sources.list \
    -&& echo "deb http://security.ubuntu.com/ubuntu jammy-security main universe\n" >> /etc/apt/sources.list ;
    +for repo in "deb http://archive.ubuntu.com/ubuntu jammy main universe" "deb http://archive.ubuntu.com/ubuntu jammy-updates main universe" "deb http://security.ubuntu.com/ubuntu jammy-security main universe"; do \
    +    echo "$repo\n" >> /etc/apt/sources.list ; \
    +done
     
    Group package installations into a single apt-get install command for better maintainability.

    To ensure the Dockerfile is more maintainable and readable, consider grouping the
    installation of packages into a single apt-get install command instead of multiple
    commands.

    NodeBase/Dockerfile [100-102]

    -apt-get -qqy --no-install-recommends install \
    -language-pack-en \
    -xfonts-cyrillic \
    -fonts-ubuntu ;
    +apt-get -qqy --no-install-recommends install language-pack-en xfonts-cyrillic fonts-ubuntu ;
     
    Possible issue
    Adjust the use parameter in setup-buildx-action to avoid potential configuration issues.

    To avoid potential issues with the use parameter being set to false in the
    setup-buildx-action, consider removing it or setting it to true if necessary for your
    workflow.

    .github/workflows/update-dev-beta-browser-images.yml [31]

    -use: false
    +use: true
     
    Reliability
    Add error handling for Docker commands to enhance the robustness of the workflow.

    Consider adding error handling or a retry mechanism for steps that might fail due to
    external dependencies, such as docker info or Docker setup actions.

    .github/workflows/build-test.yml [35]

     - name: Output Docker info
    -  run: docker info
    +  run: docker info || echo "Failed to get Docker info"
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96 VietND96 merged commit 6f0f7a1 into SeleniumHQ:trunk Apr 26, 2024
    12 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant