OAPE-698: Integration of codecov on the must-gather-operator#350
OAPE-698: Integration of codecov on the must-gather-operator#350praveencodes wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@praveencodes: This pull request references OAPE-698 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 sub-task to target the "5.0.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. |
WalkthroughThis PR adds end-to-end coverage instrumentation for the must-gather-operator. It introduces a multi-stage Dockerfile that compiles the binary with coverage flags, defines Make targets to orchestrate Docker image builds and coverage collection, and implements a Bash script that patches deployments with the coverage image, flushes and collects coverage data, and optionally uploads results to Codecov. ChangesE2E Coverage Instrumentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: praveencodes The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile.coverage`:
- Around line 23-24: The coverage dir is owned by UID 65532 but the container
switches to USER 65534 so GOCOVERDIR writes will be blocked; update the RUN line
that creates /tmp/e2e-cover to either chown it to 65534:65534 (so the runtime
user owns it) or keep chown 65532 but change the subsequent USER to 65532;
specifically modify the mkdir/chown/chmod/USER sequence in Dockerfile.coverage
so the ownership (chown) and the USER directive refer to the same UID (or make
the directory group-writable and set the USER’s group accordingly).
In `@hack/e2e-coverage.sh`:
- Around line 109-113: The current code that downloads and verifies the Codecov
binary using variables codecov_bin and codecov_version can cause the script to
exit under set -euo pipefail; make the download/verification non-fatal by
handling errors instead of letting them propagate: perform the curl and
sha256sum steps inside a conditional or use error-catching (e.g., test the
commands' exit status) and on failure emit a warning (mentioning
codecov_bin/codecov_version) and skip chmod/verification so the script continues
to the optional upload step; ensure subsequent code still checks for the
presence/executability of codecov_bin before attempting upload.
In `@Makefile`:
- Line 42: The Makefile rule invoking the container build uses
images/ci/Dockerfile.coverage which no longer exists; update the docker build
command (the target that runs "$(CONTAINER_TOOL) build -f
images/ci/Dockerfile.coverage -t $(COVERAGE_IMG) .") to point to the new
Dockerfile.coverage location introduced in this PR (or to a Makefile variable
that references the correct path) so the docker-build-coverage target uses the
correct Dockerfile path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c054540-8cc3-414f-9ec7-0d444071c1e9
📒 Files selected for processing (3)
Dockerfile.coverageMakefilehack/e2e-coverage.sh
| RUN mkdir -p /tmp/e2e-cover && chown 65532:65532 /tmp/e2e-cover && chmod 700 /tmp/e2e-cover | ||
| USER 65534:65534 |
There was a problem hiding this comment.
Fix coverage directory ownership mismatch (coverage writes will fail).
Line 23 grants /tmp/e2e-cover only to UID 65532, but Line 24 runs the process as UID 65534. With mode 700, the operator cannot write GOCOVERDIR output.
Suggested fix
-RUN mkdir -p /tmp/e2e-cover && chown 65532:65532 /tmp/e2e-cover && chmod 700 /tmp/e2e-cover
-USER 65534:65534
+RUN mkdir -p /tmp/e2e-cover && chown 65534:65534 /tmp/e2e-cover && chmod 700 /tmp/e2e-cover
+USER 65534:65534🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile.coverage` around lines 23 - 24, The coverage dir is owned by UID
65532 but the container switches to USER 65534 so GOCOVERDIR writes will be
blocked; update the RUN line that creates /tmp/e2e-cover to either chown it to
65534:65534 (so the runtime user owns it) or keep chown 65532 but change the
subsequent USER to 65532; specifically modify the mkdir/chown/chmod/USER
sequence in Dockerfile.coverage so the ownership (chown) and the USER directive
refer to the same UID (or make the directory group-writable and set the USER’s
group accordingly).
| curl -sS -o "${codecov_bin}" "https://uploader.codecov.io/${codecov_version}/linux/codecov" | ||
| curl -sS -o "${codecov_bin}.SHA256SUM" "https://uploader.codecov.io/${codecov_version}/linux/codecov.SHA256SUM" | ||
|
|
||
| cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM" && cd - >/dev/null | ||
| chmod +x "${codecov_bin}" |
There was a problem hiding this comment.
Make Codecov download/verification non-fatal to match optional upload behavior.
With set -euo pipefail, a failure on Lines 109–113 exits the script, but upload is treated as optional later (Line 138). This can fail jobs after coverage was already collected.
Suggested fix
- curl -sS -o "${codecov_bin}" "https://uploader.codecov.io/${codecov_version}/linux/codecov"
- curl -sS -o "${codecov_bin}.SHA256SUM" "https://uploader.codecov.io/${codecov_version}/linux/codecov.SHA256SUM"
-
- cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM" && cd - >/dev/null
- chmod +x "${codecov_bin}"
+ if ! curl -sS -o "${codecov_bin}" "https://uploader.codecov.io/${codecov_version}/linux/codecov" \
+ || ! curl -sS -o "${codecov_bin}.SHA256SUM" "https://uploader.codecov.io/${codecov_version}/linux/codecov.SHA256SUM" \
+ || ! (cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM" >/dev/null); then
+ echo "Warning: failed to download/verify Codecov uploader (non-fatal)"
+ else
+ chmod +x "${codecov_bin}"
+ "${codecov_bin}" "${codecov_args[@]}" || echo "Warning: Codecov upload failed (non-fatal)"
+ fi
@@
- "${codecov_bin}" "${codecov_args[@]}" || echo "Warning: Codecov upload failed (non-fatal)"
rm -f "${codecov_bin}" "${codecov_bin}.SHA256SUM"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/e2e-coverage.sh` around lines 109 - 113, The current code that downloads
and verifies the Codecov binary using variables codecov_bin and codecov_version
can cause the script to exit under set -euo pipefail; make the
download/verification non-fatal by handling errors instead of letting them
propagate: perform the curl and sha256sum steps inside a conditional or use
error-catching (e.g., test the commands' exit status) and on failure emit a
warning (mentioning codecov_bin/codecov_version) and skip chmod/verification so
the script continues to the optional upload step; ensure subsequent code still
checks for the presence/executability of codecov_bin before attempting upload.
|
|
||
| .PHONY: docker-build-coverage | ||
| docker-build-coverage: ## Build coverage Docker image from images/ci/Dockerfile.coverage. | ||
| $(CONTAINER_TOOL) build -f images/ci/Dockerfile.coverage -t $(COVERAGE_IMG) . |
There was a problem hiding this comment.
Align Dockerfile path with the file introduced in this PR.
Line 42 points to images/ci/Dockerfile.coverage, while this PR introduces Dockerfile.coverage. If that path is not present, docker-build-coverage breaks.
Suggested fix
- $(CONTAINER_TOOL) build -f images/ci/Dockerfile.coverage -t $(COVERAGE_IMG) .
+ $(CONTAINER_TOOL) build -f Dockerfile.coverage -t $(COVERAGE_IMG) .📝 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.
| $(CONTAINER_TOOL) build -f images/ci/Dockerfile.coverage -t $(COVERAGE_IMG) . | |
| $(CONTAINER_TOOL) build -f Dockerfile.coverage -t $(COVERAGE_IMG) . |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` at line 42, The Makefile rule invoking the container build uses
images/ci/Dockerfile.coverage which no longer exists; update the docker build
command (the target that runs "$(CONTAINER_TOOL) build -f
images/ci/Dockerfile.coverage -t $(COVERAGE_IMG) .") to point to the new
Dockerfile.coverage location introduced in this PR (or to a Makefile variable
that references the correct path) so the docker-build-coverage target uses the
correct Dockerfile path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #350 +/- ##
===========================================
+ Coverage 62.89% 80.36% +17.47%
===========================================
Files 8 8
Lines 830 1100 +270
===========================================
+ Hits 522 884 +362
+ Misses 302 204 -98
- Partials 6 12 +6 🚀 New features to boost your workflow:
|
|
@praveencodes: The following tests failed, say
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. |
|
@praveencodes worth taking a look at CodeRabbit comments. |
4 participants
Summary
This change wires end-to-end (cluster) test coverage and Codecov into the must-gather-operator workflow.
Dockerfile.coverage — Multi-stage image that builds the operator with Go’s -cover / -coverpkg=./... instrumentation, sets GOCOVERDIR (default /tmp/e2e-cover), and prepares a writable directory so the running pod can emit coverage data.
Makefile — Adds docker-build-coverage, docker-push-coverage (image tag$(COVERAGE_IMG), default $ (IMG)-e2e-coverage), and e2e-coverage-collect to run the collection script; documents the typical local flow (build/push → patch CSV → e2e → collect/upload).
hack/e2e-coverage.sh — setup: finds the operator CSV from the deployment owner reference, patches the CSV to use COVERAGE_IMAGE, injects GOCOVERDIR, and adds an emptyDir volume mount; collect: SIGTERM to flush coverage, oc cp data off the pod, go tool covdata textfmt / percent, and optional Codecov upload (token from env or /var/run/secrets/codecov/CODECOV_TOKEN, with Prow-friendly flags when JOB_TYPE is presubmit/postsubmit).
Summary by CodeRabbit