Skip to content

Conversation

@titusfortner
Copy link
Member

💥 What does this PR do?

Significantly simplify the nightly workflow; follow-on to pre-release.yml and release.yml simplifications

  • Adding a prepare job to centralize input parsing and outputs (language, artifact-name, artifact-path, release-in-progress)
  • Removing separate per-language jobs (ruby, python, java, dotnet, javascript, grid) in favor of a single nightly-release job
  • Removing 'grid' as a separate language option since java:release already includes grid via java:package

🔧 Implementation Notes

There's no need for a separate grid job - the grid artifacts are included when java runs

💡 Additional Considerations

We'll see how it works tonight

🔄 Types of changes

  • Cleanup (formatting, renaming)

- Reduce from 254 to 116 lines by removing per-language jobs
- Add prepare job to centralize input parsing and outputs
- Remove 'grid' as separate option (java:release includes grid via java:package)
- Use single nightly-release job with ./go {lang}:release nightly
- release-grid runs only when artifact-name is set (all/java)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@titusfortner titusfortner requested a review from Copilot January 25, 2026 20:13
@qodo-code-review
Copy link
Contributor

User description

💥 What does this PR do?

Significantly simplify the nightly workflow; follow-on to pre-release.yml and release.yml simplifications

  • Adding a prepare job to centralize input parsing and outputs (language, artifact-name, artifact-path, release-in-progress)
  • Removing separate per-language jobs (ruby, python, java, dotnet, javascript, grid) in favor of a single nightly-release job
  • Removing 'grid' as a separate language option since java:release already includes grid via java:package

🔧 Implementation Notes

There's no need for a separate grid job - the grid artifacts are included when java runs

💡 Additional Considerations

We'll see how it works tonight

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Consolidate nightly workflow from 254 to 116 lines

  • Replace per-language jobs with single nightly-release job

  • Add prepare job to centralize input parsing and outputs

  • Remove grid as separate language option (included via java:release)

  • Simplify failure notification to single on-failure job


Diagram Walkthrough

flowchart LR
  A["Workflow Inputs"] --> B["prepare job"]
  B --> C["Parse language & artifacts"]
  B --> D["Check release ruleset"]
  C --> E["nightly-release job"]
  D --> E
  E --> F["release-grid job"]
  E --> G["on-failure job"]
  F --> G
Loading

File Walkthrough

Relevant files
Configuration changes
nightly.yml
Consolidate nightly workflow jobs and simplify structure 

.github/workflows/nightly.yml

  • Removed version input parameter and grid language option
  • Added prepare job to centralize input parsing and artifact
    configuration
  • Consolidated six per-language jobs (ruby, python, java, dotnet,
    javascript, grid) into single nightly-release job
  • Removed six per-language failure notification jobs in favor of unified
    on-failure job
  • Updated release-grid to depend on prepare and nightly-release with
    conditional execution
+34/-172

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Jan 25, 2026
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 25, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Command injection

Description: The workflow propagates inputs.language (including via workflow_call) into
needs.prepare.outputs.language without validation and then uses it to build a shell
command (./go ${{ needs.prepare.outputs.language }}:release nightly), which could enable
command/argument injection if an untrusted caller supplies a crafted value outside the
workflow_dispatch choice list.
nightly.yml [41-74]

Referred Code
- name: Parse inputs
  id: parse
  env:
    LANGUAGE: ${{ inputs.language }}
  run: |
    LANG="${LANGUAGE:-all}"
    echo "language=$LANG" >> "$GITHUB_OUTPUT"
    if [[ "$LANG" == "all" || "$LANG" == "java" ]]; then
      echo "artifact-name=nightly-grid" >> "$GITHUB_OUTPUT"
      echo "artifact-path=build/dist/*.*" >> "$GITHUB_OUTPUT"
    fi
