CNTRLPLANE-244: ci: add Codecov integration to unit test workflow#8060
CNTRLPLANE-244: ci: add Codecov integration to unit test workflow#8060openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe GitHub Actions test workflow has been enhanced with additional triggers ( Sequence Diagram(s)mermaid mermaid ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/test.yaml (1)
39-42: Please verify thatbcexists onarc-runner-set.Lines 39-42 add a new runtime dependency that is not provisioned here. If the runner image does not ship
bc, every PR will fail after tests.Use `awk` instead of another CLI dependency
- DIFF=$(echo "$PR_COV - $BASE_COV" | bc) + DIFF=$(awk -v pr="$PR_COV" -v base="$BASE_COV" 'BEGIN { printf "%.2f", pr - base }') echo "### Coverage diff: ${DIFF}% (main: ${BASE_COV}% → PR: ${PR_COV}%)" >> "$GITHUB_STEP_SUMMARY" # Fail if coverage drops by more than 1% - DROP=$(echo "$DIFF < -1.0" | bc) + DROP=$(awk -v diff="$DIFF" 'BEGIN { print (diff < -1.0) ? 1 : 0 }')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yaml around lines 39 - 42, Replace the non-portable use of bc by computing the coverage diff and the boolean drop check with awk (or pure shell) so the workflow doesn't rely on bc being present; specifically change the DIFF calculation (symbol DIFF) and the DROP determination (symbol DROP) to use awk's arithmetic (e.g. DIFF=$(awk "BEGIN{print ${PR_COV} - ${BASE_COV}}") and DROP=$(awk "BEGIN{print (${DIFF} < -1.0) ? 1 : 0}") or equivalent POSIX-safe logic) and keep the existing echo that writes the coverage summary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 26-36: The workflow exposes cluster RBAC to untrusted PRs via the
"Update baseline coverage" and "Compare coverage" steps that call kubectl
against the coverage-baseline ConfigMap; remove or gate all kubectl usage from
the PR test job (the steps named "Update baseline coverage" and "Compare
coverage" that reference the coverage-baseline configmap) so PRs cannot access
the cluster, move the baseline write (the patch implemented in Update baseline
coverage) into a trusted post-merge workflow that runs only on main, and replace
the PR comparison (the BASE_COV read in Compare coverage) with a
non-cluster-backed store such as a repo-stored file, an artifacts API lookup, or
the Actions cache to fetch the baseline for pull_request jobs. Ensure the job no
longer runs any kubectl commands for pull_request events.
- Around line 18-23: The workflow extracts coverage into cover.out but never
uploads it to Codecov; add a new job step after the existing "Extract coverage
percentage" step (reference step id coverage) that runs the official Codecov
uploader (uses: codecov/codecov-action@v4) and points to the generated cover.out
(files: cover.out) and uses the CODECOV_TOKEN secret if required (token: ${{
secrets.CODECOV_TOKEN }}), so the coverage file is published to the Codecov
dashboard.
- Around line 36-48: The current BASE_COV assignment hides real kubectl errors
by using `2>/dev/null || echo ""`; change the kubectl invocation that sets
BASE_COV to use `--ignore-not-found` (e.g., `kubectl get configmap
coverage-baseline -n arc-runners --ignore-not-found -o jsonpath=...`) so only a
missing ConfigMap yields an empty result, and remove the stderr suppression;
additionally, after running that kubectl command check its exit status and if
non-zero (and not just empty output) surface the error (fail the job or print
the kubectl stderr) instead of treating it as "no baseline"—update the logic
that uses BASE_COV and the `if [ -n "$BASE_COV" ]` branch accordingly.
---
Nitpick comments:
In @.github/workflows/test.yaml:
- Around line 39-42: Replace the non-portable use of bc by computing the
coverage diff and the boolean drop check with awk (or pure shell) so the
workflow doesn't rely on bc being present; specifically change the DIFF
calculation (symbol DIFF) and the DROP determination (symbol DROP) to use awk's
arithmetic (e.g. DIFF=$(awk "BEGIN{print ${PR_COV} - ${BASE_COV}}") and
DROP=$(awk "BEGIN{print (${DIFF} < -1.0) ? 1 : 0}") or equivalent POSIX-safe
logic) and keep the existing echo that writes the coverage summary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ea535024-6c00-4f93-87a4-172887443375
📒 Files selected for processing (1)
.github/workflows/test.yaml
.github/workflows/test.yaml
Outdated
| - name: Update baseline coverage | ||
| if: github.ref == 'refs/heads/main' | ||
| run: | | ||
| kubectl patch configmap coverage-baseline -n arc-runners \ | ||
| -p "{\"data\":{\"total\":\"${{ steps.coverage.outputs.total }}\"}}" | ||
|
|
||
| # On PRs, compare against the baseline | ||
| - name: Compare coverage | ||
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| BASE_COV=$(kubectl get configmap coverage-baseline -n arc-runners -o jsonpath='{.data.total}' 2>/dev/null || echo "") |
There was a problem hiding this comment.
Avoid giving this PR test job cluster access.
These kubectl steps make kube RBAC a requirement for the same self-hosted job that executes make test. That exposes whatever get/patch rights the runner has to untrusted PR code. Keep baseline writes in a trusted post-merge path, and use a non-cluster store for PR comparisons if you still need this gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yaml around lines 26 - 36, The workflow exposes
cluster RBAC to untrusted PRs via the "Update baseline coverage" and "Compare
coverage" steps that call kubectl against the coverage-baseline ConfigMap;
remove or gate all kubectl usage from the PR test job (the steps named "Update
baseline coverage" and "Compare coverage" that reference the coverage-baseline
configmap) so PRs cannot access the cluster, move the baseline write (the patch
implemented in Update baseline coverage) into a trusted post-merge workflow that
runs only on main, and replace the PR comparison (the BASE_COV read in Compare
coverage) with a non-cluster-backed store such as a repo-stored file, an
artifacts API lookup, or the Actions cache to fetch the baseline for
pull_request jobs. Ensure the job no longer runs any kubectl commands for
pull_request events.
.github/workflows/test.yaml
Outdated
| BASE_COV=$(kubectl get configmap coverage-baseline -n arc-runners -o jsonpath='{.data.total}' 2>/dev/null || echo "") | ||
| PR_COV="${{ steps.coverage.outputs.total }}" | ||
| if [ -n "$BASE_COV" ]; then | ||
| DIFF=$(echo "$PR_COV - $BASE_COV" | bc) | ||
| echo "### Coverage diff: ${DIFF}% (main: ${BASE_COV}% → PR: ${PR_COV}%)" >> "$GITHUB_STEP_SUMMARY" | ||
| # Fail if coverage drops by more than 1% | ||
| DROP=$(echo "$DIFF < -1.0" | bc) | ||
| if [ "$DROP" -eq 1 ]; then | ||
| echo "::error::Coverage dropped by ${DIFF}% (threshold: -1%)" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "### No baseline coverage found. Skipping comparison." >> "$GITHUB_STEP_SUMMARY" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check kubectl documentation for --ignore-not-found flag behavior
# This flag should distinguish between "not found" vs actual errors
kubectl get configmap --help 2>/dev/null | grep -A 5 "ignore-not-found" || echo "Checking kubectl documentation online approach"Repository: openshift/hypershift
Length of output: 110
🌐 Web query:
kubectl get --ignore-not-found flag behavior exit code error handling
💡 Result:
The --ignore-not-found flag (default: false) for kubectl get suppresses the NotFound error message and returns exit code 0 when a specific named object does not exist. Without the flag (or with --ignore-not-found=false), it prints "Error from server (NotFound): [resource] "[name]" not found" and returns exit code 1. For collection queries (no specific name, e.g., kubectl get pods), the flag has no effect: it always prints "No resources found" (to stderr) and returns exit code 0, as an empty list is not considered an error. Example behaviors: - kubectl get pod foo (nonexistent): error message, exit 1 - kubectl get pod foo --ignore-not-found: no error message, exit 0 - kubectl get pods (none exist): "No resources found", exit 0 (flag irrelevant) This distinction is explicit in official docs: "If set to true, suppresses NotFound error for specific objects that do not exist. Using this flag with commands that query for collections of resources has no effect when no resources are found." The help text ("If the requested object does not exist the command will return exit code 0") implies specific objects, but could be clearer per ongoing SIG-CLI discussion. Source code confirms: ignores NotFound only for single-object gets, prints "No resources found" and exits 0 if no output and not ignoring NotFound (but collections always 0). For scripting/error handling, use the flag for idempotent specific-object checks (exit 0 if missing). Collections always succeed with 0 even if empty.
Citations:
- 1:
kubectl get --ignore-not-foundbehavior unclear kubernetes/kubectl#1596 - 2: kubectl get should return non-zero value if not found kubernetes/kubectl#821
- 3: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_get
- 4: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/get/get.go
Don't treat every kubectl failure as "no baseline".
2>/dev/null || echo "" suppresses RBAC, API, and connectivity errors, making them indistinguishable from a missing ConfigMap and silently disabling the coverage check. Use --ignore-not-found instead to handle only the missing-ConfigMap case as optional while surfacing real errors.
Suggested fix
- BASE_COV=$(kubectl get configmap coverage-baseline -n arc-runners -o jsonpath='{.data.total}' 2>/dev/null || echo "")
+ BASE_COV=$(kubectl get configmap coverage-baseline \
+ -n arc-runners \
+ --ignore-not-found \
+ -o jsonpath='{.data.total}')📝 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.
| BASE_COV=$(kubectl get configmap coverage-baseline -n arc-runners -o jsonpath='{.data.total}' 2>/dev/null || echo "") | |
| PR_COV="${{ steps.coverage.outputs.total }}" | |
| if [ -n "$BASE_COV" ]; then | |
| DIFF=$(echo "$PR_COV - $BASE_COV" | bc) | |
| echo "### Coverage diff: ${DIFF}% (main: ${BASE_COV}% → PR: ${PR_COV}%)" >> "$GITHUB_STEP_SUMMARY" | |
| # Fail if coverage drops by more than 1% | |
| DROP=$(echo "$DIFF < -1.0" | bc) | |
| if [ "$DROP" -eq 1 ]; then | |
| echo "::error::Coverage dropped by ${DIFF}% (threshold: -1%)" | |
| exit 1 | |
| fi | |
| else | |
| echo "### No baseline coverage found. Skipping comparison." >> "$GITHUB_STEP_SUMMARY" | |
| BASE_COV=$(kubectl get configmap coverage-baseline \ | |
| -n arc-runners \ | |
| --ignore-not-found \ | |
| -o jsonpath='{.data.total}') | |
| PR_COV="${{ steps.coverage.outputs.total }}" | |
| if [ -n "$BASE_COV" ]; then | |
| DIFF=$(echo "$PR_COV - $BASE_COV" | bc) | |
| echo "### Coverage diff: ${DIFF}% (main: ${BASE_COV}% → PR: ${PR_COV}%)" >> "$GITHUB_STEP_SUMMARY" | |
| # Fail if coverage drops by more than 1% | |
| DROP=$(echo "$DIFF < -1.0" | bc) | |
| if [ "$DROP" -eq 1 ]; then | |
| echo "::error::Coverage dropped by ${DIFF}% (threshold: -1%)" | |
| exit 1 | |
| fi | |
| else | |
| echo "### No baseline coverage found. Skipping comparison." >> "$GITHUB_STEP_SUMMARY" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yaml around lines 36 - 48, The current BASE_COV
assignment hides real kubectl errors by using `2>/dev/null || echo ""`; change
the kubectl invocation that sets BASE_COV to use `--ignore-not-found` (e.g.,
`kubectl get configmap coverage-baseline -n arc-runners --ignore-not-found -o
jsonpath=...`) so only a missing ConfigMap yields an empty result, and remove
the stderr suppression; additionally, after running that kubectl command check
its exit status and if non-zero (and not just empty output) surface the error
(fail the job or print the kubectl stderr) instead of treating it as "no
baseline"—update the logic that uses BASE_COV and the `if [ -n "$BASE_COV" ]`
branch accordingly.
bryan-cox
changed the title
bryan-cox
force-pushed
the
add-codecov
branch
13 times, most recently
from
24676e8 to
2677314
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
bryan-cox
force-pushed
the
add-codecov
branch
3 times, most recently
from
8137c83 to
10b2630
Compare
bryan-cox
changed the title
bryan-cox
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-244 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/area ci-tooling |
openshift-ci
bot
added
area/ci-tooling
Upload coverage data to Codecov after each unit test run and add codecov.yml to set the default branch to main. - Uses codecov/codecov-action@v5 to upload cover.out - Token passed via env so fork PRs fall back to tokenless upload - codecov.yml sets default branch to main (Codecov defaults to master) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/verified by unit tests |
|
/override "ci/prow/e2e-aks" |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/e2e-aks DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override e2e-aks |
|
@bryan-cox: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override "e2e-aks" |
|
@bryan-cox: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override "ci/prow/e2e-aks" |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/e2e-aks, ci/prow/e2e-aws, ci/prow/e2e-aws-upgrade-hypershift-operator, ci/prow/e2e-kubevirt-aws-ovn-reduced, ci/prow/e2e-v2-aws DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override "ci/prow/e2e-azure-self-managed" |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/e2e-azure-self-managed DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
Add Codecov integration to the Unit Tests GitHub Actions workflow to upload
coverage data after each test run.
codecov/codecov-action@v5to uploadcover.outafter each unit test runenv:(notwith:) so fork PRs gracefully fall back to tokenless uploadCODECOV_TOKENsecret must be configured in the repository settingscodecov.ymlsetsbranch: mainas the default branch (Codecov defaults tomasterotherwise)Follow-up
Test plan
make testgeneratescover.out🤖 Generated with Claude Code