OAPE-693:adds the e2e coverage collector for the operator using codecov#138
Open
siddhibhor-56 wants to merge 2 commits into
Open
OAPE-693:adds the e2e coverage collector for the operator using codecov#138siddhibhor-56 wants to merge 2 commits into
siddhibhor-56 wants to merge 2 commits into
Conversation
|
[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 |
|
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:
WalkthroughAdds E2E coverage support: Makefile targets and variable, a coverage-builder Dockerfile, and a new ChangesE2E Coverage integration
* Small unrelated addition included in the same PR. Sequence DiagramsequenceDiagram
participant CI as CI Runner
participant K8s as Kubernetes API
participant Operator as Operator Pod
participant PVC as Coverage PVC
participant Extractor as Extractor Pod
participant Codecov as Codecov
CI->>K8s: hack/e2e-coverage.sh setup (create PVC, patch CSV to coverage image)
K8s->>Operator: rollout controller with coverage image and PVC mounted (GOCOVERDIR set)
CI->>K8s: run E2E tests against instrumented Operator
CI->>K8s: hack/e2e-coverage.sh collect (scale down operator)
Operator->>PVC: write coverage files to PVC on shutdown
CI->>K8s: create Extractor Pod mounting PVC
Extractor->>PVC: read and copy coverage files
Extractor-->>CI: copy artifacts to CI workspace
CI->>CI: convert covmeta -> go coverage profile
CI->>Codecov: upload profile if `CODECOV_TOKEN` present
Codecov-->>CI: upload response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@images/ci/Dockerfile`:
- Around line 30-31: The Dockerfile currently uses "RUN mkdir -p /tmp/e2e-cover
&& chmod 777 /tmp/e2e-cover" which creates a world-writable directory; instead
create the directory, chown it to UID/GID 65534 and set restrictive permissions
(e.g. 0700 or 0750) so only the intended user can write; update the RUN that
creates /tmp/e2e-cover to use chown 65534:65534 and chmod 700 (or 750 if group
write is needed) rather than chmod 777.
In `@Makefile`:
- Around line 234-243: The kubectl cp invocation in the Makefile is causing a
nested e2e-cover directory, so change the kubectl cp usage to copy the contents
of /tmp/e2e-cover directly into $(E2E_COVERAGE_DIR); update the kubectl cp line
that references $(KUBECTL) cp "$(E2E_COVERAGE_NAMESPACE)/$$POD:/tmp/e2e-cover"
$(E2E_COVERAGE_DIR) to use a trailing dot on the source (e.g.
"$(E2E_COVERAGE_NAMESPACE)/$$POD:/tmp/e2e-cover/." $(E2E_COVERAGE_DIR)) so
covmeta and covcounters land directly in $(E2E_COVERAGE_DIR) for go tool covdata
to consume.
- Around line 245-248: Replace the unsafe "latest" downloader invocation in the
Makefile (the curl/chmod/./codecov sequence) with a pinned release workflow:
download a specific versioned uploader binary URL (not "latest") and its
corresponding SHA256SUM (or .sha256) file, verify the binary against the
checksum (using sha256sum -c or equivalent) before making it executable and
running it (the ./codecov invocation), and fail the step if verification fails;
keep the existing flags (--file=$(OUTPUTS_PATH)/coverage-e2e.out --flags=e2e
--name="E2E Coverage" --verbose) and still remove the binary afterwards. Ensure
the Makefile references the chosen version string so it is explicit and
reproducible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ff32fe0-a38f-4c43-ab77-dfdadbf0a804
📒 Files selected for processing (2)
Makefileimages/ci/Dockerfile
There was a problem hiding this comment.
🧹 Nitpick comments (10)
test/utils/bitwarden_resources.go (2)
26-32: Constants are duplicated withaws_resources.go.The constants
externalSecretsAPIVersionV1andclusterSecretStoreKindare also defined intest/utils/aws_resources.go. Consider extracting these to a shared constants file within theutilspackage to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden_resources.go` around lines 26 - 32, The constants externalSecretsAPIVersionV1 and clusterSecretStoreKind are duplicated across bitwarden_resources.go and aws_resources.go; extract these into a shared utils package constants file (e.g., constants.go) and replace the local definitions in both bitwarden_resources.go and aws_resources.go with references to the new shared constants (externalSecretsAPIVersionV1, clusterSecretStoreKind), removing the duplicate declarations so both files import/use the single source of truth.
70-96: Consider handlingSetNestedFielderrors.While
SetNestedFieldtypically only fails if the path is structurally invalid (which won't happen with these compile-time known paths), explicitly checking the error would make the code more defensive.- _ = unstructured.SetNestedField(u.Object, []interface{}{ + if err := unstructured.SetNestedField(u.Object, []interface{}{ map[string]interface{}{ "secretKey": "value", "remoteRef": map[string]interface{}{ "key": remoteKey, }, }, - }, "spec", "data") + }, "spec", "data"); err != nil { + panic(fmt.Sprintf("failed to set spec.data: %v", err)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden_resources.go` around lines 70 - 96, The SetNestedField calls in BitwardenExternalSecretByName and BitwardenExternalSecretByUUID ignore returned errors; update both functions to capture the error from unstructured.SetNestedField and handle it (e.g., if err != nil { panic(fmt.Errorf("failed to set spec.data for %s: %w", name, err)) }) so failures are surfaced; reference the SetNestedField call sites in BitwardenExternalSecretByName and BitwardenExternalSecretByUUID and include the original error text in the panic/log message for diagnosability.test/utils/bitwarden_api_runner.go (1)
35-38: Consider pinning the curl image tag for reproducibility.Using
docker.io/curlimages/curl:latestcan lead to non-reproducible test behavior if the image is updated with breaking changes. Consider pinning to a specific version.const ( - bitwardenAPITestRunnerImage = "docker.io/curlimages/curl:latest" + bitwardenAPITestRunnerImage = "docker.io/curlimages/curl:8.7.1" bitwardenCredMountPath = "/etc/bitwarden-cred" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden_api_runner.go` around lines 35 - 38, The constant bitwardenAPITestRunnerImage currently uses the mutable tag "docker.io/curlimages/curl:latest" which can cause flaky tests; change the value of bitwardenAPITestRunnerImage to a specific, immutable curlimages tag (e.g., a known semver like :8.x.y) and add a short comment noting why the tag is pinned; ensure any CI/test docs referencing this constant are updated when you intentionally upgrade the pinned tag.Makefile (2)
182-182: Add PHONY declaration for the targets on this line.The static analysis tool flagged that targets listed here should be declared as
.PHONYto ensure they're always executed regardless of whether a file with that name exists.+.PHONY: fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep: GOFLAGS=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 182, The Makefile line defining the combined targets "fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep: GOFLAGS=" lacks a .PHONY declaration, so add a .PHONY rule listing these exact target names (fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor update-dep) elsewhere in the Makefile to ensure they always run regardless of files with those names; update the .PHONY block to include all these targets.
219-222: Codecov uploader pinning and SHA256 verification are correct.The implementation correctly pins v0.7.6 and verifies the SHA256 checksum before execution. v0.7.6 is a valid release. However, v0.8.0 is now the latest available version. If this version is not pinned for specific compatibility reasons, consider upgrading to v0.8.0 to benefit from any bug fixes or security patches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 219 - 222, Update the pinned Codecov uploader by changing the CODECOV_VERSION variable from v0.7.6 to v0.8.0 and adjust the derived URLs (CODECOV_URL and CODECOV_SHA_URL) will automatically follow; if there's a deliberate compatibility constraint, add a brief comment next to CODECOV_VERSION explaining why v0.7.6 must remain pinned instead of upgrading to v0.8.0.test/utils/bitwarden.go (2)
182-201: Consider validating organization and project IDs.
FetchBitwardenCredsFromSecretvalidates thattokenis non-empty but silently returns empty strings fororgIDandprojectIDif they're missing. If these are required for Bitwarden API operations, consider validating them as well.if v, ok := secret.Data[BitwardenCredKeyOrgID]; ok { orgID = string(v) + } else { + return "", "", "", fmt.Errorf("bitwarden creds secret %s/%s missing required key %q", secretNamespace, secretName, BitwardenCredKeyOrgID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden.go` around lines 182 - 201, FetchBitwardenCredsFromSecret currently only enforces token presence but silently allows orgID and projectID to be empty; update the function to validate BitwardenCredKeyOrgID and BitwardenCredKeyProjectID like TokenSecretKey and return a descriptive error if either is missing or empty (use the same error formatting pattern as the existing token check, referencing secretNamespace/secretName and the missing key names) so callers receive a clear failure when required organization or project IDs are absent.
326-341: Permissive error handling may mask connectivity issues.The reachability check treats
http_code=000(connection refused/timeout/TLS failure) and empty logs as success to avoid failing the suite. While this is pragmatic, it could mask real connectivity problems that would cause subsequent tests to fail with less informative errors.Consider at minimum logging a warning when these permissive paths are taken, so operators can investigate if tests fail later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/bitwarden.go` around lines 326 - 341, In the PodFailed case of the reachability check (the switch handling corev1.PodFailed), add a warning log before returning true on the permissive paths so connectivity issues aren't silently ignored: when getPodLogs(...) contains "http_code=000" or when logs contain "error on the server" or are empty/whitespace, call the test/logger warning function to record the pod/container identity (use formatPodContainerStatus(p) and podName/BitwardenOperandNamespace) and the raw logs, then return true,nil as before.test/utils/dynamic_resources.go (1)
112-127: Input mutation may surprise callers.
getResourceInterfacemutates the inputunstructuredObjby callingSetNamespace(Line 121). This side effect could be unexpected for callers who pass an object they intend to reuse. Consider documenting this behavior or working with a copy.func (d DynamicResourceLoader) getResourceInterface(unstructuredObj *unstructured.Unstructured, overrideNamespace string) dynamic.ResourceInterface { gvk := unstructuredObj.GroupVersionKind() gr, err := restmapper.GetAPIGroupResources(d.KubeClient.Discovery()) require.NoError(d.t, err) mapper := restmapper.NewDiscoveryRESTMapper(gr) mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version) require.NoError(d.t, err) if mapping.Scope.Name() == meta.RESTScopeNameNamespace { if overrideNamespace != "" { - unstructuredObj.SetNamespace(overrideNamespace) + unstructuredObj.SetNamespace(overrideNamespace) // Note: mutates input object } require.NotEmpty(d.t, unstructuredObj.GetNamespace(), "Namespace can not be empty for namespaced resource") return d.DynamicClient.Resource(mapping.Resource).Namespace(unstructuredObj.GetNamespace()) } return d.DynamicClient.Resource(mapping.Resource) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/dynamic_resources.go` around lines 112 - 127, The getResourceInterface function currently mutates the caller's unstructuredObj by calling SetNamespace on it when applying overrideNamespace; avoid surprising side-effects by operating on a copy: create a deep copy of unstructuredObj (e.g., via unstructuredObj.DeepCopy()), set the namespace on the copy rather than the original, and use the copy's GetNamespace when selecting the Resource Interface; alternatively, clearly document that getResourceInterface will mutate unstructuredObj if overrideNamespace is provided. Ensure references to SetNamespace, unstructuredObj, overrideNamespace and getResourceInterface are updated accordingly.test/utils/artifact_dump.go (1)
171-177: Consider pre-compiling the regex for sanitizeFilename.The regex is compiled on every call. While acceptable for e2e test utilities where this runs infrequently, pre-compiling at package level would be slightly more efficient.
♻️ Optional: Pre-compile regex
+var sanitizeFilenameRe = regexp.MustCompile(`[^a-zA-Z0-9_-]`) + func sanitizeFilename(s string) string { - s = regexp.MustCompile(`[^a-zA-Z0-9_-]`).ReplaceAllString(s, "_") + s = sanitizeFilenameRe.ReplaceAllString(s, "_") if len(s) > 100 { s = s[:100] } return s }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/artifact_dump.go` around lines 171 - 177, The sanitizeFilename function currently compiles the regex on every call; move the regexp.MustCompile(`[^a-zA-Z0-9_-]`) to a package-level variable (e.g., filenameSanitizeRegex) and update sanitizeFilename to use filenameSanitizeRegex.ReplaceAllString(s, "_") so the regex is compiled once and behavior (truncation to 100 chars) remains unchanged.test/e2e/bitwarden_api_test.go (1)
139-144: Consider documenting why both 200 and 400 are acceptable.The test accepts either HTTP 200 or 400 for empty
idslist. Adding a brief comment explaining the expected API behavior variance would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/bitwarden_api_test.go` around lines 139 - 144, Add a short comment above the It("GET /rest/api/1/secrets-by-ids with empty ids should return 200 or 400", ...) test explaining why both HTTP 200 and 400 are accepted (e.g., different implementations may treat an empty ids list as a valid no-op returning 200 or as a client error returning 400), so future readers of the test (and maintainers of runAPIJob/script and the API contract) understand the intended variance; reference the test name, the script variable that posts '{"ids":[]}', and runAPIJob to locate the exact spot to insert the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Line 182: The Makefile line defining the combined targets "fmt vet test
test-unit test-e2e e2e-coverage-publish run update-vendor update-dep: GOFLAGS="
lacks a .PHONY declaration, so add a .PHONY rule listing these exact target
names (fmt vet test test-unit test-e2e e2e-coverage-publish run update-vendor
update-dep) elsewhere in the Makefile to ensure they always run regardless of
files with those names; update the .PHONY block to include all these targets.
- Around line 219-222: Update the pinned Codecov uploader by changing the
CODECOV_VERSION variable from v0.7.6 to v0.8.0 and adjust the derived URLs
(CODECOV_URL and CODECOV_SHA_URL) will automatically follow; if there's a
deliberate compatibility constraint, add a brief comment next to CODECOV_VERSION
explaining why v0.7.6 must remain pinned instead of upgrading to v0.8.0.
In `@test/e2e/bitwarden_api_test.go`:
- Around line 139-144: Add a short comment above the It("GET
/rest/api/1/secrets-by-ids with empty ids should return 200 or 400", ...) test
explaining why both HTTP 200 and 400 are accepted (e.g., different
implementations may treat an empty ids list as a valid no-op returning 200 or as
a client error returning 400), so future readers of the test (and maintainers of
runAPIJob/script and the API contract) understand the intended variance;
reference the test name, the script variable that posts '{"ids":[]}', and
runAPIJob to locate the exact spot to insert the comment.
In `@test/utils/artifact_dump.go`:
- Around line 171-177: The sanitizeFilename function currently compiles the
regex on every call; move the regexp.MustCompile(`[^a-zA-Z0-9_-]`) to a
package-level variable (e.g., filenameSanitizeRegex) and update sanitizeFilename
to use filenameSanitizeRegex.ReplaceAllString(s, "_") so the regex is compiled
once and behavior (truncation to 100 chars) remains unchanged.
In `@test/utils/bitwarden_api_runner.go`:
- Around line 35-38: The constant bitwardenAPITestRunnerImage currently uses the
mutable tag "docker.io/curlimages/curl:latest" which can cause flaky tests;
change the value of bitwardenAPITestRunnerImage to a specific, immutable
curlimages tag (e.g., a known semver like :8.x.y) and add a short comment noting
why the tag is pinned; ensure any CI/test docs referencing this constant are
updated when you intentionally upgrade the pinned tag.
In `@test/utils/bitwarden_resources.go`:
- Around line 26-32: The constants externalSecretsAPIVersionV1 and
clusterSecretStoreKind are duplicated across bitwarden_resources.go and
aws_resources.go; extract these into a shared utils package constants file
(e.g., constants.go) and replace the local definitions in both
bitwarden_resources.go and aws_resources.go with references to the new shared
constants (externalSecretsAPIVersionV1, clusterSecretStoreKind), removing the
duplicate declarations so both files import/use the single source of truth.
- Around line 70-96: The SetNestedField calls in BitwardenExternalSecretByName
and BitwardenExternalSecretByUUID ignore returned errors; update both functions
to capture the error from unstructured.SetNestedField and handle it (e.g., if
err != nil { panic(fmt.Errorf("failed to set spec.data for %s: %w", name, err))
}) so failures are surfaced; reference the SetNestedField call sites in
BitwardenExternalSecretByName and BitwardenExternalSecretByUUID and include the
original error text in the panic/log message for diagnosability.
In `@test/utils/bitwarden.go`:
- Around line 182-201: FetchBitwardenCredsFromSecret currently only enforces
token presence but silently allows orgID and projectID to be empty; update the
function to validate BitwardenCredKeyOrgID and BitwardenCredKeyProjectID like
TokenSecretKey and return a descriptive error if either is missing or empty (use
the same error formatting pattern as the existing token check, referencing
secretNamespace/secretName and the missing key names) so callers receive a clear
failure when required organization or project IDs are absent.
- Around line 326-341: In the PodFailed case of the reachability check (the
switch handling corev1.PodFailed), add a warning log before returning true on
the permissive paths so connectivity issues aren't silently ignored: when
getPodLogs(...) contains "http_code=000" or when logs contain "error on the
server" or are empty/whitespace, call the test/logger warning function to record
the pod/container identity (use formatPodContainerStatus(p) and
podName/BitwardenOperandNamespace) and the raw logs, then return true,nil as
before.
In `@test/utils/dynamic_resources.go`:
- Around line 112-127: The getResourceInterface function currently mutates the
caller's unstructuredObj by calling SetNamespace on it when applying
overrideNamespace; avoid surprising side-effects by operating on a copy: create
a deep copy of unstructuredObj (e.g., via unstructuredObj.DeepCopy()), set the
namespace on the copy rather than the original, and use the copy's GetNamespace
when selecting the Resource Interface; alternatively, clearly document that
getResourceInterface will mutate unstructuredObj if overrideNamespace is
provided. Ensure references to SetNamespace, unstructuredObj, overrideNamespace
and getResourceInterface are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a9d37d64-68e3-464b-9b91-29402719f719
📒 Files selected for processing (20)
MakefileREADME.mdhack/govulncheck.shimages/ci/Dockerfile.codecoveragetest/README.mdtest/apis/README.mdtest/e2e/README.mdtest/e2e/bitwarden_api_test.gotest/e2e/bitwarden_es_test.gotest/e2e/e2e_suite_test.gotest/e2e/e2e_test.gotest/go.modtest/utils/artifact_dump.gotest/utils/aws_resources.gotest/utils/bitwarden.gotest/utils/bitwarden_api_runner.gotest/utils/bitwarden_resources.gotest/utils/cleanup.gotest/utils/conditions.gotest/utils/dynamic_resources.go
✅ Files skipped from review due to trivial changes (6)
- README.md
- test/README.md
- test/apis/README.md
- hack/govulncheck.sh
- test/go.mod
- test/e2e/README.md
siddhibhor-56
force-pushed
the
codecov
branch
3 times, most recently
from
3604751 to
8f666be
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 97-118: The collect() function can leave the coverage-extractor
pod running if a later command fails; add a cleanup trap so the pod is always
removed on EXIT. Implement a small cleanup function (e.g., cleanup_extractor)
that runs oc delete pod coverage-extractor -n "${NAMESPACE}" --ignore-not-found
--wait=false 2>/dev/null || true, set trap 'cleanup_extractor' EXIT immediately
after creating the pod (right after the oc run/oc wait sequence), and
remove/reset the trap (trap - EXIT) once files are successfully copied and
before returning so normal completion doesn't trigger redundant cleanup.
- Around line 58-63: The CSV patch currently uses append ops to add env,
volumeMounts and volumes which will create duplicates if run twice; modify the
oc patch logic around oc patch "${csv}" to first check for an existing env entry
named GOCOVERDIR (or existing volume/volumeMount named coverage-data) and skip
the corresponding add ops if present, or alternatively replace the whole
container spec in
/spec/install/spec/deployments/0/spec/template/spec/containers/0 with a
sanitized container object that sets image to COVERAGE_IMAGE and sets env
GOCOVERDIR to GOCOVERDIR_PATH and the volumeMounts/volumes (using PVC_NAME),
ensuring you update the patch operations to use replace on the container array
or conditional logic to avoid appending duplicates.
🪄 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: a6f2a1e6-94ca-4808-93c6-5d2746b62eb7
📒 Files selected for processing (3)
Makefilehack/e2e-coverage.shimages/ci/Dockerfile.coverage
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/govulncheck.sh`:
- Around line 27-29: The long-lived suppression in KNOWN_VULNS_PATTERN currently
hardcodes operator-impacting vuln IDs (e.g., GO-2026-4971, GO-2026-4918); change
this to be temporary by either removing these IDs entirely or replacing them
with an explicit temporary allowlist that includes an expiry date and an
owner/issue reference (e.g., add metadata comment or use an expirable mechanism
like ENV var or a separate TEMP_ALLOWLIST variable) so future reintroductions
aren’t silently ignored; locate the KNOWN_VULNS_PATTERN declaration and update
it to reference the temporary mechanism or remove the operator-impacting IDs and
add a comment noting the owner, expiry (YYYY-MM-DD), and tracking issue number.
🪄 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: 17213dee-2264-4a7c-8ca0-61150ae6ce21
📒 Files selected for processing (2)
hack/govulncheck.shimages/ci/Dockerfile.coverage
🚧 Files skipped from review as they are similar to previous changes (1)
- images/ci/Dockerfile.coverage
Comment on lines
+27
to
+29
| # - https://pkg.go.dev/vuln/GO-2026-4971 - Dial and LookupPort panic on Windows with NUL input in net | ||
| # - https://pkg.go.dev/vuln/GO-2026-4918 - HTTP/2 infinite loop via SETTINGS_MAX_FRAME_SIZE of 0 in net/http, golang.org/x/net | ||
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602|GO-2026-4971|GO-2026-4918" |
There was a problem hiding this comment.
Avoid permanent suppression of operator-impacting vuln IDs.
These IDs are documented as impacting operator code, but adding them directly to the long-lived allowlist means future reintroductions will stay silent in CI. Make these suppressions explicitly temporary (expiry + owner/issue), or remove them so the pipeline blocks until fixed downstream.
Suggested hardening (temporary suppression with expiry)
+# Temporary suppressions for downstream-unavailable fixes.
+# Must be revisited before this date.
+TEMP_IGNORE_UNTIL="2026-06-30"
+if [[ "$(date +%F)" > "${TEMP_IGNORE_UNTIL}" ]]; then
+ echo "-- ERROR -- Temporary govulncheck suppressions expired on ${TEMP_IGNORE_UNTIL}"
+ exit 1
+fi
+
KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602|GO-2026-4971|GO-2026-4918"📝 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.
Suggested change
| # - https://pkg.go.dev/vuln/GO-2026-4971 - Dial and LookupPort panic on Windows with NUL input in net | |
| # - https://pkg.go.dev/vuln/GO-2026-4918 - HTTP/2 infinite loop via SETTINGS_MAX_FRAME_SIZE of 0 in net/http, golang.org/x/net | |
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602|GO-2026-4971|GO-2026-4918" | |
| # - https://pkg.go.dev/vuln/GO-2026-4971 - Dial and LookupPort panic on Windows with NUL input in net | |
| # - https://pkg.go.dev/vuln/GO-2026-4918 - HTTP/2 infinite loop via SETTINGS_MAX_FRAME_SIZE of 0 in net/http, golang.org/x/net | |
| # Temporary suppressions for downstream-unavailable fixes. | |
| # Must be revisited before this date. | |
| TEMP_IGNORE_UNTIL="2026-06-30" | |
| if [[ "$(date +%F)" > "${TEMP_IGNORE_UNTIL}" ]]; then | |
| echo "-- ERROR -- Temporary govulncheck suppressions expired on ${TEMP_IGNORE_UNTIL}" | |
| exit 1 | |
| fi | |
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602|GO-2026-4971|GO-2026-4918" |
🤖 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/govulncheck.sh` around lines 27 - 29, The long-lived suppression in
KNOWN_VULNS_PATTERN currently hardcodes operator-impacting vuln IDs (e.g.,
GO-2026-4971, GO-2026-4918); change this to be temporary by either removing
these IDs entirely or replacing them with an explicit temporary allowlist that
includes an expiry date and an owner/issue reference (e.g., add metadata comment
or use an expirable mechanism like ENV var or a separate TEMP_ALLOWLIST
variable) so future reintroductions aren’t silently ignored; locate the
KNOWN_VULNS_PATTERN declaration and update it to reference the temporary
mechanism or remove the operator-impacting IDs and add a comment noting the
owner, expiry (YYYY-MM-DD), and tracking issue number.
Contributor
Author
|
/test verify |
siddhibhor-56
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@siddhibhor-56: This pull request references OAPE-693 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 story 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. |
|
@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. |
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602" | ||
| # - https://pkg.go.dev/vuln/GO-2026-4971 - Dial and LookupPort panic on Windows with NUL input in net | ||
| # - https://pkg.go.dev/vuln/GO-2026-4918 - HTTP/2 infinite loop via SETTINGS_MAX_FRAME_SIZE of 0 in net/http, golang.org/x/net | ||
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602|GO-2026-4971|GO-2026-4918" |
Contributor
There was a problem hiding this comment.
Suggested change
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4601|GO-2026-4602|GO-2026-4971|GO-2026-4918" | |
| KNOWN_VULNS_PATTERN="GO-2025-3521|GO-2025-3547|GO-2026-4971|GO-2026-4918" |
| ## | ||
| ## Targets for building a coverage-instrumented operator image, collecting | ||
| ## coverage data written during E2E tests, and uploading the report to Codecov. | ||
| ## Uses emptyDir (no PVC): the collect step sends SIGTERM to the operator |
Contributor
There was a problem hiding this comment.
Does the comment need an update regarding PVC?
| if [[ -n "${CODECOV_TOKEN:-}" ]]; then | ||
| echo "Uploading to Codecov..." | ||
| local codecov_bin="${artifact_dir}/codecov" | ||
| curl -sS -o "${codecov_bin}" https://uploader.codecov.io/latest/linux/codecov |
Contributor
There was a problem hiding this comment.
We should pin and download a particular version, which will help with debugging.
| ]" | ||
|
|
||
| echo "Waiting for operator rollout with coverage image..." | ||
| sleep 5 |
Contributor
There was a problem hiding this comment.
We should poll or instead use oc wait, this could cause flakiness.
Comment on lines
+84
to
+85
| sleep 10 | ||
| oc wait pod/"${pod}" --for=condition=Ready -n "${NAMESPACE}" --timeout=120s |
Contributor
There was a problem hiding this comment.
Do we need both sleep and wait?
| --file="${coverage_profile}" | ||
| --flags=e2e | ||
| --name="E2E Coverage" | ||
| --verbose |
Contributor
There was a problem hiding this comment.
Do we really need it to be verbose, if so, please make sure, token is not leaked in the logs.
| -cover -covermode=count -coverpkg=./... \ | ||
| -o external-secrets-operator cmd/external-secrets-operator/main.go | ||
|
|
||
| FROM registry.access.redhat.com/ubi9-minimal:9.4 |
Contributor
There was a problem hiding this comment.
Suggested change
| FROM registry.access.redhat.com/ubi9-minimal:9.4 | |
| FROM registry.access.redhat.com/ubi9/ubi-minimal:latest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the e2e coverage collector for the operator using codecov
Summary by CodeRabbit