Home Original page

[build] Release workflow improvements by titusfortner · Pull Request #16947 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Serialize jobs that push to trunk

The docs and update-version jobs both push to the trunk branch and can run in
parallel, creating a race condition. To fix this, make the update-version job
dependent on the docs job to ensure they run sequentially.

Examples:

.github/workflows/release.yml [117-157] 
    needs: [stage, publish, github-release]
    permissions:
      contents: write
    strategy:
      fail-fast: false
      matrix:
        language: [java, py, rb, dotnet, node]
    uses: ./.github/workflows/update-documentation.yml
    with:
      tag: ${{ needs.stage.outputs.tag }}

 ... (clipped 31 lines)

Solution Walkthrough:

Before:

jobs:
  # ...
  docs:
    needs: [stage, publish, github-release]
    steps:
      - uses: actions/checkout@v4
        with:
          ref: 'trunk'
      # ...
      - run: git push

  update-version:
    needs: [stage, unrestrict-trunk] # Does not wait for 'docs'
    steps:
      # ...
      - run: git push
  # ...

After:

jobs:
  # ...
  docs:
    needs: [stage, publish, github-release]
    steps:
      - uses: actions/checkout@v4
        with:
          ref: 'trunk'
      # ...
      - run: git push

  update-version:
    needs: [stage, unrestrict-trunk, docs] # Waits for 'docs' to complete
    steps:
      # ...
      - run: git push
  # ...
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical race condition introduced by the PR where the docs and update-version jobs push to the trunk branch in parallel, which will cause the release workflow to fail.

High
General
Declare secrets for reusable workflow

In the mirror-selenium-releases.yml file, declare the secrets the reusable
workflow accepts under the workflow_call trigger to allow it to receive them
from the calling workflow.

.github/workflows/mirror-selenium-releases.yml [7]

 workflow_call:
+  secrets:
+    SELENIUM_CI_TOKEN:
+      required: true
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The PR makes the workflow reusable but omits declaring the secrets it needs, which would cause the workflow to fail when called. This suggestion fixes a critical flaw in the PR's implementation of the reusable workflow.

High
Possible issue
Ensure failure notifications are always sent

Modify the on-release-failure job's trigger condition from if: failure() to if:
always() and add a check for non-successful job results to ensure failure
notifications are sent even for skipped jobs.

.github/workflows/release.yml [194-217]

 on-release-failure:
   name: On Release Failure
   runs-on: ubuntu-latest
-  needs: [stage, publish, docs, github-release, update-version, nightly, mirror]
-  if: failure()
+  needs: [stage, get-approval, publish, docs, github-release, unrestrict-trunk, update-version, nightly, mirror]
+  if: always() && (needs.stage.result != 'success' || needs.get-approval.result != 'success' || needs.publish.result != 'success' || needs.docs.result != 'success' || needs.github-release.result != 'success' || needs.unrestrict-trunk.result != 'success' || needs.update-version.result != 'success' || needs.nightly.result != 'success' || needs.mirror.result != 'success')
   steps:
     - uses: actions/checkout@v4
     - name: Slack Notification
       uses: rtCamp/action-slack-notify@v2
       env:
         SLACK_ICON_EMOJI: ":rotating_light:"
         SLACK_COLOR: failure
         SLACK_CHANNEL: selenium-tlc
         SLACK_USERNAME: GitHub Workflows
         SLACK_TITLE: Release failed
         SLACK_MESSAGE: |
           • Selenium Published: ${{ needs.publish.result }}
           • Docs Updated: ${{ needs.docs.result }}
           • GitHub Release Published: ${{ needs.github-release.result }}
           • Nightly Version Updated: ${{ needs.update-version.result }}
           • Nightly Packages: ${{ needs.nightly.result }}
           • Mirror Updated: ${{ needs.mirror.result }}
         MSG_MINIMAL: actions url
         SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL }}
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that if: failure() will not trigger for skipped jobs, leading to missed failure notifications. The proposed change to if: always() with a condition checking job results is a robust pattern for ensuring notifications are always sent on any unsuccessful run.

Medium
  • More