OAPE-695:codecov integration changes using dockerfile and .sh file#425
OAPE-695:codecov integration changes using dockerfile and .sh file#425siddhibhor-56 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references OAPE-695 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 introduces E2E coverage infrastructure for ChangesE2E Coverage Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 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: siddhibhor-56 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: 2
🧹 Nitpick comments (1)
images/ci/Dockerfile.coverage (1)
24-24: ⚡ Quick winDocument the rationale for UID/GID 65534.
The numeric UID/GID
65534corresponds to thenobodyuser/group, which is a common choice for running unprivileged containers. Consider adding a brief inline comment to clarify this choice for future maintainers.📝 Suggested comment addition
-RUN mkdir -p /tmp/e2e-cover && chown 65534:65534 /tmp/e2e-cover && chmod 700 /tmp/e2e-cover +# 65534 is the 'nobody' user/group - a standard unprivileged user for containers +RUN mkdir -p /tmp/e2e-cover && chown 65534:65534 /tmp/e2e-cover && chmod 700 /tmp/e2e-cover🤖 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 `@images/ci/Dockerfile.coverage` at line 24, Add a brief inline comment to the RUN line that changes ownership to UID/GID 65534 so future maintainers know 65534 maps to the unprivileged "nobody" user/group; update the RUN command in Dockerfile.coverage (the line that creates /tmp/e2e-cover and runs chown 65534:65534 and chmod 700) to include a short comment explaining that 65534 is used to run as an unprivileged user (nobody) to minimize permissions and improve security.
🤖 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 `@hack/e2e-coverage.sh`:
- Around line 94-99: The fixed "sleep 10" after sending SIGTERM is flaky;
replace the static delay with an adaptive wait that polls pod readiness. Remove
the sleep and instead loop (or call oc wait directly) to check the pod
referenced by "${pod}" in "${NAMESPACE}" for phase=Running and container ready
(or use oc wait pod/"${pod}" --for=condition=Ready --timeout=120s without the
prior sleep), retrying until success or timeout; ensure the logic surrounds the
oc exec kill -TERM 1 call and uses the same pod variable so it handles variable
restart times instead of a fixed 10s pause.
- Around line 122-128: The checksum verification currently does a plain cd and
then runs sha256sum, which hides failures and still attempts cd -; change this
to run verification in a safe context and abort on failure: use a subshell or
pushd/popd so directory changes cannot leak (e.g., (cd "$(dirname
"${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM"))
and check the exit status of sha256sum, exiting the script on non-zero; only run
chmod +x "${codecov_bin}" after the verification succeeds. Reference:
codecov_bin, sha256sum -c, and chmod +x "${codecov_bin}".
---
Nitpick comments:
In `@images/ci/Dockerfile.coverage`:
- Line 24: Add a brief inline comment to the RUN line that changes ownership to
UID/GID 65534 so future maintainers know 65534 maps to the unprivileged "nobody"
user/group; update the RUN command in Dockerfile.coverage (the line that creates
/tmp/e2e-cover and runs chown 65534:65534 and chmod 700) to include a short
comment explaining that 65534 is used to run as an unprivileged user (nobody) to
minimize permissions and improve security.
🪄 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: d26f2852-30f2-42db-96f6-5f664ed4041a
📒 Files selected for processing (3)
Makefilehack/e2e-coverage.shimages/ci/Dockerfile.coverage
| echo "Sending SIGTERM to operator process to flush coverage data..." | ||
| oc exec -n "${NAMESPACE}" "${pod}" -c cert-manager-operator -- /bin/sh -c 'kill -TERM 1' || true | ||
|
|
||
| echo "Waiting for container to restart..." | ||
| sleep 10 | ||
| oc wait pod/"${pod}" --for=condition=Ready -n "${NAMESPACE}" --timeout=120s |
There was a problem hiding this comment.
Hardcoded sleep may be insufficient for container restart.
The script waits exactly 10 seconds after sending SIGTERM before checking pod readiness. Container restart time can vary based on image pull policies, resource constraints, and cluster load. This fixed delay creates a race condition that may cause premature readiness checks.
⏱️ Proposed fix to add adaptive wait logic
echo "Sending SIGTERM to operator process to flush coverage data..."
oc exec -n "${NAMESPACE}" "${pod}" -c cert-manager-operator -- /bin/sh -c 'kill -TERM 1' || true
- echo "Waiting for container to restart..."
- sleep 10
- oc wait pod/"${pod}" --for=condition=Ready -n "${NAMESPACE}" --timeout=120s
+ echo "Waiting for pod to become NotReady (container restarting)..."
+ for i in {1..30}; do
+ if ! oc get pod/"${pod}" -n "${NAMESPACE}" -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' | grep -q "True"; then
+ echo "Pod is restarting..."
+ break
+ fi
+ sleep 1
+ done
+
+ echo "Waiting for pod to become Ready again..."
+ oc wait pod/"${pod}" --for=condition=Ready -n "${NAMESPACE}" --timeout=120s📝 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.
| echo "Sending SIGTERM to operator process to flush coverage data..." | |
| oc exec -n "${NAMESPACE}" "${pod}" -c cert-manager-operator -- /bin/sh -c 'kill -TERM 1' || true | |
| echo "Waiting for container to restart..." | |
| sleep 10 | |
| oc wait pod/"${pod}" --for=condition=Ready -n "${NAMESPACE}" --timeout=120s | |
| echo "Sending SIGTERM to operator process to flush coverage data..." | |
| oc exec -n "${NAMESPACE}" "${pod}" -c cert-manager-operator -- /bin/sh -c 'kill -TERM 1' || true | |
| echo "Waiting for pod to become NotReady (container restarting)..." | |
| for i in {1..30}; do | |
| if ! oc get pod/"${pod}" -n "${NAMESPACE}" -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' | grep -q "True"; then | |
| echo "Pod is restarting..." | |
| break | |
| fi | |
| sleep 1 | |
| done | |
| echo "Waiting for pod to become Ready again..." | |
| oc wait pod/"${pod}" --for=condition=Ready -n "${NAMESPACE}" --timeout=120s |
🤖 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 94 - 99, The fixed "sleep 10" after
sending SIGTERM is flaky; replace the static delay with an adaptive wait that
polls pod readiness. Remove the sleep and instead loop (or call oc wait
directly) to check the pod referenced by "${pod}" in "${NAMESPACE}" for
phase=Running and container ready (or use oc wait pod/"${pod}"
--for=condition=Ready --timeout=120s without the prior sleep), retrying until
success or timeout; ensure the logic surrounds the oc exec kill -TERM 1 call and
uses the same pod variable so it handles variable restart times instead of a
fixed 10s pause.
| curl -sS -o "${codecov_bin}" \ | ||
| https://uploader.codecov.io/latest/linux/codecov | ||
| curl -sS -o "${codecov_bin}.SHA256SUM" \ | ||
| https://uploader.codecov.io/latest/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.
Improve error handling in checksum verification.
The directory change for sha256sum verification (line 127) does not properly handle failures. If cd fails or the checksum verification fails, the script continues to cd - which may fail or return to the wrong directory, and the error is suppressed by redirecting to /dev/null.
🔒 Proposed fix for safer checksum verification
- cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM" && cd - >/dev/null
+ if ! (cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM"); then
+ echo "Error: Codecov binary checksum verification failed"
+ exit 1
+ fi📝 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.
| curl -sS -o "${codecov_bin}" \ | |
| https://uploader.codecov.io/latest/linux/codecov | |
| curl -sS -o "${codecov_bin}.SHA256SUM" \ | |
| https://uploader.codecov.io/latest/linux/codecov.SHA256SUM | |
| cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM" && cd - >/dev/null | |
| chmod +x "${codecov_bin}" | |
| curl -sS -o "${codecov_bin}" \ | |
| https://uploader.codecov.io/latest/linux/codecov | |
| curl -sS -o "${codecov_bin}.SHA256SUM" \ | |
| https://uploader.codecov.io/latest/linux/codecov.SHA256SUM | |
| if ! (cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename "${codecov_bin}").SHA256SUM"); then | |
| echo "Error: Codecov binary checksum verification failed" | |
| exit 1 | |
| fi | |
| chmod +x "${codecov_bin}" |
🤖 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 122 - 128, The checksum verification
currently does a plain cd and then runs sha256sum, which hides failures and
still attempts cd -; change this to run verification in a safe context and abort
on failure: use a subshell or pushd/popd so directory changes cannot leak (e.g.,
(cd "$(dirname "${codecov_bin}")" && sha256sum -c "$(basename
"${codecov_bin}").SHA256SUM")) and check the exit status of sha256sum, exiting
the script on non-zero; only run chmod +x "${codecov_bin}" after the
verification succeeds. Reference: codecov_bin, sha256sum -c, and chmod +x
"${codecov_bin}".
|
@siddhibhor-56: 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. |
2 participants
Summary by CodeRabbit