- name: Check if release ruleset is active
  id: check
  env:
    GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
  run: |
    set -euo pipefail
    if ! ENFORCEMENT=$(gh api /repos/${{ github.repository }}/rulesets/11911909 --jq '.enforcement' 2>/dev/null); then
      echo "::warning::Failed to check release ruleset, assuming release in progress"
      echo "release-in-progress=true" >> "$GITHUB_OUTPUT"
      exit 0


 ... (clipped 13 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing output defaults: The prepare step only sets artifact-name and artifact-path for all/java, so other
languages may pass empty values to nightly-release and cause runtime failures depending on
how ./.github/workflows/bazel.yml consumes these inputs.

Referred Code
- name: Parse inputs
  id: parse
  env:
    LANGUAGE: ${{ inputs.language }}
  run: |
    LANG="${LANGUAGE:-all}"
    echo "language=$LANG" >> "$GITHUB_OUTPUT"
    if [[ "$LANG" == "all" || "$LANG" == "java" ]]; then
      echo "artifact-name=nightly-grid" >> "$GITHUB_OUTPUT"
      echo "artifact-path=build/dist/*.*" >> "$GITHUB_OUTPUT"
    fi
- name: Check if release ruleset is active
  id: check
  env:
    GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
  run: |
    set -euo pipefail
    if ! ENFORCEMENT=$(gh api /repos/${{ github.repository }}/rulesets/11911909 --jq '.enforcement' 2>/dev/null); then
      echo "::warning::Failed to check release ruleset, assuming release in progress"
      echo "release-in-progress=true" >> "$GITHUB_OUTPUT"
      exit 0


 ... (clipped 14 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 25, 2026

PR Code Suggestions ✨

Latest suggestions up to a315e68

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore modified manifest after publish

Restore the original javascript/selenium-webdriver/package.json file after the
publish step to avoid permanent mutations that could affect subsequent
non-nightly releases.

rake_tasks/node.rake [91-100]

-if nightly
-  puts 'Updating Node version to nightly...'
-  Rake::Task['node:version'].invoke('nightly')
-  setup_github_npm_auth unless dry_run
-else
-  setup_npm_auth unless dry_run
+original_package_json = nil
+package_json_path = 'javascript/selenium-webdriver/package.json'
+
+begin
+  if nightly
+    puts 'Updating Node version to nightly...'
+    Rake::Task['node:version'].invoke('nightly')
+    unless dry_run
+      original_package_json = File.read(package_json_path)
+      setup_github_npm_auth
+    end
+  else
+    setup_npm_auth unless dry_run
+  end
+
+  puts dry_run ? 'Running Node package dry-run...' : 'Running Node package release...'
+  target = '//javascript/selenium-webdriver:selenium-webdriver.publish'
+ensure
+  File.write(package_json_path, original_package_json) if original_package_json
 end
 
-puts dry_run ? 'Running Node package dry-run...' : 'Running Node package release...'
-target = '//javascript/selenium-webdriver:selenium-webdriver.publish'
-
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical bug where package.json is permanently modified, which would leave the repository in a dirty state and cause subsequent non-nightly releases to fail or publish incorrectly.

High
Incremental [*]
Modify package.json via JSON

Modify package.json by parsing it as JSON and updating the relevant keys, rather
than using brittle string substitutions.

rake_tasks/node.rake [45-49]

 package_json = 'javascript/selenium-webdriver/package.json'
-content = File.read(package_json)
-content = content.gsub('https://registry.npmjs.org/', 'https://npm.pkg.github.com')
-content = content.gsub('"name": "selenium-webdriver"', '"name": "@seleniumhq/selenium-webdriver"')
-File.write(package_json, content)
+json = JSON.parse(File.read(package_json))
+json['name'] = '@seleniumhq/selenium-webdriver'
+json['publishConfig'] ||= {}
+json['publishConfig']['registry'] = 'https://npm.pkg.github.com'
+File.write(package_json, JSON.pretty_generate(json) + "\n")
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This is an excellent suggestion that replaces a fragile string replacement on a JSON file with a robust method of parsing, modifying, and re-serializing the JSON, preventing potential future breakages.

Medium
Prevent duplicate npmrc entries

To prevent duplicate entries in .npmrc, modify the script to remove any existing
GitHub Packages configuration before appending the new settings.

rake_tasks/node.rake [37-42]

 if File.exist?(npmrc)
-  File.open(npmrc, 'a') { |f| f.puts("\n#{npmrc_content}") }
+  existing = File.read(npmrc).lines.reject { |line|
+    line.start_with?('//npm.pkg.github.com/:_authToken=') ||
+      line.start_with?('@seleniumhq:registry=') ||
+      line.start_with?('always-auth=')
+  }.join
+  File.write(npmrc, existing.rstrip + "\n\n#{npmrc_content}\n")
 else
   File.write(npmrc, "#{npmrc_content}\n")
 end
 File.chmod(0o600, npmrc)
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that appending to .npmrc can lead to duplicate entries, and proposes a more robust, idempotent approach by cleaning up old entries before adding new ones.

Low
  • More

Previous suggestions

Suggestions up to commit ef6cb2d
CategorySuggestion                                                                                                                                    Impact
High-level
Use a matrix strategy instead

Instead of consolidating all language releases into a single, sequential job,
use a dynamic matrix strategy. This will restore parallel execution for the
'all' languages option, reducing runtime and providing better failure isolation
with per-language job statuses.

Examples:

.github/workflows/nightly.yml [31-75]
  prepare:
    name: Prepare
    runs-on: ubuntu-latest
    if: github.event.repository.fork == false
    outputs:
      language: ${{ steps.parse.outputs.language }}
      artifact-name: ${{ steps.parse.outputs.artifact-name }}
      artifact-path: ${{ steps.parse.outputs.artifact-path }}
      release-in-progress: ${{ steps.check.outputs.release-in-progress }}
    steps:

 ... (clipped 35 lines)

Solution Walkthrough:

Before:

jobs:
  prepare:
    outputs:
      language: ${{ steps.parse.outputs.language }}
    steps:
      - id: parse
        run: |
          LANG="${LANGUAGE:-all}"
          echo "language=$LANG" >> "$GITHUB_OUTPUT"
          ...
  nightly-release:
    needs: prepare
    ...
    run: ./go ${{ needs.prepare.outputs.language }}:release nightly

After:

jobs:
  prepare:
    outputs:
      matrix: ${{ steps.parse.outputs.matrix }}
    steps:
      - id: parse
        run: |
          if [[ "${LANGUAGE:-all}" == "all" ]]; then
            echo 'matrix=["java", "ruby", "python", ...]' >> "$GITHUB_OUTPUT"
          else
            echo 'matrix=["${LANGUAGE}"]' >> "$GITHUB_OUTPUT"
          fi
  nightly-release:
    needs: prepare
    strategy:
      matrix:
        language: ${{ fromJson(needs.prepare.outputs.matrix) }}
    ...
    run: ./go ${{ matrix.language }}:release nightly
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a major regression in the PR, the loss of parallel execution, and proposes a superior matrix-based solution that maintains simplification while restoring parallelism and improving job monitoring.

High
Possible issue
Fix go command syntax

Correct the go command syntax in the nightly-release job from release nightly to
release[nightly] to match the expected format.

.github/workflows/nightly.yml [72]

-run: ./go ${{ needs.prepare.outputs.language }}:release nightly
+run: ./go ${{ needs.prepare.outputs.language }}:release[nightly]
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a syntax error in the refactored go command that would cause the nightly-release job to fail for all languages. Fixing this is critical for the workflow to function as intended.

High
Fix broken Slack notification title

In the on-failure job, remove release-grid from the needs list to prevent the
Slack notification title from being broken when release-grid is skipped.

.github/workflows/nightly.yml [99-115]

 on-failure:
   name: Notify on Failure
   runs-on: ubuntu-latest
-  needs: [prepare, nightly-release, release-grid]
+  needs: [prepare, nightly-release]
   if: failure()
   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: Nightly ${{ needs.prepare.outputs.language }} Release failed
         MSG_MINIMAL: actions url
         SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK_URL }}
Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a subtle bug in GitHub Actions logic where a skipped job in needs can cause output access to fail, leading to uninformative notifications. The fix is correct and significantly improves the reliability of failure reporting.

