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: get latest upstream build in release workflow #2325

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

VietND96
Copy link
Member

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

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 condition in chart_test.sh to set chromeNode.imageName if TEST_CHROMIUM is true.
  • Updated deploy workflow to use a new stable input instead of release.
  • Added base_nightly target in Makefile and updated various chart test targets to include TEST_CHROMIUM=true.
  • Modified commands in Dockerfiles to get the full version of Chrome, Chromium, and Edge.
  • Changed Chromium installation method to use PPA for better package management.

Changes walkthrough 📝

Relevant files
Enhancement
chart_test.sh
Add condition for Chromium testing in Helm chart                 

tests/charts/make/chart_test.sh

  • Added condition to set chromeNode.imageName if TEST_CHROMIUM is true.
  • +5/-0     
    Makefile
    Add nightly build target and update chart tests                   

    Makefile

  • Added base_nightly target.
  • Updated various chart test targets to include TEST_CHROMIUM=true.
  • +12/-2   
    Dockerfile
    Update Chrome version extraction command                                 

    NodeChrome/Dockerfile

    • Modified command to get full Chrome version.
    +1/-1     
    Dockerfile
    Update Chromium installation and version extraction           

    NodeChromium/Dockerfile

  • Changed Chromium installation method to use PPA.
  • Modified command to get full Chromium version.
  • +5/-5     
    Dockerfile
    Update Edge version extraction command                                     

    NodeEdge/Dockerfile

    • Modified command to get full Edge version.
    +1/-1     
    Configuration changes
    deploy.yml
    Update deploy workflow to use stable input                             

    .github/workflows/deploy.yml

  • Added stable input for workflow_dispatch.
  • Updated references from release to stable.
  • +7/-2     

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

    @VietND96 VietND96 merged commit b4327c2 into SeleniumHQ:trunk Jul 26, 2024
    14 of 15 checks passed
    Copy link

    PR Reviewer Guide 🔍

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

    Possible Bug
    The condition in line 33 checks for a specific commit message or a workflow dispatch event, but it uses 'github.event.inputs.skip-test' which is not defined in the inputs of the workflow dispatch. This might lead to unexpected behaviors or errors during the workflow execution.

    Redundant Condition
    The condition in line 176 checks for the same platform condition as in line 170 but adds an additional check for 'TEST_CHROMIUM'. This seems redundant and could be simplified or better integrated with the existing conditions to avoid repetition and improve clarity.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Correct the logical expression to respect the 'stable' input from the workflow dispatch

    The expression ${{ github.event.inputs.stable || true }} will always evaluate to
    true because of the logical OR with true. This might not be the intended behavior if
    you want to respect the stable input from the workflow dispatch.

    .github/workflows/deploy.yml [36]

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

    Why: Correcting the logical expression is essential to ensure that the 'stable' input from the workflow dispatch is respected, which is critical for the intended functionality.

    10
    Possible bug
    Remove the misplaced 'fi' statement to ensure proper logical flow

    The closing fi statement at line 174 seems to be misplaced or unnecessary, as it
    closes the if block prematurely before the additional conditions are checked. This
    could lead to logical errors in script execution.

    tests/charts/make/chart_test.sh [174]

    -fi
    +# Removed the misplaced 'fi'
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Removing the misplaced 'fi' statement is crucial to ensure the proper logical flow of the script, preventing potential execution errors.

    9
    Enhancement
    Add a dependency on the 'base' target for 'base_nightly' to ensure correct build order

    The target base_nightly should depend on base to ensure that the base image is built
    before the nightly version, similar to how hub depends on base. This ensures that
    all prerequisites are met before building the nightly image.

    Makefile [73-74]

    -base_nightly:
    +base_nightly: base
       cd ./Base && docker buildx build --platform $(PLATFORMS) $(BUILD_ARGS) --build-arg VERSION=$(BASE_VERSION_NIGHTLY) --build-arg RELEASE=$(BASE_RELEASE_NIGHTLY) --build-arg AUTHORS=$(AUTHORS) -t $(NAME)/base:$(TAG_VERSION) .
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a dependency on the 'base' target for 'base_nightly' improves the build process by ensuring the correct order, which is important for maintainability and correctness.

    8
    Modify the command to capture only the major version of the browser if that's all that's required

    The command to get the browser version now captures the full version string, which
    might include more detail than needed for some uses. If only the major version is
    needed, consider parsing it explicitly.

    NodeChrome/Dockerfile [77]

    -RUN google-chrome --version | awk '{print $3}' > /opt/selenium/browser_version
    +RUN google-chrome --version | awk '{print $3}' | cut -d'.' -f1 > /opt/selenium/browser_version
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: While capturing only the major version might be useful in some contexts, the current implementation is not necessarily incorrect. This suggestion is more about optimization and clarity.

    7

    @VietND96 VietND96 added this to the 4.23.0 milestone Jul 27, 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