Skip to content

Conversation

@mahlau-flex
Copy link
Contributor

@mahlau-flex mahlau-flex commented Oct 7, 2025

fixed a bug in the commitlint check in github actions. The variable github.event_name was missing the ${{ }} around them to make it a proper string comparison.

Another check has the same problem and therefore never run as well. I deleted the other check, because @yaugenst and I believe that this check is superficial anyways (in the past it definitely never ran because of this bug).

related to #2862.

Greptile Overview

Updated On: 2025-10-07 06:39:35 UTC

Summary

This PR fixes a critical bug in the GitHub Actions workflow for the Tidy3D Python client where commit message linting was never actually executing due to incorrect template syntax. The main issue was that `github.event_name` variables were used as bare strings instead of being properly wrapped in `${{ }}` brackets, causing string comparisons to always evaluate as false.

The changes update the workflow to properly check if the event is a merge_group before running commit message validation, rename a job for consistency (check-commit-messageslint-commit-messages), remove a problematic approval check that was preventing execution, and add debugging output to provide visibility into workflow operation. This ensures that commit message linting now runs as intended during merge group events, aligning with the established pattern of validating commits before merges while avoiding disruption during regular PR development.

The fix addresses a longstanding issue where the commit linting functionality existed in the codebase but was silently failing, which could have led to inconsistent commit message standards over time.

Important Files Changed

Changed Files
Filename Score Overview
.github/workflows/tidy3d-python-client-tests.yml 4/5 Fixed critical GitHub Actions syntax bugs in commit message linting, renamed job for consistency, removed problematic checks, and added debugging output

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it fixes a clear syntax bug and improves workflow reliability
  • Score reflects straightforward syntax fixes to GitHub Actions workflow with clear before/after behavior
  • Pay close attention to the GitHub Actions workflow file to ensure the commit linting logic works as expected

Sequence Diagram

sequenceDiagram
    participant User
    participant GitHub
    participant DetermineScope as "determine-test-scope"
    participant Approval as "PR Approval Check"
    participant Lint as "lint"
    participant CommitLint as "lint-commit-messages" 
    participant SchemaVerify as "verify-schema-change"
    participant LocalTests as "local-tests"
    participant RemoteTests as "remote-tests"
    participant PRRequirements as "pr-requirements-pass"

    User->>GitHub: "Creates/Updates Pull Request"
    GitHub->>DetermineScope: "Trigger workflow"
    
    alt Pull Request Event
        DetermineScope->>Approval: "Check PR approval status"
        Approval->>DetermineScope: "Return approval state"
    end
    
    DetermineScope->>DetermineScope: "Determine test scope based on event/approval"
    DetermineScope->>GitHub: "Output local_tests and remote_tests flags"
    
    par Conditional Job Execution
        alt Local or Remote Tests Required
            GitHub->>Lint: "Run linting checks"
            Lint->>Lint: "Run ruff format and check"
            Lint->>GitHub: "Return lint results"
        end
        
        alt Merge Group Event
            GitHub->>CommitLint: "Check commit messages"
            CommitLint->>CommitLint: "Validate conventional commits format"
            CommitLint->>GitHub: "Return commit lint results"
        end
        
        alt Local or Remote Tests Required
            GitHub->>SchemaVerify: "Verify schema changes"
            SchemaVerify->>SchemaVerify: "Check schema consistency and versioning"
            SchemaVerify->>GitHub: "Return schema verification results"
        end
        
        alt Local Tests Required
            GitHub->>LocalTests: "Run local tests"
            LocalTests->>LocalTests: "Execute test suite with coverage"
            LocalTests->>GitHub: "Return local test results and coverage"
        end
        
        alt Remote Tests Required  
            GitHub->>RemoteTests: "Run remote tests"
            RemoteTests->>RemoteTests: "Execute tests across multiple platforms/versions"
            RemoteTests->>GitHub: "Return remote test results"
        end
    end
    
    GitHub->>PRRequirements: "Aggregate all job results"
    PRRequirements->>PRRequirements: "Validate all required jobs passed"
    PRRequirements->>GitHub: "Final PR status"
    GitHub->>User: "Workflow completion status"
Loading

@mahlau-flex mahlau-flex requested review from daquinteroflex and yaugenst-flex and removed request for daquinteroflex October 7, 2025 06:38
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Thanks @mahlau-flex!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

@yaugenst-flex yaugenst-flex added this pull request to the merge queue Oct 7, 2025
Merged via the queue into develop with commit 38fd381 Oct 7, 2025
22 checks passed
@yaugenst-flex yaugenst-flex deleted the chore/fix-commitlint branch October 7, 2025 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants