chore: change artefacts upload strategy for Coveralls #429
Conversation
|
Warning Rate limit exceeded@martin-georgiev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe coverage workflow (.github/workflows/coverage.yml) was restructured to trigger on push/pull_request to main, add top-level permissions, replace workflow_run gating and script-based waits with kachick/wait-other-jobs waiting on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GitHub as "GitHub Actions"
participant CoverageWF as "coverage.yml"
participant UnitWF as "unit-tests.yml"
participant IntWF as "integration-tests.yml"
participant Coveralls as "Coveralls Action"
Developer->>GitHub: push / pull_request to main
GitHub->>CoverageWF: start workflow
CoverageWF->>IntWF: wait (kachick/wait-other-jobs, workflowFile)
CoverageWF->>UnitWF: wait (kachick/wait-other-jobs, workflowFile)
alt Both workflows finish within 15m
CoverageWF->>Coveralls: finalize parallel build (carryforward: Integration,Unit)
else Timeout or failure
CoverageWF-->>Developer: job fails or times out
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/coverage.yml (1)
44-56: Action inputs unsupported in pinned commit
The pinned commit 648a8eb78e6d50909eff900e4ec85cab4524a45b of coverallsapp/github-action only defines the following inputs in action.yml:
github-tokenrepo-tokenpathparallelparallel-finishedflag-nameworking-directoryverboseIt does not support or document these inputs at that commit:
carryforwardgit-commitgit-branchservice-numberservice-job-idpull-requestTo prevent CI failures or ignored parameters, choose one of the following:
- Bump the action reference to a later commit or released version where those inputs are implemented and documented.
- Remove any unsupported inputs from your workflow.
Please update the workflow accordingly to ensure all provided inputs are recognized by the action.
🧹 Nitpick comments (2)
.github/workflows/coverage.yml (2)
53-53: Question: confirm correctness of service-job-id
service-job-idshould match whatever was used in the original upload steps, otherwise it’s superfluous or could confuse build correlation. Using the original workflow run’s ID here may not reflect a “job ID” and might be inconsistent with uploads.Options:
- Remove this line, letting the action infer as it does for uploads.
- Or set it to the same value the upload steps used (verify first).
Suggested change (if you cannot verify consistent usage):
- service-job-id: ${{ github.event.workflow_run.id }} + # service-job-id intentionally omitted to avoid mismatch with original uploads
55-55: Harden PR-number extraction to avoid null/empty array edge cases
github.event.workflow_run.pull_requests[0].numbercan throw ifpull_requestsis empty or missing (e.g., push builds). GitHub expressions do support||, but dereferencing[0].numberon an empty array can evaluate to null in some cases and error in others.Safer inline expression:
- pull-request: ${{ github.event.workflow_run.pull_requests[0].number || '' }} + pull-request: ${{ (github.event.workflow_run.pull_requests && github.event.workflow_run.pull_requests[0] && github.event.workflow_run.pull_requests[0].number) || '' }}If you prefer maximum safety and readability, compute this via a tiny
github-scriptstep and reference its output.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/coverage.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: sloth
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
🔇 Additional comments (2)
.github/workflows/coverage.yml (2)
50-51: LGTM: forwarding original commit/branch is correct and helpfulPassing
git-commitandgit-branchfromworkflow_runensures Coveralls attributes the finalization to the exact commit/branch that produced the uploads. This avoids mismatched “unknown commit” issues when the finalizer runs in a separate workflow.
48-48: ✅ Carryforward flags verified and correctI’ve checked the upload steps and confirmed the exact
flag-namevalues:
- .github/workflows/unit-tests.yml (line 117):
flag-name: "Unit"- .github/workflows/integration-tests.yml (line 134):
flag-name: "Integration"These match the
carryforward: "Integration,Unit"settings, so no changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/coverage.yml (1)
37-42: Missing Coveralls service-number (and commit) risks finalizing the wrong build across workflowsFinalization will default
service-numberto this Coverage workflow’s Run ID, which likely does not match the IDs used by the upload steps in the “Unit Tests” and “Integration Tests” workflows. That breaks correlation and can leave the intended build open. Pass the original triggering run’s ID (or your chosen shared identifier) and the commit explicitly.Apply this diff to explicitly set the identifiers:
uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b with: github-token: ${{ secrets.GITHUB_TOKEN }} parallel-finished: true + # Use the original workflow_run's Run ID to match uploads across workflows. + service-number: ${{ github.event.workflow_run.id || github.run_id }} + # Ensure Coveralls associates the finalization with the correct commit. + git-commit: ${{ github.event.workflow_run.head_sha || github.sha }} carryforward: "Unit,Integration"Follow-up verification (ensure the upload steps used the same
service-numberand flags):#!/bin/bash # Show where Coveralls action is used and which inputs are set in all workflows. rg -nC2 -g '.github/workflows/*.yml' -P 'uses:\s*coverallsapp/github-action@' rg -nC2 -g '.github/workflows/*.yml' -P '\b(service-number|service-job-id|flag-name|parallel)\s*:'
🧹 Nitpick comments (2)
.github/workflows/coverage.yml (2)
12-12: Dedupe parallel finalizers and ensure required token scopesWith “success or manual” gating, both workflow_run triggers can reach “ready” simultaneously and race to finalize. Add a short concurrency group keyed by the commit SHA to avoid double-finalize; also set minimal token permissions for the GitHub API call.
finalize: runs-on: ubuntu-latest if: ${{ github.event.workflow_run.conclusion == 'success' || github.event_name == 'workflow_dispatch' }} + permissions: + actions: read + contents: read + concurrency: + group: coverage-${{ github.event.workflow_run.head_sha || github.sha }} + cancel-in-progress: true
7-7: Add an optional SHA input for manual runs to target a specific commitFor
workflow_dispatch,context.shamay not correspond to the test runs you want to finalize (e.g., backfilling an older commit). Provide an input to override the target commit and thread it into the “head_sha” filter.on: workflow_run: workflows: ["Integration Tests", "Unit Tests"] types: [completed] workflow_dispatch: + inputs: + head_sha: + description: 'Commit SHA to finalize (optional; defaults to the current SHA or triggering run SHA)' + required: false + type: stringAnd update the list call to prefer the manual input:
- head_sha: context.payload.workflow_run?.head_sha || context.sha, + head_sha: context.payload?.inputs?.head_sha || context.payload.workflow_run?.head_sha || context.sha,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/coverage.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Code Quality
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PostGIS 3.5 + PHP 8.1
- GitHub Check: PostgreSQL 16 + PostGIS 3.4 + PHP 8.2
- GitHub Check: PostgreSQL 17 + PostGIS 3.4 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PostGIS 3.5 + PHP 8.2
- GitHub Check: sloth
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 1.2
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
🔇 Additional comments (2)
.github/workflows/coverage.yml (2)
36-36: Nice: explicit step name improves discoverability in logsNaming the finalization step makes it easier to locate in Actions logs and triage failures.
41-41: No mismatched flag names found — carryforward entries align with upload stepsBoth uploader steps define
flag-nameexactly as"Unit"and"Integration", matching thecarryforward: "Unit,Integration"in.github/workflows/coverage.yml. No further changes are needed.• In
.github/workflows/unit-tests.yml, the Coveralls step usesflag-name: "Unit"
• In.github/workflows/integration-tests.yml, the Coveralls step usesflag-name: "Integration"
• In.github/workflows/coverage.yml, the finalization usescarryforward: "Unit,Integration"All names and casing align correctly.
martin-georgiev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/coverage.yml (1)
75-82: Pass original commit and branch to Coveralls to ensure correct attribution.Without explicit git-commit/git-branch, Coveralls will use this finalizer’s SHA/branch. Use the values determined in the verification step.
Apply:
- if: ${{ steps.check.outputs.ready == 'true' }} name: Upload merged coverage to Coveralls uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} file: ./merged-coverage/Clover.xml fail-on-error: false + git-commit: ${{ steps.check.outputs.head_sha }} + git-branch: ${{ steps.check.outputs.head_branch }}
🧹 Nitpick comments (6)
.github/workflows/unit-tests.yml (1)
110-117: Pin actions/upload-artifact for supply-chain safetyConfirmed that your PHPUnit “unit” script emits a Clover report to the expected path (
var/logs/test-coverage/unit/clover.xml), so the workflow’s artifact upload will succeed:• In composer.json,
"phpunit:unit"invokesXDEBUG_MODE=coverage phpunit --configuration=./ci/phpunit/config-unit.xml
• In ci/phpunit/config-unit.xml, the<clover>logger writes to../../var/logs/test-coverage/unit/clover.xml【run_scripts】Optional refactor (recommended):
• In.github/workflows/unit-tests.yml(lines 110–117), replace the floating tag with a full commit SHA. For example:- uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@<commit-sha>– this pins the action version and guards against unintended updates.
.github/workflows/coverage.yml (5)
47-55: Pin download-artifact and avoid implicit defaults.
- actions/download-artifact is referenced by tag v4; pin to a commit SHA like the rest of the repo.
- Minor: set if-no-files-found to error to fail faster if the integration artifact is missing (mirrors your upload behavior).
Possible tweak:
- uses: actions/download-artifact@v4 + uses: actions/download-artifact@v4 # pin to a full-length commit SHA with: name: coverage-integration-clover github-token: ${{ secrets.GITHUB_TOKEN }} run-id: ${{ steps.check.outputs.integration_run_id }} path: integration-coverage + if-no-files-found: error
56-64: Same as above for the unit artifact download.Mirror the pinning and missing-artifact behavior for consistency.
- uses: actions/download-artifact@v4 + uses: actions/download-artifact@v4 # pin to a full-length commit SHA with: name: coverage-unit-clover github-token: ${{ secrets.GITHUB_TOKEN }} run-id: ${{ steps.check.outputs.unit_run_id }} path: unit-coverage + if-no-files-found: error
66-74: Nit: make the reports list unambiguous for ReportGenerator.ReportGenerator accepts semicolon-separated paths; mixing newlines and semicolons works but is easy to mis-edit. Consider a single line or remove the trailing semicolon from the first line.
- reports: | - unit-coverage/clover.xml; - integration-coverage/clover.xml + reports: unit-coverage/clover.xml;integration-coverage/clover.xml
7-8: Optional: accept manual inputs to finalize an arbitrary run.When triggered via workflow_dispatch, headSha falls back to this workflow’s commit SHA, which may not correlate with any recent test runs. Consider adding inputs (e.g., head_sha or PR number) and using them in the verification step.
Example:
on: workflow_run: workflows: ["Integration Tests", "Unit Tests"] types: [completed] workflow_dispatch: + inputs: + head_sha: + description: "Commit SHA to finalize coverage for" + required: false + head_branch: + description: "Branch name (for Coveralls attribution)" + required: falseThen in the script:
-const headSha = context.payload.workflow_run?.head_sha || context.sha; +const headSha = core.getInput('head_sha') || context.payload.workflow_run?.head_sha || context.sha; +const headBranch = core.getInput('head_branch') || context.payload.workflow_run?.head_branch || '';
34-34: Trim trailing whitespace (YAMLlint).There’s trailing space on this line flagged by YAMLlint. Safe to remove.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/coverage.yml(1 hunks).github/workflows/integration-tests.yml(1 hunks).github/workflows/unit-tests.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/coverage.yml
[error] 34-34: trailing spaces
(trailing-spaces)
.github/workflows/coverage.yml
Outdated
| name: Verify all prerequisite workflows succeeded | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const workflows = await github.rest.actions.listWorkflowRunsForRepo({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| head_sha: context.payload.workflow_run.head_sha, | ||
| const {owner, repo} = context.repo; | ||
| const headSha = context.payload.workflow_run?.head_sha || context.sha; | ||
| const runs = await github.rest.actions.listWorkflowRunsForRepo({ | ||
| owner, | ||
| repo, | ||
| head_sha: headSha, | ||
| status: 'completed', | ||
| per_page: 100 | ||
| }); | ||
| const bothComplete = ['Integration Tests', 'Unit Tests'].every(name => | ||
| workflows.data.workflow_runs.some( | ||
| run => run.name === name && run.conclusion === 'success' | ||
| ) | ||
| ); | ||
| const findRun = (name) => runs.data.workflow_runs.find(r => r.name === name && r.conclusion === 'success'); | ||
| const integration = findRun('Integration Tests'); | ||
| const unit = findRun('Unit Tests'); | ||
| const ok = Boolean(unit && integration); | ||
| core.info(ok ? 'All workflows completed successfully.' : 'Waiting for a remaining workflow to complete.'); | ||
| core.setOutput('ready', String(ok)); | ||
| if (ok) { | ||
| core.setOutput('unit_run_id', String(unit.id)); | ||
| core.setOutput('integration_run_id', String(integration.id)); | ||
| } | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose original head_sha and head_branch as outputs; use them in the Coveralls upload.
The goal is to “pass original context to Coveralls.” You already resolve headSha from the triggering run, but it isn’t exported for later steps; the Coveralls action will otherwise default to this workflow’s GITHUB_SHA/branch, which can mis-attribute coverage.
Apply:
- id: check
name: Verify all prerequisite workflows succeeded
uses: actions/github-script@v7
with:
script: |
const {owner, repo} = context.repo;
const headSha = context.payload.workflow_run?.head_sha || context.sha;
+ const headBranch = context.payload.workflow_run?.head_branch || '';
const runs = await github.rest.actions.listWorkflowRunsForRepo({
owner,
repo,
head_sha: headSha,
status: 'completed',
per_page: 100
});
const findRun = (name) => runs.data.workflow_runs.find(r => r.name === name && r.conclusion === 'success');
const integration = findRun('Integration Tests');
const unit = findRun('Unit Tests');
const ok = Boolean(unit && integration);
core.info(ok ? 'All workflows completed successfully.' : 'Waiting for a remaining workflow to complete.');
core.setOutput('ready', String(ok));
+ core.setOutput('head_sha', headSha);
+ core.setOutput('head_branch', headBranch);
if (ok) {
core.setOutput('unit_run_id', String(unit.id));
core.setOutput('integration_run_id', String(integration.id));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Verify all prerequisite workflows succeeded | |
| uses: actions/github-script@v7 | |
| with: | |
| script: | | |
| const workflows = await github.rest.actions.listWorkflowRunsForRepo({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| head_sha: context.payload.workflow_run.head_sha, | |
| const {owner, repo} = context.repo; | |
| const headSha = context.payload.workflow_run?.head_sha || context.sha; | |
| const runs = await github.rest.actions.listWorkflowRunsForRepo({ | |
| owner, | |
| repo, | |
| head_sha: headSha, | |
| status: 'completed', | |
| per_page: 100 | |
| }); | |
| const bothComplete = ['Integration Tests', 'Unit Tests'].every(name => | |
| workflows.data.workflow_runs.some( | |
| run => run.name === name && run.conclusion === 'success' | |
| ) | |
| ); | |
| const findRun = (name) => runs.data.workflow_runs.find(r => r.name === name && r.conclusion === 'success'); | |
| const integration = findRun('Integration Tests'); | |
| const unit = findRun('Unit Tests'); | |
| const ok = Boolean(unit && integration); | |
| core.info(ok ? 'All workflows completed successfully.' : 'Waiting for a remaining workflow to complete.'); | |
| core.setOutput('ready', String(ok)); | |
| if (ok) { | |
| core.setOutput('unit_run_id', String(unit.id)); | |
| core.setOutput('integration_run_id', String(integration.id)); | |
| } | |
| - id: check | |
| name: Verify all prerequisite workflows succeeded | |
| uses: actions/github-script@v7 | |
| with: | |
| script: | | |
| const {owner, repo} = context.repo; | |
| const headSha = context.payload.workflow_run?.head_sha || context.sha; | |
| const headBranch = context.payload.workflow_run?.head_branch || ''; | |
| const runs = await github.rest.actions.listWorkflowRunsForRepo({ | |
| owner, | |
| repo, | |
| head_sha: headSha, | |
| status: 'completed', | |
| per_page: 100 | |
| }); | |
| const findRun = (name) => runs.data.workflow_runs.find(r => r.name === name && r.conclusion === 'success'); | |
| const integration = findRun('Integration Tests'); | |
| const unit = findRun('Unit Tests'); | |
| const ok = Boolean(unit && integration); | |
| core.info(ok ? 'All workflows completed successfully.' : 'Waiting for a remaining workflow to complete.'); | |
| core.setOutput('ready', String(ok)); | |
| core.setOutput('head_sha', headSha); | |
| core.setOutput('head_branch', headBranch); | |
| if (ok) { | |
| core.setOutput('unit_run_id', String(unit.id)); | |
| core.setOutput('integration_run_id', String(integration.id)); | |
| } |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 34-34: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
.github/workflows/coverage.yml around lines 18-46: the script computes headSha
from the triggering run but never exposes it (nor the originating head_branch),
so downstream steps (Coveralls upload) may use this workflow's SHA/branch and
mis-attribute coverage; add a headBranch variable by reading
context.payload.workflow_run?.head_branch || context.ref.split('/').pop(), and
call core.setOutput for both head_sha and head_branch (always, not only when ok)
so later steps can consume the original head commit and branch for the Coveralls
upload.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/coverage.yml (1)
34-40: Risk: Coveralls “build” correlation across separate workflows is undefined; unify the build idUploads from Unit/Integration occur in different workflow runs than this finalizer. By default, the Coveralls action uses the current run’s build number; without an explicit, shared identifier, the “Finalize” call may close an empty build and never merge the uploaded reports. Align all uploads and the finalizer on the same build id (e.g., PR head SHA). You can do this by forcing the service job/build identifier (via env supported by Coveralls integrations) and, optionally, pinning commit/branch for attribution. (docs.coveralls.io)
Minimal change here (you’ll also need to set the exact same env in the Unit/Integration upload steps):
- if: ${{ always() }} name: Finalize Coveralls parallel build uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 + env: + # Use the originating commit as the shared “build” id across all workflows + COVERALLS_SERVICE_JOB_ID: ${{ github.event.pull_request.head.sha || github.sha }} with: github-token: ${{ secrets.GITHUB_TOKEN }} parallel-finished: true carryforward: "Integration,Unit" + # Optional: ensure attribution to the originating commit/branch + git-commit: ${{ github.event.pull_request.head.sha || github.sha }} + git-branch: ${{ github.head_ref || github.ref_name }}Follow-up:
- Ensure the Unit/Integration workflows upload with
parallel: true, use flagsIntegrationandUnit, and set the sameCOVERALLS_SERVICE_JOB_ID. Otherwise carryforward and finalization won’t target the intended build. (docs.coveralls.io)
🧹 Nitpick comments (1)
.github/workflows/coverage.yml (1)
22-31: Wait step usage looks good; consider scoping by event and adding grace periodCurrent config is valid (step-level timeout is supported and recommended for this action). To reduce false negatives when event types differ or when jobs register a bit later, optionally add
eventNameand a shortstartupGracePeriodper the action’s docs. (github.com)Example:
- name: Wait for Unit and Integration jobs uses: kachick/wait-other-jobs@755be1ff093bd2a6a82a24bfc50ef4d9fe491a59 # v3.8.1 with: skip-same-workflow: 'true' wait-list: | [ - {"workflowFile": "integration-tests.yml"}, - {"workflowFile": "unit-tests.yml"} + {"workflowFile": "integration-tests.yml", "eventName": "${{ github.event_name }}", "startupGracePeriod": "PT5M"}, + {"workflowFile": "unit-tests.yml", "eventName": "${{ github.event_name }}", "startupGracePeriod": "PT5M"} ] timeout-minutes: 15
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/coverage.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/coverage.yml
33-33: property "check" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: sloth
- GitHub Check: coverage
🔇 Additional comments (2)
.github/workflows/coverage.yml (2)
13-16: Top-level permissions are correct for cross-workflow waiting
contents: read,checks: read, andactions: readare exactly what wait-other-jobs needs to enumerate runs and checks. Nice. (github.com)
4-12: Coveralls finalization is safe: dependent workflows share the same triggersBoth the unit-tests and integration-tests workflows are configured to run on
pushtomainand onpull_requesttargetingmain, matching the triggers in.github/workflows/coverage.yml. No jobs in the wait-list need to be marked optional.•
.github/workflows/unit-tests.yml
– on.push.branches:main
– on.pull_request.branches:main•
.github/workflows/integration-tests.yml
– on.push.branches:main
– on.pull_request.branches:main•
.github/workflows/coverage.yml
– on.push.branches:main(paths-ignore: release-please)
– on.pull_request.branches:mainAll coverage-upload steps (
parallel: truein unit/integration andparallel-finished: truein coverage) will reliably run under the same event context.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/coverage.yml (1)
35-41: Add consistentservice-numberto all Coveralls steps and make finalization unconditionalOur inspection shows:
- In unit-tests.yml (lines ≈112–118) and integration-tests.yml (lines ≈129–135), the Coveralls upload steps include
parallel: trueand the correctflag-name(“Unit” / “Integration”) but do not set a sharedservice-number.- In coverage.yml (lines 35–41), the finalization step likewise omits
service-number(so it will close only this workflow’s build), and lacks an unconditional execution guard (if: always()).These omissions mean the “Finalize Coveralls parallel build” step won’t reliably aggregate the uploads from the Unit and Integration workflows.
Please apply the following critical fixes:
• .github/workflows/unit-tests.yml (around line 112)
- name: Upload coverage results to Coveralls if: matrix.calculate-code-coverage == true uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} file: ./var/logs/test-coverage/unit/clover.xml parallel: true - flag-name: "Unit" + flag-name: "Unit" + # Ensure all uploads share the same build identity + service-number: ${{ github.event.pull_request.head.sha || github.sha }} fail-on-error: false• .github/workflows/integration-tests.yml (around line 133)
- name: Upload coverage results to Coveralls if: matrix.calculate-code-coverage == true uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} file: ./var/logs/test-coverage/integration/clover.xml parallel: true - flag-name: "Integration" + flag-name: "Integration" + # Ensure all uploads share the same build identity + service-number: ${{ github.event.pull_request.head.sha || github.sha }} fail-on-error: false• .github/workflows/coverage.yml (lines 35–41)
- Add a build-meta step just before finalization:
- name: Determine Coveralls build identity id: build_meta shell: bash run: | if [[ "${{ github.event_name }}" == "pull_request" ]]; then echo "head_sha=${{ github.event.pull_request.head.sha }}" >> "$GITHUB_OUTPUT" else echo "head_sha=${{ github.sha }}" >> "$GITHUB_OUTPUT" fi
- Update the finalize step to be unconditional and reference that identity:
- - name: Finalize Coveralls parallel build - uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 + - name: Finalize Coveralls parallel build + if: ${{ always() }} + uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b # v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} - parallel-finished: true - carryforward: "Integration,Unit" + parallel-finished: true + # Must match the service-number used by all upload steps + service-number: ${{ steps.build_meta.outputs.head_sha }} + carryforward: "Integration,Unit"These changes will ensure all uploads and the finalization step operate on the same Coveralls build, even across separate workflow runs.
🧹 Nitpick comments (3)
.github/workflows/coverage.yml (3)
4-12: Guard against duplicate “finalize” runs across push and pull_requestWith both push→main and pull_request→main triggers, the same commit can fire this workflow twice (when a PR branch gets updated). Coveralls can tolerate repeated finalization, but this increases noise and can race with uploads. Consider limiting to a single event (e.g., only pull_request) or add a lightweight job-level condition/concurrency.
Example options:
on: - push: - branches: - - main - paths-ignore: - - '.github/actions/release-please/**' pull_request: branches: - mainOr keep both events but gate the job:
jobs: - wait-for-tests-worflows-before-upload: + wait-for-tests-worflows-before-upload: + if: ${{ github.event_name == 'pull_request' }}Or use concurrency to collapse duplicates:
concurrency: group: coverage-${{ github.event_name }}-${{ github.ref_name }} cancel-in-progress: true
19-19: Typo in job id (“worflows”)Minor but visible in logs and when referencing the job id.
Apply:
- wait-for-tests-worflows-before-upload: + wait-for-tests-workflows-before-upload:
25-33: All workflow filenames are valid—consider increasing the wait step timeoutI’ve confirmed that both
.github/workflows/integration-tests.ymlandunit-tests.ymlexist, so thewait-listentries will be respected. However, a 15-minute timeout on GitHub-hosted runners can sometimes be tight if there’s queueing or particularly long integration tests. You may want to bump this to 30–40 minutes to avoid unexpected timeouts.• File:
.github/workflows/coverage.yml
Lines: 25–33wait-list: | [ {"workflowFile": "integration-tests.yml"}, {"workflowFile": "unit-tests.yml"} ] - timeout-minutes: 15 + # Optional: extend if your tests often approach the limit + timeout-minutes: 30
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/coverage.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Code Quality
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: sloth
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: wait-for-tests-worflows-before-upload
🔇 Additional comments (2)
.github/workflows/coverage.yml (2)
13-16: Top-level permissions look right for Actions API + artifact readscontents: read, checks: read, and actions: read are appropriate for waiting on other runs and for Coveralls finalization.
22-24: Good: action pin by commit SHA with version commentPinning kachick/wait-other-jobs to a specific commit (v3.8.1) is a solid supply-chain practice.
martin-georgiev
changed the title
Summary by CodeRabbit