Skip to content

fix: skip coverage grade comment on fork PRs - #14299

Open
PrashantBhanage wants to merge 2 commits into
apache:mainfrom
PrashantBhanage:fix/sonar-check-workflow
Open

PrashantBhanage wants to merge 2 commits into
apache:mainfrom
PrashantBhanage:fix/sonar-check-workflow

Conversation

@PrashantBhanage

@PrashantBhanage PrashantBhanage commented Oct 3, 2026 •

Copy link
Copy Markdown

Description

This PR skips the "Post Coverage Grade Comment on PR" step on fork PRs. On forks the token is read-only, so posting the comment fails with a 403. The step now uses the same condition as the "Upload to SonarQube" step.

It also falls back to N/A when branch coverage is not available.

This change was separated from #13490 as requested by @DaanHoogland.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

N/A

How Has This Been Tested?

Checked the diff against apache:main. Only .github/workflows/sonar-check.yml changes. No application code or unit tests were changed.

How did you try to break this feature and the system with this change?

On fork PRs the comment step is now skipped, so it no longer tries to post with the read-only token. Fork PRs will not get a coverage comment. On PRs from the main repo the step runs as before.

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clgtm

Comment thread .github/workflows/sonar-check.yml Outdated
const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`;
const marker = '<!-- coverage-grade-bot -->';

if (!grade || grade === 'N/A') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

when does the grade come through empty here? the script exits before writing it when the report is missing, so this step doesnt run then

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

when does the grade come through empty here? the script exits before writing it when the report is missing, so this step doesnt run then

You’re right, the grade is never empty when this step runs. Removed the guard and added the same fork check the Sonar upload step uses, so the comment step is skipped on fork PRs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

that covers it, thanks

Comment thread .github/workflows/sonar-check.yml Outdated
const marker = '<!-- coverage-grade-bot -->';

if (!grade || grade === 'N/A') {
console.log('Coverage grade not available, skipping comment');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

does this fix the 403 from #13490? on a fork PR the grade is there, so posting the comment still fails with the read-only token

@PrashantBhanage PrashantBhanage changed the title fix: skip coverage grade comment when grade unavailable fix: skip coverage grade comment on fork PRs Oct 19, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants