Skip to content

ci: handle cancelled post-merge verification#19590

Open
Jayant-kernel wants to merge 1 commit into
kubestellar:mainfrom
Jayant-kernel:fix/post-merge-cancel-status
Open

ci: handle cancelled post-merge verification#19590
Jayant-kernel wants to merge 1 commit into
kubestellar:mainfrom
Jayant-kernel:fix/post-merge-cancel-status

Conversation

@Jayant-kernel

Copy link
Copy Markdown
Contributor

Related to #19558 and #19568.

Summary of Changes

  • Treat cancelled post-merge verification runs as cancelled, not failed.
  • Prevent superseded runs from commenting on the original issue as a Playwright failure.
  • Keep the existing regression issue flow unchanged for real Playwright failures.

Changes Made

  • Updated .github/workflows/post-merge-verify.yml result handling for needs.run-tests.result == cancelled.
  • Kept this CI-only; no runtime code changed.

Checklist

  • I have reviewed the project's contribution guidelines
  • Workflow-only change; no app tests required
  • All commits are signed with DCO (git commit -s)

Signed-off-by: Jayant <212013719+Jayant-kernel@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 25, 2026 10:12
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Jun 25, 2026
@netlify

netlify Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploy Preview for kubestellarconsole canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 5c29025
🔍 Latest deploy log https://app.netlify.com/projects/kubestellarconsole/deploys/6a3cff111eb171000835699a

@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign clubanderson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubestellar-prow kubestellar-prow Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 25, 2026

Copilot AI 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.

Pull request overview

This PR updates the post-merge Playwright verification workflow to correctly classify cancelled (superseded) runs as cancelled instead of failed, preventing noisy “Playwright failed” comments/regression issue creation when a run is cancelled due to newer pushes. No runtime/application code is changed; build/lint are validated by CI on the PR.

Changes:

  • Add explicit handling for needs.run-tests.result == cancelled in the reporting step.
  • Ensure cancelled runs don’t fall through to the “failed” path that triggers issue/regression creation.

@clubanderson

Copy link
Copy Markdown
Collaborator

✅ All CI checks passing. Ready to merge — requires admin merge due to workflow file changes (OAuth scope limitation).

@clubanderson

Copy link
Copy Markdown
Collaborator

⚠️ Merge blocked: This PR modifies .github/workflows/post-merge-verify.yml. The OAuth app token lacks workflow scope, so automated merge is not possible. Requires manual merge by a maintainer with workflow permissions.

All blocking CI checks (build-gate) are passing. Ready for manual squash-merge.

@clubanderson

Copy link
Copy Markdown
Collaborator

🔍 Quality Review

Finding: The cancelled status handling is a valid fix that prevents spurious regression issues from being filed. This directly improves CI signal-to-noise ratio.

Testing concern — edge case not covered: When cancel-in-progress: true fires mid-run, the run-tests job may not set its result output at all (since the step never ran). The report job then sees:

  • RESULT="" (empty)
  • JOB_RESULT="cancelled"

The new elif [ "$JOB_RESULT" = "cancelled" ] branch catches this correctly ✅

One additional gap: If the wait-for-deploy job is cancelled (e.g., concurrency group eviction happens during the polling loop), the run-tests job's if condition (needs.extract-context.outputs.spec_files != '') doesn't account for this — it would proceed with tests against possibly stale production. Consider adding && needs.wait-for-deploy.result == 'success' to the run-tests job condition if strict deploy verification is desired.

Otherwise, LGTM from a test-infrastructure perspective.


Review by quality agent (ACMM L4/L6 — full mode)

@clubanderson

Copy link
Copy Markdown
Collaborator

Quality Review ✅

Reviewed the workflow changes — the cancelled state handling is correct:

  • needs.run-tests.result == 'cancelled' correctly maps to STATUS="cancelled" with ⏹️ icon
  • Prevents false regression issues when concurrent runs cancel each other (via cancel-in-progress: true)
  • No functional code changes, CI-only — no additional test coverage needed

LGTM from quality perspective.


Quality agent (ACMM L4/L6 — full mode)

@clubanderson clubanderson left a 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.

Quality Review

Correctly handles the cancelled result status to prevent cancelled runs from being misreported as failures. Small, targeted fix.

One minor note: the cancelled status handling is correctly placed before the else (failed) fallback, so it won't mask real failures. No quality concerns.

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

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants