Skip to content

Conversation

@withinfocus
Copy link
Contributor

🎟️ Tracking

Testing of #465 in the field.

📔 Objective

Workflow name clashes exist since a common scan.yml from the template calls the independent scan reusable workflows. Adds a clarifying name in the concurrency group.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@withinfocus withinfocus marked this pull request as ready for review October 30, 2025 14:49
@withinfocus withinfocus requested a review from a team as a code owner October 30, 2025 14:49
Copilot AI review requested due to automatic review settings October 30, 2025 14:49
@claude
Copy link

claude bot commented Oct 30, 2025

Claude finished @withinfocus's task —— View job


PR Review: Disambiguate the two scan reusable workflows

Summary of Changes

This PR adds disambiguating suffixes (-checkmarx and -sonar) to the concurrency group names in two reusable workflow files to prevent workflow name clashes when called from a common scan.yml template.

Changes:

  • .github/workflows/_checkmarx.yml:23 - Added -checkmarx- suffix to concurrency group
  • .github/workflows/_sonar.yml:27 - Added -sonar- suffix to concurrency group

Critical Issues

None found.


Suggested Improvements

🤔 Consider the impact of ${{ github.workflow }}

The concurrency group uses ${{ github.workflow }}, which resolves to the calling workflow's name (not the reusable workflow's name) when used in a reusable workflow context.

Looking at scan.yml:1, the calling workflow is named "Scan". This means:

  • Both concurrency groups would have previously resolved to: <repo>-Scan-<pr/ref>
  • After this change, they resolve to:
    • Checkmarx: <repo>-Scan-checkmarx-<pr/ref>
    • Sonar: <repo>-Scan-sonar-<pr/ref>

This achieves the stated objective of disambiguation. However, there's a subtle consideration:

If multiple repositories use these reusable workflows but call them from workflows with different names (e.g., one repo calls it from "Scan", another from "Security Scan"), the concurrency groups would differ based on the calling workflow name. This is likely the intended behavior, but worth confirming.

Alternative approach (if you want concurrency based only on the reusable workflow)

If you want concurrency to be independent of the calling workflow name, you could use the workflow file name instead:

# _checkmarx.yml
concurrency:
  group: ${{ github.repository }}-checkmarx-${{ github.event.pull_request.number || github.head_ref || github.ref }}
  cancel-in-progress: true

This would make the concurrency group the same regardless of which workflow calls it. However, the current approach (using github.workflow) is more granular and allows the same reusable workflow to run concurrently when called from different parent workflows, which is often desirable.

📝 Documentation consideration

Consider adding a comment above the concurrency groups explaining why the suffixes are needed, to prevent future maintainers from removing them thinking they're redundant:

# Include workflow-specific suffix to prevent concurrency clashes when
# multiple reusable workflows are called from the same parent workflow
concurrency:
  group: ${{ github.repository }}-${{ github.workflow }}-checkmarx-${{ github.event.pull_request.number || github.head_ref || github.ref }}
  cancel-in-progress: true

Good Practices Observed

  • ✅ Minimal, surgical change that directly addresses the stated problem
  • ✅ Consistent naming pattern applied to both workflows (-checkmarx- and -sonar-)
  • ✅ No changes to functionality, only concurrency grouping logic
  • ✅ Maintains workflow linter compliance (no rule violations introduced)

Workflow Linter Compliance

✅ All relevant linter rules remain satisfied:

  • name_exists.py - Both workflows have names defined
  • name_capitalized.py - Names are properly capitalized
  • permissions_exist.py - Permissions are explicitly set
  • pinned_job_runner.py - Runners are pinned to ubuntu-24.04
  • step_pinned.py - External actions are pinned to commit hashes

Action Items

  • Verify that the use of ${{ github.workflow }} (which resolves to the calling workflow's name) is intentional and provides the desired concurrency behavior across all repositories using these reusable workflows
  • Consider adding explanatory comments above the concurrency groups (optional but recommended for maintainability)
  • Test in the field as mentioned in the PR description to confirm the change resolves the workflow name clashes

Overall Assessment

This is a well-targeted fix that directly addresses the concurrency group collision issue introduced in PR #465. The changes are minimal, consistent, and maintain all existing functionality while preventing workflow cancellation conflicts. The solution is sound and should effectively resolve the name clashes when scan.yml calls both reusable workflows.


Copy link

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 updates the concurrency group identifiers for reusable GitHub Actions workflows to prevent unintended job cancellations. The change adds workflow-specific suffixes to concurrency groups, ensuring that different workflow types (Sonar and Checkmarx) can run independently without interfering with each other.

  • Added unique identifiers (-sonar- and -checkmarx-) to concurrency group names to distinguish between different workflow types

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
.github/workflows/_sonar.yml Added -sonar- suffix to concurrency group to uniquely identify Sonar workflow runs
.github/workflows/_checkmarx.yml Added -checkmarx- suffix to concurrency group to uniquely identify Checkmarx workflow runs

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Oct 30, 2025

Logo
Checkmarx One – Scan Summary & Detailsc3657e8e-5011-490d-9eac-ad55960cd065

Great job! No new security vulnerabilities introduced in this pull request

@withinfocus withinfocus merged commit 5108818 into main Oct 30, 2025
19 of 20 checks passed
@withinfocus withinfocus deleted the disambiguate branch October 30, 2025 15:05
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