[build] Migrate workflows to use centralized bazel.yml by titusfortner · Pull Request #16955 · SeleniumHQ/selenium
PR Code Suggestions ✨
Latest suggestions up to c819f09
| Category | Suggestion | Impact |
| Possible issue |
Avoid failing on missing artifacts
In the commit-fixes job, make the download-artifact step non-fatal by adding
continue-on-error: true and adjust the subsequent step to handle cases where the artifact is missing. This prevents masking the original failure cause from the
format job.
.github/workflows/ci-lint.yml [56-95]
commit-fixes:
name: Commit fixes
needs: format
if: failure() && github.event_name == 'pull_request'
runs-on: ubuntu-latest
permissions:
contents: write
actions: read
steps:
- name: Check Permissions
if: github.event.pull_request.head.repo.fork == true
run: |
echo "::error::Code needs formatting. Run ./scripts/format.sh locally and push changes."
exit 1
- name: Checkout PR
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }}
- name: Download format changes
uses: actions/download-artifact@v4
+ continue-on-error: true
with:
name: format-changes
- name: Apply and commit fixes
id: apply
run: |
- if [ -s changes.patch ]; then
+ if [ -f changes.patch ] && [ -s changes.patch ]; then
git apply --index changes.patch
rm changes.patch
git config --local user.name "Selenium CI Bot"
git config --local user.email "selenium-ci@users.noreply.github.com"
git commit -m "Auto-format code"
echo "has_changes=true" >> "$GITHUB_OUTPUT"
else
- echo "::notice::No formatting changes needed."
+ echo "::notice::No format patch artifact found; skipping auto-commit."
+ echo "has_changes=false" >> "$GITHUB_OUTPUT"
fi
- name: Push fixes
if: steps.apply.outputs.has_changes == 'true'
run: |
git push
echo "::notice::Auto-formatted and pushed. New CI run will start."
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that the commit-fixes job could fail and mask the root cause if the format job fails for reasons other than formatting, thus not producing the expected artifact. Adding continue-on-error makes the workflow more robust and improves debuggability.
| Medium
|
Upload only when files exist
In the bazel.yml workflow, modify the if condition for the Upload artifact step to use hashFiles to ensure an artifact is only uploaded if the specified
artifact-path contains files or if changes.patch exists and is not empty.
.github/workflows/bazel.yml [257-264]
- name: Upload artifact
- if: always() && inputs.artifact-name != ''
+ if: |
+ always() && inputs.artifact-name != '' && (
+ (inputs.artifact-path != '' && hashFiles(inputs.artifact-path) != '') ||
+ (inputs.artifact-path == '' && hashFiles('changes.patch') != '')
+ )
uses: actions/upload-artifact@v5
with:
name: ${{ inputs.artifact-name }}
path: ${{ inputs.artifact-path || 'changes.patch' }}
retention-days: 6
if-no-files-found: ${{ inputs.artifact-path != '' && 'error' || 'warn' }}
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a potential failure point where a downstream job might fail if an expected artifact is not created. Conditionally running the upload step based on file existence makes the reusable workflow more robust and prevents downstream failures.
| Medium
|
| Incremental [*] |
Include untracked files in patch
Before running git diff, stage untracked files using git add -N . to ensure they are included in the generated changes.patch.
.github/workflows/bazel.yml [256]
-run: git diff --binary > changes.patch
+run: |
+ git add -N .
+ git diff --binary > changes.patch
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that git diff misses untracked files and proposes using git add -N . to include them in the patch, which prevents changes from being silently lost.
| Medium
|
Avoid empty commits after patch
After applying a patch with git apply --index, check for staged changes using
git diff --cached --quiet before attempting to commit, to avoid errors from empty commits.
.github/workflows/ci-renovate-rbe.yml [58-66]
if [ -f changes.patch ] && [ -s changes.patch ]; then
git apply --index changes.patch
+ if git diff --cached --quiet; then
+ echo "No changes to commit"
+ exit 0
+ fi
git config --local user.email "selenium-ci@users.noreply.github.com"
git config --local user.name "Selenium CI Bot"
git commit -m 'Repin dependencies'
git push
else
echo "No changes to commit"
fi
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that git apply might not produce changes, which would cause git commit to fail, and proposes a robust check using git diff --cached --quiet to prevent the error.
| Medium
|
|
| |
Previous suggestions
✅ Suggestions up to commit 62af24c
| Category | Suggestion | Impact |
| Possible issue |
✅ Fix artifact path nesting
Suggestion Impact:The commit removed the explicit `path: docs/api` from the artifact download step, which effectively causes the artifact to be downloaded to the default workspace directory (equivalent to `path: .`), addressing the nested directory issue.
code diff:
@@ -95,7 +95,6 @@
uses: actions/download-artifact@v4
with:
name: documentation
- path: docs/api
- name: Setup git
In .github/workflows/update-documentation.yml, change the download path for the
documentation artifact from docs/api to . to prevent creating a nested and incorrect directory structure.
.github/workflows/update-documentation.yml [94-98]
- name: Download documentation
uses: actions/download-artifact@v4
with:
name: documentation
- path: docs/api
+ path: .
[Suggestion processed]
Suggestion importance[1-10]: 9
__
Why: This is a critical bug fix. The upload-artifact action preserves the full path, so downloading to a subdirectory creates a nested, incorrect path structure (docs/api/docs/api/...), which would break the documentation update logic.
| High
|
✅ Prevent nested dist directories
Suggestion Impact:The workflow no longer downloads the nightly-grid artifact into build/dist (the explicit 'path: build/dist' line was removed), which effectively avoids creating a nested build/dist/build/dist structure and aligns with downloading into the default location.
code diff:
@@ -192,7 +192,6 @@
uses: actions/download-artifact@v4
with:
name: nightly-grid
- path: build/dist
- name: Create nightly release
In .github/workflows/nightly.yml, change the download path for the nightly-grid artifact from build/dist to . to avoid creating a nested directory structure and ensure the release step finds the files.
.github/workflows/nightly.yml [191-195]
- name: Download grid packages
uses: actions/download-artifact@v4
with:
name: nightly-grid
- path: build/dist
+ path: .
[Suggestion processed]
Suggestion importance[1-10]: 9
__
Why: This is a critical bug fix. The upload-artifact action preserves the full path, so downloading to build/dist would create a nested build/dist/build/dist/ path. This would cause the subsequent release step to fail as it would not find any files to upload.
| High
|
✅ Avoid doubled output directories
Suggestion Impact:The commit removed the explicit `path: build/dist` from the download-artifact step, causing the artifact to be downloaded to the default location (workspace, effectively like `.`), which addresses the doubled output directory issue the suggestion targeted.
code diff:
uses: actions/download-artifact@v4
with:
name: release-packages
- path: build/dist
In .github/workflows/release.yml, change the download path for the
release-packages artifact from build/dist to . to prevent a nested directory issue that would cause the release creation to fail.
.github/workflows/release.yml [72-76]
- name: Download release packages
uses: actions/download-artifact@v4
with:
name: release-packages
- path: build/dist
+ path: .
[Suggestion processed]
Suggestion importance[1-10]: 9
__
Why: This is a critical bug fix. The upload-artifact action preserves the full path, so downloading to build/dist would create a nested build/dist/build/dist/ path. This would cause the ncipollo/release-action to fail as it would not find the packages for the draft release.
| High
|
| Incremental [*] |
✅ Capture complete patch diffs
Suggestion Impact:The workflow was changed to use `git diff --binary > changes.patch`, partially addressing the suggestion (binary handling). However, it did not add `git diff --cached --binary` nor the empty-file removal check.
code diff:
- name: Save git diff
if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
- run: git diff > changes.patch
+ run: git diff --binary > changes.patch
Modify the Save git diff step to capture both staged and unstaged changes, handle binary files, and avoid creating empty patch files. Update the run command to git diff --binary > changes.patch; git diff --cached --binary >>
changes.patch; [ -s changes.patch ] || rm -f changes.patch.
.github/workflows/bazel.yml [254-258]
- name: Save git diff
if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
- run: git diff > changes.patch
+ run: |
+ git diff --binary > changes.patch
+ git diff --cached --binary >> changes.patch
+ [ -s changes.patch ] || rm -f changes.patch
- name: Upload artifact
if: always() && inputs.artifact-name != ''
[Suggestion processed]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that git diff alone is insufficient as it misses staged changes, which are used by downstream jobs (git apply --index). The proposed change to include git diff --cached and --binary is a crucial fix for the correctness of the new reusable workflow.
| Medium
|
|
| |
✅ Suggestions up to commit 28ce7c2
| Category | Suggestion | Impact |
| Possible issue |
✅ Upload artifacts even on failure
Suggestion Impact:The workflow conditions for "Save git diff" and "Upload artifact" were updated to include always(), ensuring these steps run even if earlier steps fail. (The upload action version was also bumped to v5.)
code diff:
- name: Save git diff
- if: inputs.artifact-name != '' && inputs.artifact-path == ''
+ if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
run: git diff > changes.patch
- name: Upload artifact
- if: inputs.artifact-name != ''
- uses: actions/upload-artifact@v4
+ if: always() && inputs.artifact-name != ''
+ uses: actions/upload-artifact@v5
Add always() to the conditions for saving and uploading artifacts in bazel.yml to ensure they run even if the main run step fails.
.github/workflows/bazel.yml [254-264]
- name: Save git diff
- if: inputs.artifact-name != '' && inputs.artifact-path == ''
+ if: always() && inputs.artifact-name != '' && inputs.artifact-path == ''
run: git diff > changes.patch
- name: Upload artifact
- if: inputs.artifact-name != ''
+ if: always() && inputs.artifact-name != ''
uses: actions/upload-artifact@v4
with:
name: ${{ inputs.artifact-name }}
path: ${{ inputs.artifact-path || 'changes.patch' }}
retention-days: 6
if-no-files-found: ${{ inputs.artifact-path != '' && 'error' || 'warn' }}
[Suggestion processed]
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical flaw where artifacts are not generated if the main run step fails, which breaks several dependent jobs like auto-formatting. Adding always() is essential for the new workflow design to function as intended.
| High
|
Avoid failing on missing artifacts
In the commit-fixes job, add continue-on-error: true to the artifact download step and adjust the subsequent script to handle cases where the artifact is missing.
.github/workflows/ci-lint.yml [56-96]
commit-fixes:
name: Commit fixes
needs: format
if: failure() && github.event_name == 'pull_request'
runs-on: ubuntu-latest
permissions:
contents: write
actions: read
steps:
- name: Check Permissions
if: github.event.pull_request.head.repo.fork == true
run: |
echo "::error::Code needs formatting. Run ./scripts/format.sh locally and push changes."
exit 1
- name: Checkout PR
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }}
- name: Download format changes
uses: actions/download-artifact@v4
with:
name: format-changes
+ continue-on-error: true
- name: Apply and commit fixes
id: apply
run: |
- if [ -s changes.patch ]; then
+ if [ -f changes.patch ] && [ -s changes.patch ]; then
git apply changes.patch
rm changes.patch
git config --local user.name "Selenium CI Bot"
git config --local user.email "selenium-ci@users.noreply.github.com"
git add -A
git commit -m "Auto-format code"
echo "has_changes=true" >> "$GITHUB_OUTPUT"
else
- echo "::notice::No formatting changes needed."
+ echo "::notice::No formatting patch artifact found; nothing to apply."
+ echo "has_changes=false" >> "$GITHUB_OUTPUT"
fi
- name: Push fixes
if: steps.apply.outputs.has_changes == 'true'
run: |
git push
echo "::notice::Auto-formatted and pushed. New CI run will start."
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that the download-artifact step will fail if the artifact doesn't exist, halting the job. Adding continue-on-error: true and checking for the patch file's existence makes the commit-fixes job more robust and resilient to this scenario.
| Medium
|
Learned best practice |
Safely escape injected context strings
Use toJSON(...) to safely escape commit messages before putting them into a bash variable, and normalize whitespace so unexpected quotes/newlines in messages can’t break the script.
.github/workflows/ci-renovate-rbe.yml [19-38]
run: |
- COMMITS="${{ join(github.event.commits.*.message, ' ') }}"
+ COMMITS_JSON='${{ toJSON(join(github.event.commits.*.message, " ")) }}'
+ COMMITS="${COMMITS_JSON%\"}"; COMMITS="${COMMITS#\"}"
+ COMMITS="${COMMITS//\\n/ }"; COMMITS="${COMMITS//\\r/ }"
if [[ "$COMMITS" == *"[java]"* ]]; then
RULES_JVM_EXTERNAL_REPIN=1 bazel run @maven//:pin
fi
if [[ "$COMMITS" == *"[rust]"* ]]; then
CARGO_BAZEL_REPIN=true bazel run @crates//:all
fi
if [[ "$COMMITS" == *"[js]"* ]]; then
bazel run -- @pnpm//:pnpm install --dir "$PWD" --lockfile-only
fi
if [[ "$COMMITS" == *"[dotnet]"* ]]; then
./dotnet/update-deps.sh
fi
if [[ "$COMMITS" == *"[py]"* ]]; then
bazel run //py:requirements.update
fi
if [[ "$COMMITS" == *"[rb]"* ]]; then
./go rb:pin
fi
Suggestion importance[1-10]: 5
__
Why:
Relevant best practice - Add validation/escaping at GitHub Actions integration boundaries; avoid directly injecting untrusted context strings into shell scripts without safe encoding.
| Low
|
|
| |
✅ Suggestions up to commit 26be08a
| Category | Suggestion | Impact |
| Possible issue |
✅ Handle empty bazel targets file correctly
Suggestion Impact:The workflow step was changed to conditionally write the multiline targets output only when bazel-targets.txt is non-empty, and otherwise write `targets=` to GITHUB_OUTPUT, preventing an empty newline output.
code diff:
@@ -44,11 +44,15 @@
- name: Read targets
id: read
run: |
- {
- echo "targets<<EOF"
- cat bazel-targets.txt
- echo "EOF"
- } >> "$GITHUB_OUTPUT"
+ if [ -s bazel-targets.txt ]; then
+ {
+ echo "targets<<EOF"
+ cat bazel-targets.txt
+ echo "EOF"
+ } >> "$GITHUB_OUTPUT"
+ else
+ echo "targets=" >> "$GITHUB_OUTPUT"
+ fi
In the read-targets job, check if bazel-targets.txt is empty. If so, set the
targets output to an empty string to avoid it containing a newline, which could cause issues in downstream jobs.
.github/workflows/ci.yml [33-51]
read-targets:
name: Read Targets
needs: check
runs-on: ubuntu-latest
outputs:
targets: ${{ steps.read.outputs.targets }}
steps:
- name: Download targets
uses: actions/download-artifact@v4
with:
name: check-targets
- name: Read targets
id: read
run: |
- {
- echo "targets<<EOF"
- cat bazel-targets.txt
- echo "EOF"
- } >> "$GITHUB_OUTPUT"
+ if [ -s bazel-targets.txt ]; then
+ {
+ echo "targets<<EOF"
+ cat bazel-targets.txt
+ echo "EOF"
+ } >> "$GITHUB_OUTPUT"
+ else
+ echo "targets=" >> "$GITHUB_OUTPUT"
+ fi
[Suggestion processed]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that an empty bazel-targets.txt file leads to a job output with a newline, which can cause issues in downstream jobs. The proposed fix ensures the output is a truly empty string, which is a critical improvement for the correctness of the workflow logic.
| Medium
|
✅ Prevent job failure if no changes
Suggestion Impact:The workflow was changed to check whether changes.patch exists and is non-empty before applying it and performing git commit/push. The implementation differs from the suggestion by combining steps and by failing the job when the patch is missing/empty (exit 1) instead of skipping commit/push.
code diff:
+ - name: Apply and push
+ run: |
+ if [ -f changes.patch ] && [ -s changes.patch ]; then
+ git apply --index changes.patch
+ git config --local user.email "selenium-ci@users.noreply.github.com"
+ git config --local user.name "Selenium CI Bot"
+ git commit -m "update selenium manager version and rust changelog"
+ git push origin HEAD:rust-release-${{ inputs.version }} --force
+ else
+ echo "::error::No changes to apply for rust version update"
+ exit 1
+ fi
In the push-rust-version job, add a condition to check if changes.patch is non-empty before attempting to apply it and commit, preventing job failure when there are no changes.
.github/workflows/pre-release.yml [43-66]
push-rust-version:
name: Push Rust Version
needs: generate-rust-version
runs-on: ubuntu-latest
steps:
- name: Checkout repo
uses: actions/checkout@v4
with:
token: ${{ secrets.SELENIUM_CI_TOKEN }}
fetch-depth: 0
- name: Download rust version patch
uses: actions/download-artifact@v4
with:
name: rust-version
- name: Apply patch
- run: git apply --index changes.patch
+ run: |
+ if [ -s changes.patch ]; then
+ git apply --index changes.patch
+ echo "CHANGES_APPLIED=true" >> "$GITHUB_ENV"
+ fi
- name: Prep git
+ if: env.CHANGES_APPLIED == 'true'
run: |
git config --local user.email "selenium-ci@users.noreply.github.com"
git config --local user.name "Selenium CI Bot"
- name: Push changes
+ if: env.CHANGES_APPLIED == 'true'
run: |
git commit -m "update selenium manager version and rust changelog"
git push origin HEAD:rust-release-${{ inputs.version }} --force
[Suggestion processed]
Suggestion importance[1-10]: 7
__
Why: This is a valid improvement for workflow robustness, preventing a potential failure if no version changes are generated. This pattern of checking for a non-empty patch is used elsewhere in the PR, making this a consistent and valuable addition.
| Medium
|
| General |
✅ Fetch full git history
Suggestion Impact:The workflow was updated to support configuring actions/checkout's fetch-depth via a new reusable-workflow input, and the checkout step now uses that input. This enables callers to set fetch-depth to 0 for full history, addressing the underlying need, though it does not hardcode fetch-depth: 0 by default.
code diff:
@@ -81,6 +81,11 @@
required: false
type: boolean
default: false
+ fetch-depth:
+ description: Number of commits to fetch (0 for full history)
+ required: false
+ type: number
+ default: 1
jobs:
bazel:
@@ -99,6 +104,7 @@
uses: actions/checkout@v4
with:
ref: ${{ inputs.ref || github.ref }}
+ fetch-depth: ${{ inputs.fetch-depth }}
Add fetch-depth: 0 to the actions/checkout step in the bazel.yml reusable workflow to ensure the full git history is available for subsequent git operations.
.github/workflows/bazel.yml [98-101]
- name: Checkout source tree
uses: actions/checkout@v4
with:
ref: ${{ inputs.ref || github.ref }}
+ fetch-depth: 0
[Suggestion processed]
Suggestion importance[1-10]: 8
__
Why: This is a critical fix for the reusable workflow, as several calling workflows rely on git diff or commit ranges which require the full git history. Without fetch-depth: 0, these operations would fail or produce incorrect results.
| Medium
|
Learned best practice |
✅ Avoid swallowing patch-apply errors
Suggestion Impact:The workflow was changed to conditionally run `git apply --index changes.patch` only when the patch file exists and is non-empty, and removed the unconditional `|| echo "No changes to apply"` that would mask real git apply failures. If no patch is present, it now emits a notice and skips apply/commit/push.
code diff:
+ - name: Apply and push
run: |
- git config --local user.email "selenium-ci@users.noreply.github.com"
- git config --local user.name "Selenium CI Bot"
- - name: Push version changes
- run: |
- git commit -m "[build] Reset versions to nightly after ${{ needs.prepare.outputs.tag }} release" || echo "No changes to commit"
- git push
+ if [ -f changes.patch ] && [ -s changes.patch ]; then
+ git apply --index changes.patch
+ git config --local user.email "selenium-ci@users.noreply.github.com"
+ git config --local user.name "Selenium CI Bot"
+ git commit -m "[build] Reset versions to nightly after ${{ needs.prepare.outputs.tag }} release"
+ git push
+ else
+ echo "::notice::No version changes to apply"
+ fi
Replace the unconditional || echo ... with an explicit file existence/size check so real git apply failures fail the job instead of being silently ignored.
.github/workflows/release.yml [189-190]
- name: Apply patch
- run: git apply --index changes.patch || echo "No changes to apply"
+ shell: bash
+ run: |
+ if [ ! -f changes.patch ] || [ ! -s changes.patch ]; then
+ echo "No changes to apply"
+ exit 0
+ fi
+ git apply --index changes.patch
[Suggestion processed]
Suggestion importance[1-10]: 5
__
Why:
Relevant best practice - Add explicit validation/availability guards at integration boundaries (e.g., downloaded artifacts/files) and avoid swallowing errors from critical steps.
| Low
|
|
| |