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 | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 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
| 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}".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/e2e-coverage.sh (2)
84-91: 💤 Low valuePod selection during rollout edge case.
Using
{.items[0].metadata.name}selects the first pod matching the label. During transient states (e.g., partial rollout), this could pick a terminating pod. Consider filtering bystatus.phase=Runningor adding--field-selector=status.phase=Runningto ensure the active pod is selected.This is likely fine in practice since
setupwaits for rollout completion before tests run.🤖 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 84 - 91, The pod selection currently uses oc get pod ... -o jsonpath='{.items[0].metadata.name}' which can return a terminating pod; update the oc invocation to restrict to running pods by adding a field selector or filter (e.g., include --field-selector=status.phase=Running or adjust jsonpath to pick an item whose status.phase == "Running") so that the variable pod (and the POD_LABEL/NAMESPACE lookup) always points to an active pod; keep the rest of the error handling and echo "Operator pod: ${pod}" unchanged.
36-41: 💤 Low valueHardcoded JSON patch indices assume specific CSV structure.
The paths
deployments/0andcontainers/0assume the operator deployment is always the first deployment in the CSV and uses the first container. If the CSV structure ever changes, this could silently patch the wrong resource.Consider using
jqor a more targeted approach to find the deployment/container by name rather than index, or add a comment documenting this assumption.🤖 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 36 - 41, The JSON patch currently hardcodes array indices in the oc patch call (paths containing deployments/0 and containers/0), which can target the wrong deployment/container if the CSV order changes; update the script to locate the correct deployment and container by name before patching (e.g., use jq to read .spec.install.spec.deployments[] to find the index for the deployment with the expected name and its containers[] to find the container index, then construct the JSON patch with those computed indices or patch by replacing the whole deployment object), referencing the oc patch invocation and variables COVERAGE_IMAGE and GOCOVERDIR_PATH to build the final patch; if you intentionally keep the index approach, add a clear comment documenting the assumption and fail the script if the expected named deployment/container is not found.
🤖 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.
Nitpick comments:
In `@hack/e2e-coverage.sh`:
- Around line 84-91: The pod selection currently uses oc get pod ... -o
jsonpath='{.items[0].metadata.name}' which can return a terminating pod; update
the oc invocation to restrict to running pods by adding a field selector or
filter (e.g., include --field-selector=status.phase=Running or adjust jsonpath
to pick an item whose status.phase == "Running") so that the variable pod (and
the POD_LABEL/NAMESPACE lookup) always points to an active pod; keep the rest of
the error handling and echo "Operator pod: ${pod}" unchanged.
- Around line 36-41: The JSON patch currently hardcodes array indices in the oc
patch call (paths containing deployments/0 and containers/0), which can target
the wrong deployment/container if the CSV order changes; update the script to
locate the correct deployment and container by name before patching (e.g., use
jq to read .spec.install.spec.deployments[] to find the index for the deployment
with the expected name and its containers[] to find the container index, then
construct the JSON patch with those computed indices or patch by replacing the
whole deployment object), referencing the oc patch invocation and variables
COVERAGE_IMAGE and GOCOVERDIR_PATH to build the final patch; if you
intentionally keep the index approach, add a clear comment documenting the
assumption and fail the script if the expected named deployment/container is not
found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a09920f2-42fc-4c6e-9994-8a55586d507b
📒 Files selected for processing (3)
Makefilehack/e2e-coverage.shimages/ci/Dockerfile.coverage
🚧 Files skipped from review as they are similar to previous changes (2)
- images/ci/Dockerfile.coverage
- Makefile
|
@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