Test: Ztwim e2e coverage#79297
Conversation
|
/pj-rehearse max |
WalkthroughThis PR adds coverage instrumentation support to the zero-trust-workload-identity-manager operator's CI pipeline. It introduces a multi-stage coverage image build, a setup step that configures the cluster to use the coverage image and injects coverage environment variables, and a post-test collection step that gathers coverage artifacts and uploads them to Codecov. ChangesCoverage Instrumentation for Operator Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
@rausingh-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rausingh-rh 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.
🧹 Nitpick comments (2)
ci-operator/config/openshift/zero-trust-workload-identity-manager/openshift-zero-trust-workload-identity-manager-main.yaml (2)
108-113: 💤 Low valueConsider potential timing issue with coverage data flush.
After sending SIGTERM to flush coverage data, there's a race between when coverage files are written and when the container restarts. The 10-second sleep is arbitrary and may not be sufficient if the process takes longer to write coverage data before exiting.
Consider adding an explicit check that coverage files exist before the container restarts, or use a more deterministic approach like checking for process termination:
oc exec -n "${NAMESPACE}" "${pod}" -c manager -- kill -s TERM 1 || true # Wait for the process to actually terminate and write coverage for i in {1..30}; do if ! oc exec -n "${NAMESPACE}" "${pod}" -c manager -- pgrep -x zero-trust-workload-identity-manager >/dev/null 2>&1; then echo "Process terminated, coverage data should be flushed" break fi sleep 1 done🤖 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 `@ci-operator/config/openshift/zero-trust-workload-identity-manager/openshift-zero-trust-workload-identity-manager-main.yaml` around lines 108 - 113, The fixed 10s sleep after sending SIGTERM to the manager container can race with coverage file flush; replace the arbitrary sleep with a deterministic wait that polls the manager process and/or the expected coverage file inside the container (using oc exec against the pod and container "manager") until the process (e.g., zero-trust-workload-identity-manager) is no longer running or the coverage file appears, with a sensible timeout (e.g., loop for up to 30s), and only proceed to oc wait once the check confirms termination or coverage file presence.
202-207: 💤 Low valueHardcoded JSON path indices are brittle.
The patch uses hardcoded indices (
deployments/0,containers/0) which assumes the CSV structure won't change. If the deployment or container order changes, this will silently patch the wrong resource or fail.This is a common pattern in CI scripts, but for robustness, consider using
jqto find the correct indices dynamically based on deployment/container names, or add a verification step that the patched container is indeed themanagercontainer.🤖 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 `@ci-operator/config/openshift/zero-trust-workload-identity-manager/openshift-zero-trust-workload-identity-manager-main.yaml` around lines 202 - 207, The patch currently uses brittle hardcoded JSON path indices in the oc patch csv call (paths containing /spec/install/spec/deployments/0/... and /containers/0) which can break if CSV ordering changes; update the script to locate the correct deployment and container dynamically (e.g., use jq to read the CSV JSON, find the index of the deployment that matches the expected name and the index of the container named "manager" or another canonical name, then construct the patch paths using those indices or patch the full object rather than by numeric index), or add a verification step after oc patch that fetches the CSV from NAMESPACE and asserts the patched container image equals COVERAGE_IMAGE and that the GOCOVERDIR_PATH env var and volume/volumeMount named "coverage-data" are present; reference oc patch csv, ${NAMESPACE}, ${COVERAGE_IMAGE}, and ${GOCOVERDIR_PATH} when implementing the dynamic lookup and verification.
🤖 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
`@ci-operator/config/openshift/zero-trust-workload-identity-manager/openshift-zero-trust-workload-identity-manager-main.yaml`:
- Around line 108-113: The fixed 10s sleep after sending SIGTERM to the manager
container can race with coverage file flush; replace the arbitrary sleep with a
deterministic wait that polls the manager process and/or the expected coverage
file inside the container (using oc exec against the pod and container
"manager") until the process (e.g., zero-trust-workload-identity-manager) is no
longer running or the coverage file appears, with a sensible timeout (e.g., loop
for up to 30s), and only proceed to oc wait once the check confirms termination
or coverage file presence.
- Around line 202-207: The patch currently uses brittle hardcoded JSON path
indices in the oc patch csv call (paths containing
/spec/install/spec/deployments/0/... and /containers/0) which can break if CSV
ordering changes; update the script to locate the correct deployment and
container dynamically (e.g., use jq to read the CSV JSON, find the index of the
deployment that matches the expected name and the index of the container named
"manager" or another canonical name, then construct the patch paths using those
indices or patch the full object rather than by numeric index), or add a
verification step after oc patch that fetches the CSV from NAMESPACE and asserts
the patched container image equals COVERAGE_IMAGE and that the GOCOVERDIR_PATH
env var and volume/volumeMount named "coverage-data" are present; reference oc
patch csv, ${NAMESPACE}, ${COVERAGE_IMAGE}, and ${GOCOVERDIR_PATH} when
implementing the dynamic lookup and verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e0f1e462-afac-445e-9913-e882d1a5ce9e
📒 Files selected for processing (1)
ci-operator/config/openshift/zero-trust-workload-identity-manager/openshift-zero-trust-workload-identity-manager-main.yaml
|
[REHEARSALNOTIFIER]
Prior to this PR being merged, you will need to either run and acknowledge or opt to skip these rehearsals. Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@rausingh-rh: 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. |
1 participant
Summary
This PR enhances the OpenShift CI configuration for the
zero-trust-workload-identity-manageroperator to enable end-to-end (e2e) coverage testing and Codecov integration.Changes Made
Modified file:
ci-operator/config/openshift/zero-trust-workload-identity-manager/openshift-zero-trust-workload-identity-manager-main.yamlCoverage Image Build
Added a new multi-stage container image (
zero-trust-workload-identity-manager-coverage) that:-cover -covermode=count)/tmp/e2e-coverdirectory with proper permissions for coverage data collectionGOCOVERDIRenvironment variable to enable Go's coverage data outputE2E Test Workflow Steps
Added two new steps to the
e2e-operatortest:1.
setup-coverage(dependency step): Patches the operator's ClusterServiceVersion (CSV) to inject the coverage image and configure the manager container with:emptyDir)GOCOVERDIRenvironment variable2.
collect-coverage(post-step): Runs after e2e tests complete to:go tool covdataImpact
This enables the zero-trust-workload-identity-manager project to automatically collect and report e2e test coverage metrics, improving visibility into operator test coverage and enabling integration with code coverage tracking services.