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

build: dynamic fetch upstream build #2316

Merged
merged 1 commit into from
Jul 20, 2024
Merged

Conversation

VietND96
Copy link
Member

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

build: dynamic fetch upstream build

Motivation and Context

Dynamic fetch build number (latest stable, Nightly) from the upstream repo SeleniumHQ/selenium via GitHub CLI to build-test-deploy docker images

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, configuration changes


Description

  • Added a new GitHub Action to fetch the latest upstream Selenium release.
  • Updated multiple workflows (build-test.yml, deploy.yml, docker-test.yml, helm-chart-release.yml, helm-chart-test.yml, nightly.yml) to change release input type from boolean to string.
  • Integrated the new action to set Selenium base version in relevant workflows.
  • Updated job names in workflows for better clarity.
  • Simplified Makefile by removing base_nightly target.

Changes walkthrough 📝

Relevant files
Enhancement
action.yml
Add GitHub Action for fetching latest Selenium release     

.github/actions/get-latest-upstream/action.yml

  • Added a new GitHub Action to fetch the latest upstream Selenium
    release.
  • Included steps to install GitHub CLI and authenticate using a token.
  • Implemented logic to determine the latest stable or nightly release.
  • +40/-0   
    Configuration changes
    build-test.yml
    Update build-test workflow with new release handling         

    .github/workflows/build-test.yml

  • Changed release input type from boolean to string.
  • Updated job names for better clarity.
  • Integrated new action to set Selenium base version.
  • +5/-2     
    deploy.yml
    Update deploy workflow with new release handling                 

    .github/workflows/deploy.yml

  • Changed release input type from boolean to string.
  • Updated job names for better clarity.
  • Integrated new action to set Selenium base version.
  • +10/-4   
    docker-test.yml
    Update docker-test workflow with new release handling       

    .github/workflows/docker-test.yml

  • Changed release input type from boolean to string.
  • Integrated new action to set Selenium base version.
  • +6/-6     
    helm-chart-release.yml
    Update helm-chart-release workflow input type                       

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

    • Changed release input type from boolean to string.
    +2/-2     
    helm-chart-test.yml
    Update helm-chart-test workflow with new release handling

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

  • Changed release input type from boolean to string.
  • Integrated new action to set Selenium base version.
  • +6/-6     
    nightly.yml
    Update nightly workflow with new release handling               

    .github/workflows/nightly.yml

  • Updated job names for better clarity.
  • Integrated new action to set Selenium base version.
  • +8/-2     
    Makefile
    Simplify Makefile by removing base_nightly target               

    Makefile

    • Removed base_nightly target.
    +0/-3     

    💡 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
    🔒 Security concerns

    No specific security vulnerabilities identified beyond the potential exposure of gh_cli_token as noted in the key issues.

    ⚡ Key issues to review

    Possible Bug
    The script uses sudo to install gh which might not be necessary or ideal in GitHub Actions environment. Consider using a pre-built Docker image with gh installed or modify the runner's environment to pre-install gh without sudo.

    Security Concern
    Storing the gh_cli_token directly in the script via echo might expose the token in logs. It's safer to use GitHub secrets or environment variables managed by GitHub Actions to handle sensitive information.

    Type Mismatch
    The input type for 'release' has been changed from boolean to string which might cause issues in conditional checks expecting boolean values in the workflows.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure the release input is evaluated correctly as a boolean by handling case sensitivity

    Correct the logical expression to ensure that the release input is properly
    evaluated as a boolean in the workflow conditions.

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

    -release: ${{ inputs.release == 'true' }}
    +release: ${{ inputs.release == 'true' || inputs.release == 'True' }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug by ensuring that the release input is correctly evaluated as a boolean, regardless of case sensitivity, which is crucial for the workflow's correct execution.

    9
    Possible issue
    Correct the handling of the release input to ensure it behaves as expected in conditional checks

    Modify the conditional expression to correctly handle the release input as a boolean
    string, ensuring consistent behavior across different workflow triggers.

    .github/workflows/deploy.yml [31]

    -release: ${{ github.event.inputs.release || true }}
    +release: ${{ github.event.inputs.release == 'true' || github.event.inputs.release == 'True' || true }}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion fixes a potential issue by ensuring the release input is correctly interpreted as a boolean string, which is important for consistent behavior across different workflow triggers.

    9
    Security
    Remove the use of sudo to enhance security and reduce privilege requirements

    Replace the use of sudo in the GitHub Actions script to avoid potential security
    risks and ensure that the action does not require elevated privileges unnecessarily.

    .github/actions/get-latest-upstream/action.yml [20-21]

    -sudo apt update
    -sudo apt install gh
    +apt update
    +apt install gh
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Removing sudo can enhance security by reducing the need for elevated privileges, which is a good practice in CI/CD pipelines. However, it is important to ensure that the commands will still work without sudo.

    8
    Reliability
    Improve the reliability of release extraction by specifying the field separator in awk

    Use a more specific selector for the awk command to ensure that the correct field is
    always extracted, regardless of potential format changes in the gh release list
    output.

    .github/actions/get-latest-upstream/action.yml [25]

    -RELEASE=$(gh release list -R SeleniumHQ/selenium | grep -v nightly | awk '{ print $4 }' | head -1)
    +RELEASE=$(gh release list -R SeleniumHQ/selenium | grep -v nightly | awk -F'\t' '{ print $4 }' | head -1)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more specific field separator in awk can improve the reliability of the script by ensuring the correct field is always extracted, even if the format of gh release list output changes.

    7

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 merged commit fffb8da into SeleniumHQ:trunk Jul 20, 2024
    29 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.

    None yet

    1 participant