Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Discussion options

@PatrickRitchie, I wanted to put an idea on your radar. The build-test-coverage workflow (.github/workflows/dotnet.yml) was disabled at the workflow level a few minutes after #139 merged—state: disabled_manually per the GitHub API, with the timestamp matching the master push event for the merge. As a result, the rewritten workflow that landed with #139 has actually never executed: the workflow has zero recorded runs since the rewrite. Ready-for-review PRs in the campaign queue (#141 today; #145, #143, #147, #146, #144, #142, #148, #140, #149, #150, #155 as they flip ready) all show an empty status-check rollup as a knock-on effect.

I don't know whether the disable was a deliberate gate ('I want to verify the new YAML before letting it run automatically') or an interim concern about something specific. Either way, the suggestion is: re-enable the workflow so master pushes and ready PRs get a green-or-red CI signal. The maintenance burden is low—I'd be happy to help.

Current state—already configured for ready-only, no edit needed

It's worth highlighting because it's a small surprise: the rewritten YAML in #139 already excludes draft PRs from CI. The on: block lists pull_request: types: [opened, synchronize, reopened, ready_for_review] and the build-and-test job carries if: github.event_name == 'push' || github.event.pull_request.draft == false. So if you re-enable as-is:

  • master pushes (post-merge): trigger CI.
  • Ready-for-review PRs: trigger CI on the flip from draft to ready, on every subsequent push to the PR branch, on reopen.
  • Draft PRs: skipped entirely. The campaign currently has ~12 draft PRs that get force-pushed multiple times per day during history rewrites—those are filtered out, no runner time spent.
  • Drafts that flip to ready: the ready_for_review event triggers CI automatically; no manual re-run needed.

So no YAML edit is required to get the 'ready PRs only' behaviour—just the re-enable. (The previous-generation YAML, which has been on the repo since 2022, gated pull_request on branches: [main] instead of master, so it had effectively been a no-op on the PR side for the entire history of the repo. The rewrite in #139 fixed that branch name plus added the matrix and the draft gate.)

Pros

  • Reviewer confidence on ready PRs. The merge button shows verified green checks instead of an empty rollup. You don't have to clone + test locally to confirm a diff builds across Ubuntu + Windows; CI has already done it.
  • Per-PR regression detection. A failure surfaces at PR open or push, not post-merge on master. With the campaign's queue size, that's the difference between catching a regression in isolation vs. having to bisect across multiple landings.
  • Master push runs. Even setting PRs aside, master itself currently has zero CI signal (the rewrite landed but never ran). Re-enabling means every merged PR gets verified on master immediately afterwards—useful as a 'did the merge introduce something the per-PR run missed' check.
  • Compliance-harness fit. Once the layered XSD / cross-impl / regression harness in PR test: comprehensive test-suite overhaul (100% coverage + E2E + compliance harness) #150 lands, having CI run it on every ready PR catches v2.6 / v2.7 spec compliance regressions automatically. The harness is most useful when CI runs it on PRs.

Cons / things to weigh

  • First-time-contributor approval. GitHub already requires the maintainer to manually approve the first workflow run on a fork PR from a new contributor—that gate stays in place even if the workflow is enabled. If you'd rather make it more aggressive across the board, Settings → Actions → General → Fork pull request workflows from outside collaborators lets you require approval for every outside contributor, not just first-timers.
  • Matrix breadth. The new workflow runs the full solution build + tests on ubuntu-latest and windows-latest (.NET 8 + 9). If wall-clock time becomes a queue concern—e.g. multiple pushes within a few minutes—trimming the PR matrix (ubuntu-only on PR; full matrix on master push) is a small change. It would be easy to add later if needed; it isn't a blocker for re-enable.
  • The reason you disabled it. I genuinely don't know whether the disable was a 'let me look at the new YAML first' gate or a concrete concern. If there's a specific issue you noticed (action permissions, secrets handling, runner-image incompatibility, anything else), I'd be happy to investigate and propose a fix. If it was just the cautious default, no investigation is needed—the re-enable button is enough.

How to re-enable

Three steps, all on your side:

  1. Settings → Actions → General—confirm the actions policy allows the workflow's actions/checkout + actions/setup-dotnet (the workflow already pins them to commit SHAs per GitHub's hardening guidance). Workflow permissions should be Read and write so the workflow can upload TRX + coverage artifacts.
  2. Actions tab → build-test-coverage workflow → Enable workflow—single button click; flips disabled_manually to active.
  3. (Optional) trigger an immediate run via workflow_dispatch from the same Actions UI to verify the new YAML works on master before the next PR push fires it. The workflow doesn't currently include a workflow_dispatch trigger—I'd be happy to add one in PR test: comprehensive test-suite overhaul (100% coverage + E2E + compliance harness) #150 if a manual-trigger button would be useful for ad-hoc smoke checks.

Where to land follow-up CI changes

If any further workflow tweaks come out of this discussion (matrix trim, workflow_dispatch button, lighter test filter on PR runs, etc.), PR #150 (test/coverage-and-compliance) already touches .github/workflows/dotnet.yml and is the natural landing spot—I'd be happy to fold the changes in there. If you'd rather isolate the CI changes from the test-harness PR, I'd be equally happy to open a small separate PR. Just say which you prefer.


If the suggestion lands, the campaign queue gets per-PR CI signal automatically as each PR flips to ready, and master gets ongoing post-merge verification. If you'd rather keep the workflow disabled for now, that's no problem—the campaign PRs can merge on the strength of code review + clean merge state regardless. I just wanted to flag the option.

You must be logged in to vote

Replies: 0 comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Category
💡
Ideas
Labels
None yet
1 participant
Morty Proxy This is a proxified and sanitized view of the page, visit original site.