Medium
General
Avoid using a hardcoded ruleset ID

Replace the hardcoded ruleset ID 11911909 in the GitHub API call with a dynamic
lookup by the ruleset name "Release In Progress Access" to improve robustness.

.github/workflows/nightly.yml [56-63]

 run: |
   set -euo pipefail
-  if ! ENFORCEMENT=$(gh api /repos/${{ github.repository }}/rulesets/11911909 --jq '.enforcement' 2>/dev/null); then
+  RULESET_ID=$(gh api /repos/${{ github.repository }}/rulesets --jq '.[] | select(.name == "Release In Progress Access") | .id' 2>/dev/null)
+  if [[ -z "$RULESET_ID" ]]; then
+    echo "::warning::Could not find 'Release In Progress Access' ruleset. Assuming release is in progress."
+    echo "release-in-progress=true" >> "$GITHUB_OUTPUT"
+    exit 0
+  fi
+
+  if ! ENFORCEMENT=$(gh api /repos/${{ github.repository }}/rulesets/${RULESET_ID} --jq '.enforcement' 2>/dev/null); then
     echo "::warning::Failed to check release ruleset, assuming release in progress"
     echo "release-in-progress=true" >> "$GITHUB_OUTPUT"
     exit 0
   fi
   echo "release-in-progress=$([[ "$ENFORCEMENT" == "active" ]] && echo 'true' || echo 'false')" >> "$GITHUB_OUTPUT"
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that using a hardcoded ID is brittle and proposes a more robust solution by fetching the ruleset ID by name, which improves the workflow's maintainability.

Medium
Learned
best practice
Validate and normalize workflow inputs

Validate and normalize inputs.language (trim + allowlist) and always set
artifact-name/artifact-path outputs (possibly empty) to avoid accidental command
injection and missing-output edge cases.

.github/workflows/nightly.yml [41-51]

 - name: Parse inputs
   id: parse
   env:
     LANGUAGE: ${{ inputs.language }}
   run: |
-    LANG="${LANGUAGE:-all}"
+    set -euo pipefail
+
+    LANG="$(echo "${LANGUAGE:-all}" | xargs)"
+    case "$LANG" in
+      all|java|ruby|python|dotnet|javascript) ;;
+      *) echo "::error::Unsupported language: $LANG"; exit 1 ;;
+    esac
+
     echo "language=$LANG" >> "$GITHUB_OUTPUT"
+
+    # default outputs (empty means "not applicable")
+    echo "artifact-name=" >> "$GITHUB_OUTPUT"
+    echo "artifact-path=" >> "$GITHUB_OUTPUT"
+
     if [[ "$LANG" == "all" || "$LANG" == "java" ]]; then
       echo "artifact-name=nightly-grid" >> "$GITHUB_OUTPUT"
       echo "artifact-path=build/dist/*.*" >> "$GITHUB_OUTPUT"
     fi
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries (GitHub Actions inputs/env) before using them in shell commands and outputs.

Low
Use outputs instead of hardcoding

Use the computed artifact-name/artifact-path outputs instead of hardcoding
nightly-grid, and make the if guard explicit to avoid accidental
truthiness/empty-string behavior.

.github/workflows/nightly.yml [77-89]

 release-grid:
   name: Release Grid
   needs: [prepare, nightly-release]
-  if: needs.prepare.outputs.artifact-name
+  if: needs.prepare.outputs.artifact-name != ''
   runs-on: ubuntu-latest
   permissions:
     contents: write
   steps:
     - name: Download grid packages
       uses: actions/download-artifact@v4
       with:
-        name: nightly-grid
+        name: ${{ needs.prepare.outputs.artifact-name }}
         path: build/dist
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Reduce duplication and avoid hardcoding shared values by using a single source of truth (outputs) across jobs.

Low

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies the nightly GitHub Actions workflow by consolidating multiple per-language jobs into a single release job with a preparatory input-parsing step, and by removing the dedicated “grid” language option.

Changes:

  • Added a prepare job to parse inputs and expose shared outputs (language, artifact info, release-in-progress flag).
  • Replaced per-language nightly jobs with a single nightly-release job that runs ./go <language>:release nightly.
  • Removed the separate grid language option and consolidated failure notifications into one on-failure job.

Move the nightly-specific npm configuration from the workflow into the
rake task. This configures:
- GitHub Packages registry and auth token via ~/.npmrc
- Package name change to @seleniumhq/selenium-webdriver
- Registry URL change to npm.pkg.github.com

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

@titusfortner titusfortner merged commit 371a719 into trunk Jan 25, 2026
32 checks passed
@titusfortner titusfortner deleted the consolidate-nightly-workflow branch January 25, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants