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

chart(build): Push chart to Docker Hub OCI-based registry #2319

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 22, 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

chart(build): Push chart to Docker Hub OCI-based registry

Motivation and Context

Besides, the chart repository is hosted by the GH page in the repository.
Now, the chart is also pushed to the Docker Hub OCI-based registry. Refer to how to interact with OCI-based registry via Helm CLI - https://helm.sh/docs/topics/registries/#using-an-oci-based-registry

Repo on Docker Hub: https://hub.docker.com/r/selenium/selenium-grid

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.

PR Type

Enhancement, Tests


Description

  • Enhanced chart_build.sh script to include version control and debugging.
  • Added a new script chart_release.sh to push Helm charts to Docker Hub OCI-based registry.
  • Updated GitHub Actions workflows (deploy.yml and nightly.yml) to include steps for building and pushing Helm charts.
  • Modified Makefile to include a new target chart_release for releasing Helm charts.

Changes walkthrough 📝

Relevant files
Enhancement
chart_build.sh
Enhance chart build script with version control and debugging

tests/charts/make/chart_build.sh

  • Added set -x for debugging.
  • Introduced SET_VERSION variable to conditionally set Helm chart
    version.
  • Modified Helm package command to use ADD_VERSION based on SET_VERSION.

  • +11/-2   
    chart_release.sh
    Add script to release Helm chart to Docker Hub                     

    tests/charts/make/chart_release.sh

  • Created new script to push Helm chart to Docker Hub OCI-based
    registry.
  • Added error handling for missing chart package path.
  • +22/-0   
    deploy.yml
    Update deployment workflow to include Helm chart release 

    .github/workflows/deploy.yml

  • Added steps to build and push Helm chart in the deployment workflow.
  • Included Docker Hub and Helm registry login for pushing charts.
  • +24/-6   
    nightly.yml
    Update nightly workflow to include Helm chart release       

    .github/workflows/nightly.yml

  • Added steps to build and push Helm chart in the nightly workflow.
  • Included Docker Hub and Helm registry login for pushing charts.
  • +20/-9   
    Makefile
    Add chart release target to Makefile                                         

    Makefile

  • Added chart_release target to Makefile for releasing Helm charts.
  • +3/-0     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The script lacks proper error handling for the helm package command. Consider adding error checks or a trap to handle potential failures in this step.

    Error Handling
    The script exits only if the chart package path is empty or the file does not exist. It should also handle potential errors during the helm push command.

    Credentials Exposure
    Ensure that the environment variables DOCKER_USERNAME and DOCKER_PASSWORD are securely handled and not exposed in logs or error messages.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a specific version of the GitHub Action to ensure stability

    It's recommended to use a specific version of the GitHub Action rather than the
    master branch to avoid potential issues with changes that could break your workflow.

    .github/workflows/deploy.yml [105]

    -uses: nick-invision/retry@master
    +uses: nick-invision/[email protected]
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Using a specific version of the GitHub Action ensures stability and prevents potential issues caused by changes in the master branch, which is a best practice for CI/CD pipelines.

    10
    Possible bug
    Ensure the VERSION variable is defined to avoid runtime errors

    Ensure that the VERSION variable is defined before using it in the conditional
    block. This can be done by providing a default value or checking its existence.

    tests/charts/make/chart_build.sh [37-41]

    +VERSION=${VERSION:-"default_version"}
     if [ "${SET_VERSION}" = "true" ]; then
       ADD_VERSION="--version ${VERSION}"
     else
       ADD_VERSION=""
     fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by ensuring that the VERSION variable is defined before use, which is crucial for the script's reliability.

    9
    Enhancement
    Add error handling to the script to manage failures gracefully

    Use trap to catch and handle errors during the execution of the script, ensuring
    that any failure in the pipeline is caught and handled properly.

    tests/charts/make/chart_build.sh [2]

    -set -x
    +set -xe
    +trap 'echo "An error occurred. Exiting..." && exit 1' ERR
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling improves the robustness of the script by ensuring that any failure in the pipeline is caught and handled properly, preventing unexpected behavior.

    8
    Performance
    Optimize dependency installation by checking existing installations before running pip install

    Instead of suppressing 'Requirement already satisfied' messages, consider using a
    requirements file and checking if the installation is necessary before running pip
    install.

    tests/charts/make/chart_build.sh [27-29]

    -python3 -m pip install yamale==4.0.4 \
    -                      yamllint==1.33.0 \
    -                      | grep -v 'Requirement already satisfied' || true
    +if ! python3 -m pip freeze | grep -q 'yamale==4.0.4' && ! python3 -m pip freeze | grep -q 'yamllint==1.33.0'; then
    +  python3 -m pip install yamale==4.0.4 yamllint==1.33.0
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion optimizes the dependency installation process, improving performance by avoiding unnecessary installations. However, it is a minor enhancement compared to critical bug fixes.

    7

    @VietND96 VietND96 merged commit 0bb07ef into SeleniumHQ:trunk Jul 22, 2024
    14 of 16 checks passed
    @VietND96 VietND96 added this to the 4.23.0 milestone Jul 22, 2024
    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