chore(SREP-4482, SREP-4486, SREP-4800: Boilerplate Update for Agentic SDLC Rollout)#443
chore(SREP-4482, SREP-4486, SREP-4800: Boilerplate Update for Agentic SDLC Rollout)#443devppratik wants to merge 8 commits intoopenshift:masterfrom
Conversation
Updates from openshift/boilerplate repository. Related: https://issues.redhat.com/browse/ROSA-730 Update with changes
openshift-ci-robot
added
the
jira/valid-reference
|
@devppratik: This pull request references SREP-4482 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. This pull request references SREP-4486 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. This pull request references SREP-4800 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThreads context.Context through controllers, clients, helpers and tests (replacing context.TODO()), updates related function signatures, and separately updates CI/build/config (pre-commit, codecov, Docker image tags) and OWNERS_ALIASES. ChangesContext propagation across controllers, clients, and helpers
Infrastructure, CI and repository hygiene
sequenceDiagram
participant Controller
participant KubeAPI
participant LetsEncrypt
participant DNSProvider
participant DB
Controller->>KubeAPI: Reconcile(ctx) -> Get/List/Update (with ctx)
Controller->>LetsEncrypt: NewClient(ctx) / Revoke/Issue (ctx)
Controller->>DNSProvider: VerifyDnsResourceRecordUpdate(ctx)
Controller->>KubeAPI: Create/Update Secret (with ctx)
Controller->>DB: QueryRowContext / metrics ops (with ctx)
KubeAPI-->>Controller: Resource objects / Secrets
LetsEncrypt-->>Controller: ACME responses
DNSProvider-->>Controller: DNS lookup responses
DB-->>Controller: Query results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: devppratik 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
🧹 Nitpick comments (2)
.codecov.yml (1)
11-18: ⚡ Quick winLGTM — valid CodeCov configuration activating coverage gates.
The
target: 35%/target: 50%values are valid; CodeCov accepts the coverage target as a string, int, or float (e.g.75%or100%). Thethreshold: 1%on both blocks is standard and correctly grants a 1% wiggle room before the status fails.The choice of a higher
patchtarget (50%) than theprojectfloor (35%) follows the recommended pattern of focusing test requirements on newly written code rather than legacy code, which is a sensible bootstrapping approach for an MVP codebase.One thing to keep in mind:
informationalis not set, so it defaults tofalse— meaning these are blocking status checks. If the current project coverage is already below 35% and a coverage report is uploaded on this PR, the check will fail immediately. Consider addinginformational: truetemporarily while the team ramps up coverage, and then removing it once the baseline is stable.💡 Optional: make checks informational until baseline is confirmed stable
project: default: target: 35% threshold: 1% + informational: true # remove once project coverage >= 35% patch: default: target: 50% threshold: 1% + informational: true # remove once patch coverage >= 50%🤖 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 @.codecov.yml around lines 11 - 18, Add the optional Codecov "informational" flag to make the coverage gates non-blocking for now; specifically, update the project.default and patch.default blocks (the existing keys with target: 35% and target: 50%) to include informational: true so the checks are informational instead of failing the status while team coverage baseline is being ramped up..pre-commit-config.yaml (1)
129-134: 💤 Low value
rbac-wildcard-checklacks the timeout guard used by every other local hook.Hooks 4 (
go-build, 30 s) and 5 (go-mod-tidy, 60 s) both wrap their entry with the portabletimeout/gtimeoutdetection pattern described in theTIMEOUT NOTEcomment (lines 88–92). Hook 6 invokesmake rbac-wildcard-checkbare, so a hungmakeinvocation can block the commit indefinitely.♻️ Proposed fix
- entry: bash -c 'make rbac-wildcard-check' + entry: bash -c 'T=$(command -v timeout || command -v gtimeout || echo); ${T:+$T 30s} make rbac-wildcard-check'🤖 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 @.pre-commit-config.yaml around lines 129 - 134, The local pre-commit hook "rbac-wildcard-check" currently runs entry: bash -c 'make rbac-wildcard-check' without a timeout guard; update its entry to wrap the make invocation using the same portable timeout detection pattern referenced in the TIMEOUT NOTE (detecting timeout/gtimeout and prefixing the command) used by the other hooks (e.g., go-build, go-mod-tidy) so that make rbac-wildcard-check is executed through the timeout wrapper to prevent commits from hanging.
🤖 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 @.pre-commit-config.yaml:
- Line 68: Update the gitleaks hook revision from the pinned "rev: v8.18.0" to
the current stable "v8.30.1" in .pre-commit-config.yaml for the gitleaks entry;
locate the gitleaks hook by the "gitleaks" repository/hook name and replace the
rev field value, then run pre-commit autoupdate or validate the config to ensure
the new version is accepted.
In `@build/Dockerfile`:
- Line 1: The Dockerfile's base image uses a non-existent tag "image-v8.3.6" in
the FROM instruction; change the tag to the latest released stable
"image-v8.3.4" (or another valid released tag) in the FROM line so the builder
can pull the base image successfully, ensuring the change is applied to the FROM
statement in the Dockerfile.
In `@controllers/certificaterequest/update_status_test.go`:
- Around line 221-224: The test's failure message incorrectly references
GetCertificate() instead of updateStatusError; inside TestUpdateStatusError (or
the test containing the call to reconciler.updateStatusError), update the
t.Errorf string to refer to updateStatusError (or a generic "Got unexpected
error") so the message matches the function under test and avoids copy-paste
confusion; locate the assertion around reconciler.updateStatusError(...) and
replace the "GetCertificate() Got unexpected error: %v" text accordingly.
---
Nitpick comments:
In @.codecov.yml:
- Around line 11-18: Add the optional Codecov "informational" flag to make the
coverage gates non-blocking for now; specifically, update the project.default
and patch.default blocks (the existing keys with target: 35% and target: 50%) to
include informational: true so the checks are informational instead of failing
the status while team coverage baseline is being ramped up.
In @.pre-commit-config.yaml:
- Around line 129-134: The local pre-commit hook "rbac-wildcard-check" currently
runs entry: bash -c 'make rbac-wildcard-check' without a timeout guard; update
its entry to wrap the make invocation using the same portable timeout detection
pattern referenced in the TIMEOUT NOTE (detecting timeout/gtimeout and prefixing
the command) used by the other hooks (e.g., go-build, go-mod-tidy) so that make
rbac-wildcard-check is executed through the timeout wrapper to prevent commits
from hanging.
🪄 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: 967d58d5-338d-41e8-8bdd-53b1a05ff10b
⛔ Files ignored due to path filters (11)
boilerplate/_data/backing-image-tagis excluded by!boilerplate/**boilerplate/_data/last-boilerplate-commitis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/.codecov.ymlis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/OWNERS_ALIASESis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/README.mdis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/ensure.shis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/golangci.ymlis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/olm_pko_migration.pyis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/pre-commit-config.yamlis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/standard.mkis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/updateis excluded by!boilerplate/**
📒 Files selected for processing (20)
.ci-operator.yaml.codecov.yml.pre-commit-config.yamlOWNERS_ALIASESapi/v1alpha1/groupversion_info.gobuild/Dockerfilebuild/Dockerfile.olm-registrycontrollers/certificaterequest/certificate.gocontrollers/certificaterequest/certificate_test.gocontrollers/certificaterequest/certificaterequest_controller.gocontrollers/certificaterequest/certificaterequest_controller_test.gocontrollers/certificaterequest/renew_certificate.gocontrollers/certificaterequest/renew_certificate_test.gocontrollers/certificaterequest/revoke_certificate.gocontrollers/certificaterequest/revoke_certificate_test.gocontrollers/certificaterequest/update_status.gocontrollers/certificaterequest/update_status_test.gocontrollers/certificaterequest/utils.gocontrollers/certificaterequest/utils_test.gotest/e2e/utils/utils.go
💤 Files with no reviewable changes (1)
- OWNERS_ALIASES
| # High-confidence findings block; configure .gitleaks.toml for allowlist. | ||
| # --------------------------------------------------------------------------- | ||
| - repo: https://github.com/gitleaks/gitleaks | ||
| rev: v8.18.0 # pinned immutable tag (golden rule 15) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable release of gitleaks?
💡 Result:
The latest stable release of Gitleaks is v8.30.1, released on 2026-03-21.[1][2][3] This is confirmed as the latest release on the official GitHub repository releases page[4], the latest release redirect[1], the specific tag page with assets and changelog[2], the main repo page listing it as latest[3][5], and third-party trackers like mise[6]. No newer releases are mentioned in searches up to today's date of 2026-05-07, and it includes standard binary assets for multiple platforms, indicating stability (not marked as prerelease).[2]
Citations:
- 1: https://github.com/zricethezav/gitleaks/releases/latest
- 2: https://github.com/gitleaks/gitleaks/releases/tag/v8.30.1
- 3: https://www.github.com/gitleaks/gitleaks
- 4: https://github.com/gitleaks/gitleaks/releases
- 5: https://github.com/gitleaks/gitleaks
- 6: https://mise-versions.jdx.dev/tools/gitleaks
Update gitleaks to the latest stable release — v8.30.1.
The pinned version v8.18.0 is significantly outdated. As a security-focused tool, gitleaks releases regularly include new secret pattern detection rules. Staying current ensures the hook catches newly added threat vectors.
Proposed fix
- rev: v8.18.0 # pinned immutable tag (golden rule 15)
+ rev: v8.30.1 # pinned immutable tag (golden rule 15)📝 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.
| rev: v8.18.0 # pinned immutable tag (golden rule 15) | |
| rev: v8.30.1 # pinned immutable tag (golden rule 15) |
🤖 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 @.pre-commit-config.yaml at line 68, Update the gitleaks hook revision from
the pinned "rev: v8.18.0" to the current stable "v8.30.1" in
.pre-commit-config.yaml for the gitleaks entry; locate the gitleaks hook by the
"gitleaks" repository/hook name and replace the rev field value, then run
pre-commit autoupdate or validate the config to ensure the new version is
accepted.
| @@ -1,4 +1,4 @@ | |||
| FROM quay.io/redhat-services-prod/openshift/boilerplate:image-v8.3.4 AS builder | |||
| FROM quay.io/redhat-services-prod/openshift/boilerplate:image-v8.3.6 AS builder | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new boilerplate image tag exists and is accessible
echo "Checking if the new boilerplate image tag is accessible..."
docker manifest inspect quay.io/redhat-services-prod/openshift/boilerplate:image-v8.3.6 >/dev/null 2>&1
if [ $? -eq 0 ]; then
echo "✓ Image tag boilerplate:image-v8.3.6 exists and is accessible"
else
echo "✗ Image tag boilerplate:image-v8.3.6 could not be verified"
fi
# Also check the old tag for comparison
echo ""
echo "Checking if the old boilerplate image tag is still accessible..."
docker manifest inspect quay.io/redhat-services-prod/openshift/boilerplate:image-v8.3.4 >/dev/null 2>&1
if [ $? -eq 0 ]; then
echo "✓ Image tag boilerplate:image-v8.3.4 exists and is accessible"
else
echo "✗ Image tag boilerplate:image-v8.3.4 could not be verified"
fiRepository: openshift/certman-operator
Length of output: 1934
🌐 Web query:
openshift boilerplate image-v8.3.6 changelog release notes
💡 Result:
No changelog or release notes found for openshift/boilerplate image-v8.3.6. The official GitHub repository https://github.com/openshift/boilerplate/ has only 8 releases, with the latest being image-v8.3.4 released on 2026-02-25 [1][2]. Searches for the specific tag image-v8.3.6 returned no matching releases or pages [3][4]. The repository's last push was 2026-03-31, but no evidence of v8.3.6 exists in available sources [1]. It may not have been released yet, or the tag exists without a published release page. Check the full tags list directly at https://github.com/openshift/boilerplate/tags for confirmation.
Citations:
- 1: https://github.com/openshift/boilerplate/
- 2: https://github.com/openshift/boilerplate
- 3: https://github.com/openshift/boilerplate/releases/tag/image-v8.3.4
- 4: https://github.com/openshift/boilerplate/tree/84124f7d4bed3bad80a9998f87552c02b0c3c4f5
🌐 Web query:
site:github.com/openshift/boilerplate "image-v8.3.6"
💡 Result:
"image-v8.3.6" refers to a potential Git tag in the OpenShift Boilerplate repository (https://github.com/openshift/boilerplate), which standardizes development conventions and builds a container image used in OpenShift CI/CD pipelines [1][2]. Tags follow the format image-vX.Y.Z for versioning the build image defined in config/Dockerfile, based on RHEL with Go, golangci-lint, kustomize, etc. [3][1]. The latest released tag as of 2026-05-07 is image-v8.3.4 (published 2026-02-25), including updates like konflux references and envtest fixes [4]. No sources mention image-v8.3.6 or higher; it may be unreleased, upcoming, or internal [1][4][2]. The repo's last push was 2026-03-31, but no new image-v* tags post-v8.3.4 [1].
Citations:
- 1: https://github.com/openshift/boilerplate
- 2: https://github.com/openshift/boilerplate/
- 3: https://github.com/openshift/boilerplate/blob/9e32f3652525c3d1ebe62ad1655820c6a7bc0c0e/README.md
- 4: https://github.com/openshift/boilerplate/releases/tag/image-v8.3.4
The boilerplate image tag image-v8.3.6 does not exist. The latest released version is image-v8.3.4 (released 2026-02-25). This will cause the build to fail when attempting to pull the base image. Either revert to the stable image-v8.3.4 or wait for image-v8.3.6 to be officially released before using it.
🤖 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 `@build/Dockerfile` at line 1, The Dockerfile's base image uses a non-existent
tag "image-v8.3.6" in the FROM instruction; change the tag to the latest
released stable "image-v8.3.4" (or another valid released tag) in the FROM line
so the builder can pull the base image successfully, ensuring the change is
applied to the FROM statement in the Dockerfile.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/localmetrics/crt_sh.go (1)
21-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential nil-pointer dereference if
sql.Openfails.If
sql.Openreturns an error,dbmay be nil and the deferreddb.Close()call will panic. The current code only logs the open error and continues. Guard the close or return early.🛡️ Proposed guard
db, err := sql.Open("postgres", getPsqlInfo()) - if err != nil { log.Error(err, "Failed to establish connection with crt.sh database") + return 0 } defer func() { if closeErr := db.Close(); closeErr != nil { log.Error(closeErr, "error closing database connection") } }()🤖 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 `@pkg/localmetrics/crt_sh.go` around lines 21 - 31, The code calls sql.Open in pkg/localmetrics/crt_sh.go and defers db.Close() even when sql.Open returned an error, risking a nil-pointer panic; modify the error handling in the block that calls sql.Open (the db, err := sql.Open(...) section) to return early (or otherwise abort the function) when err != nil instead of just logging, or only install the defer db.Close() after confirming db != nil (i.e., move the defer below the nil-check), so that Close is never called on a nil db.pkg/localmetrics/localmetrics.go (1)
118-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't mark the counter initialized after a failed list.
Now that this uses reconcile-scoped
ctx, cancellation/timeouts can hit the initialListcall. On that path the function still sets the metric and flipsareCountInitialized = true, so a transient failure can permanently pin the gauge to0until restart.Suggested fix
func CheckInitCounter(ctx context.Context, c client.Client) { if !areCountInitialized { counter := 0.0 var certRequestList certmanv1alpha1.CertificateRequestList if err := c.List(ctx, &certRequestList, &client.ListOptions{}); err != nil { logger.Error(err, "Failed to Init counter for Certificate Request") + return } for _, cr := range certRequestList.Items { if utils.ContainsString(cr.Finalizers, certmanv1alpha1.CertmanOperatorFinalizerLabel) { counter++ } } MetricCertRequestsCount.Set(counter) areCountInitialized = true } }🤖 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 `@pkg/localmetrics/localmetrics.go` around lines 118 - 135, In CheckInitCounter, do not mark areCountInitialized true or set MetricCertRequestsCount when the initial c.List(ctx, &certRequestList, ...) returns an error; instead return early after logging the error so transient list failures (due to reconcile-scoped ctx cancellation/timeouts) do not permanently seal the gauge at 0. Locate the c.List call and its error handling in CheckInitCounter, ensure you exit the function on error (leaving areCountInitialized false) and only proceed to iterate certRequestList.Items, call MetricCertRequestsCount.Set(counter), and set areCountInitialized = true when the list succeeds.controllers/certificaterequest/issue_certificate_test.go (2)
282-299:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActually enable the FedRAMP branch in the test.
The FedRAMP case never sets the package-level
fedramp/fedrampHostedZoneIDflags thatFindZoneIDForChallengereads, so this path still exercises the non-FedRAMP logic. The final assertion only compares the fixture value against itself.Suggested fix
t.Run(tc.name, func(t *testing.T) { + origFedramp, origHostedZoneID := fedramp, fedrampHostedZoneID + fedramp = tc.fedramp + fedrampHostedZoneID = tc.fedrampHostedZoneID + t.Cleanup(func() { + fedramp = origFedramp + fedrampHostedZoneID = origHostedZoneID + }) testClient := setUpTestClient(t, tc.KubeObjects) @@ zoneID, err := reconciler.FindZoneIDForChallenge(context.TODO(), "test", mockClient) @@ - if tc.expectedFedrampHostedZoneID != mockClient.FedrampHostedZoneID && tc.fedramp == true { - t.Fatalf("unexpected zone id - Expected Fedramp Zone Id: %v - Got - %v", tc.expectedFedrampHostedZoneID, fedrampHostedZoneID) + if tc.fedramp && zoneID != tc.expectedFedrampHostedZoneID { + t.Fatalf("unexpected zone id - Expected Fedramp Zone Id: %v - Got - %v", tc.expectedFedrampHostedZoneID, zoneID) } }) }🤖 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 `@controllers/certificaterequest/issue_certificate_test.go` around lines 282 - 299, Test never enables the FedRAMP branch because the package-level flags fedramp and fedrampHostedZoneID (which FindZoneIDForChallenge reads) are not set in the test; set fedramp = true and fedrampHostedZoneID = mockClient.FedrampHostedZoneID (or the expected test value) before calling reconciler.FindZoneIDForChallenge in the FedRAMP case, run the assertion comparing zoneID to the expectedFedrampHostedZoneID, and then restore the original package-level values (or reset fedramp = false) after the subtest to avoid impacting other tests; refer to FindZoneIDForChallenge, fedramp, fedrampHostedZoneID, and mockClient.FedrampHostedZoneID to locate the relevant code to change.
112-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
testErrin the assertions.The updated call stores the function result in
testErr, but the assertions still inspect the oldererrvariable from the setup path. That means unexpectedIssueCertificatefailures are missed, and the error-path assertion can dereferencenil.Suggested fix
testErr := rcr.IssueCertificate(context.TODO(), nullLogger, cr, s, test.LEClient) - if err != nil && !test.ExpectError { - t.Errorf("got unexpected error: %s", err) + if testErr != nil && !test.ExpectError { + t.Errorf("got unexpected error: %s", testErr) } @@ if test.ExpectError { + if testErr == nil { + t.Fatalf("expected error containing %q, got nil", test.ExpectedErrorMessage) + } if !strings.Contains(testErr.Error(), test.ExpectedErrorMessage) { - t.Errorf("error (%s) did not contain expected message (%s)", err.Error(), test.ExpectedErrorMessage) + t.Errorf("error (%s) did not contain expected message (%s)", testErr.Error(), test.ExpectedErrorMessage) } }🤖 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 `@controllers/certificaterequest/issue_certificate_test.go` around lines 112 - 131, The test calls IssueCertificate and assigns its result to testErr but later assertions still reference err; change those to use testErr: replace the unexpected-error check "if err != nil && !test.ExpectError" with "if testErr != nil && !test.ExpectError", and in the error-path assertion use testErr.Error() (or check testErr non-nil before calling Error()) instead of err.Error(); keep the local "err" used for metric Write as-is but ensure you don't mix it with testErr.
🧹 Nitpick comments (5)
pkg/clients/aws/route53.go (1)
248-249: ⚡ Quick winAdd a short rationale next to
//nolint:gocyclosuppressions.Please append a brief reason (and ideally a follow-up issue ID) to each suppression so future cleanup is actionable.
Also applies to: 354-355
🤖 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 `@pkg/clients/aws/route53.go` around lines 248 - 249, Add a short rationale comment next to the existing //nolint:gocyclo on the DeleteAcmeChallengeResourceRecords function (and the other occurrence around lines 354-355) explaining why cyclomatic complexity is being suppressed and include an action item or issue ID for future refactor; locate the //nolint:gocyclo on the awsClient methods (e.g., DeleteAcmeChallengeResourceRecords) and append a brief note like “//nolint:gocyclo: complex AWS Route53 logic; refactor tracked in ISSUE-1234” so maintainers know the reason and have a follow-up to address later.controllers/certificaterequest/update_status.go (1)
72-105: 💤 Low valueSentinel-via-empty-
Statusis fragile; consider returning a bool or*Condition.
acmeErrornow returns the zero-value condition when one already exists, and callers detect this withnewCondition.Status != "". That couples the "should append" decision to a default-valued field, which is easy to break later (e.g., if a caller forgets the empty check, or ifStatusever becomes legitimately empty for some other path). A(condition, ok bool)or*CertificateRequestConditionreturn makes intent explicit.♻️ Proposed change
-func acmeError(reqLogger logr.Logger, cr *certmanv1alpha1.CertificateRequest, err error) certmanv1alpha1.CertificateRequestCondition { +func acmeError(reqLogger logr.Logger, cr *certmanv1alpha1.CertificateRequest, err error) (certmanv1alpha1.CertificateRequestCondition, bool) { var found bool var newCondition certmanv1alpha1.CertificateRequestCondition for index := range cr.Status.Conditions { if cr.Status.Conditions[index].Type == "acme error" { found = true } } - if !found { - m := fmt.Sprint(err) - newCondition.Type = certmanv1alpha1.CertificateRequestConditionType("acme error") - newCondition.Status = corev1.ConditionStatus("Error") - newCondition.Message = &m - reqLogger.Info("Added condition 'acme error'") - } - return newCondition + if found { + return newCondition, false + } + m := fmt.Sprint(err) + newCondition.Type = certmanv1alpha1.CertificateRequestConditionType("acme error") + newCondition.Status = corev1.ConditionStatus("Error") + newCondition.Message = &m + reqLogger.Info("Added condition 'acme error'") + return newCondition, true }Update the caller:
- newCondition := acmeError(reqLogger, cr, err) - if newCondition.Status != "" { - cr.Status.Conditions = append(cr.Status.Conditions, newCondition) - } + if newCondition, ok := acmeError(reqLogger, cr, err); ok { + cr.Status.Conditions = append(cr.Status.Conditions, newCondition) + }🤖 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 `@controllers/certificaterequest/update_status.go` around lines 72 - 105, The acmeError function currently returns a zero-value CertificateRequestCondition to signal "no new condition", which is fragile; change acmeError to return a pointer and a bool (or just a pointer) — e.g., func acmeError(reqLogger logr.Logger, cr *certmanv1alpha1.CertificateRequest, err error) (*certmanv1alpha1.CertificateRequestCondition, bool) — and return (nil, false) when an existing "acme error" condition is found and ( &newCondition, true) when constructing a new one; then update the caller in updateStatusError to use the new signature and append only when ok is true (or when the returned pointer is non-nil) instead of checking newCondition.Status != "" so the intent is explicit and not tied to zero-valued fields.controllers/certificaterequest/cloudflare.go (1)
61-92: ⚡ Quick winMake the per-attempt sleep ctx-aware.
time.Sleep(...)ignoresctx; if the reconcile context is cancelled (e.g., shutdown, deadline) the loop will keep sleeping for up tomaxNegativeCacheTTLseconds per attempt before noticing. Now thatctxis threaded through, the sleep should respect cancellation.♻️ Proposed change
reqLogger.Info(fmt.Sprintf("will query DNS in %v seconds", sleepDuration)) - time.Sleep(time.Duration(sleepDuration) * time.Second) + select { + case <-time.After(time.Duration(sleepDuration) * time.Second): + case <-ctx.Done(): + return false + }🤖 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 `@controllers/certificaterequest/cloudflare.go` around lines 61 - 92, The loop in cloudflare.go currently uses time.Sleep(sleepDuration) which ignores ctx; replace the direct sleep with a ctx-aware wait (use a timer or time.After combined with select on ctx.Done()) so the reconcile exits immediately if ctx is cancelled. Specifically, in the for loop around TryFetchResourceRecordUsingPublicDNS, swap the time.Sleep(time.Duration(sleepDuration)*time.Second) with a select that either waits the duration or returns/propagates when <-ctx.Done() fires (log the cancellation via reqLogger and return false or ctx.Err() as appropriate) while keeping the existing variables sleepDuration, attempt, fqdn and txtValue intact.controllers/utils/utils.go (1)
50-86: ⚡ Quick winThread
ctxthroughGetCredentialsJSONandgetSecretfor consistency.The PR established a pattern of threading
ctxthroughGetDefaultNotificationEmailAddressandgetConfig, butGetCredentialsJSONand its helpergetSecretstill use hardcodedcontext.Background()andcontext.TODO(). This prevents cancellation and timeouts from flowing into the credentials fetch path.Update both function signatures to accept
ctx, propagate it to the Kubernetes client calls andgoogle.CredentialsFromJSON, and update call sites (pkg/clients/gcp/dns.go:170and tests) accordingly.🤖 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 `@controllers/utils/utils.go` around lines 50 - 86, Update GetCredentialsJSON and getSecret to accept a context.Context parameter and use that ctx instead of context.Background()/context.TODO(); specifically, change GetCredentialsJSON(ctx context.Context, kubeClient client.Client, namespacesedName types.NamespacedName) to pass ctx into getSecret and into google.CredentialsFromJSON(ctx, sa, ...), and change getSecret(ctx context.Context, kubeClient client.Client, namespacesedName types.NamespacedName) to call kubeClient.Get(ctx, namespacesedName, secret) (instead of context.TODO()); then update all call sites (notably pkg/clients/gcp/dns.go:170 and related tests) to supply the context argument so cancellation/timeouts propagate.pkg/localmetrics/crt_sh.go (1)
19-48: ⚡ Quick winConsider accepting
context.Contextas a parameter instead of usingcontext.Background().The function makes a network call to
crt.shthat could block indefinitely if the service is slow or unresponsive. Usingcontext.Background()means there is no way to cancel this query (e.g., on shutdown).However, note that
GetCountOfCertsIssuedis called only fromUpdateCertsIssuedInLastDayGauge()andUpdateCertsIssuedInLastWeekGauge(), which are invoked fromUpdateMetrics()as a background ticker loop inmain.go. Implementing this change would require threading context through the entire chain:main()→UpdateMetrics()→UpdateCertsIssuedInLastDayGauge/Week()→GetCountOfCertsIssued(), which requires broader architectural changes since the current ticker-based loop has no natural context lifecycle.🤖 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 `@pkg/localmetrics/crt_sh.go` around lines 19 - 48, Change GetCountOfCertsIssued to accept a context.Context parameter (e.g., func GetCountOfCertsIssued(ctx context.Context, domain string, durationDays int) int) and use that ctx in db.QueryRowContext instead of context.Background(); update callers UpdateCertsIssuedInLastDayGauge and UpdateCertsIssuedInLastWeekGauge to pass a context through, then propagate that context from UpdateMetrics and from main’s ticker loop so the background ticker can be cancelled on shutdown; ensure the db opening/closing and error paths remain the same and that you pass the same ctx into any other DB calls used in this flow.
🤖 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 `@main.go`:
- Around line 143-146: The leader-election branch currently treats
k8sutil.ErrRunLocal and k8sutil.ErrNoNamespace the same; change the conditional
so that only errors.Is(err, k8sutil.ErrRunLocal) results in skipping leader
election (keep setupLog.Info("Skipping leader election; not running in a
cluster.") there), and handle errors.Is(err, k8sutil.ErrNoNamespace) as a
configuration/runtime error instead (e.g., log an error via setupLog.Error and
exit/return so the operator fails fast). Update the surrounding leader-election
logic where errors.Is(err, k8sutil.ErrRunLocal) and k8sutil.ErrNoNamespace are
checked to reflect this separation.
---
Outside diff comments:
In `@controllers/certificaterequest/issue_certificate_test.go`:
- Around line 282-299: Test never enables the FedRAMP branch because the
package-level flags fedramp and fedrampHostedZoneID (which
FindZoneIDForChallenge reads) are not set in the test; set fedramp = true and
fedrampHostedZoneID = mockClient.FedrampHostedZoneID (or the expected test
value) before calling reconciler.FindZoneIDForChallenge in the FedRAMP case, run
the assertion comparing zoneID to the expectedFedrampHostedZoneID, and then
restore the original package-level values (or reset fedramp = false) after the
subtest to avoid impacting other tests; refer to FindZoneIDForChallenge,
fedramp, fedrampHostedZoneID, and mockClient.FedrampHostedZoneID to locate the
relevant code to change.
- Around line 112-131: The test calls IssueCertificate and assigns its result to
testErr but later assertions still reference err; change those to use testErr:
replace the unexpected-error check "if err != nil && !test.ExpectError" with "if
testErr != nil && !test.ExpectError", and in the error-path assertion use
testErr.Error() (or check testErr non-nil before calling Error()) instead of
err.Error(); keep the local "err" used for metric Write as-is but ensure you
don't mix it with testErr.
In `@pkg/localmetrics/crt_sh.go`:
- Around line 21-31: The code calls sql.Open in pkg/localmetrics/crt_sh.go and
defers db.Close() even when sql.Open returned an error, risking a nil-pointer
panic; modify the error handling in the block that calls sql.Open (the db, err
:= sql.Open(...) section) to return early (or otherwise abort the function) when
err != nil instead of just logging, or only install the defer db.Close() after
confirming db != nil (i.e., move the defer below the nil-check), so that Close
is never called on a nil db.
In `@pkg/localmetrics/localmetrics.go`:
- Around line 118-135: In CheckInitCounter, do not mark areCountInitialized true
or set MetricCertRequestsCount when the initial c.List(ctx, &certRequestList,
...) returns an error; instead return early after logging the error so transient
list failures (due to reconcile-scoped ctx cancellation/timeouts) do not
permanently seal the gauge at 0. Locate the c.List call and its error handling
in CheckInitCounter, ensure you exit the function on error (leaving
areCountInitialized false) and only proceed to iterate certRequestList.Items,
call MetricCertRequestsCount.Set(counter), and set areCountInitialized = true
when the list succeeds.
---
Nitpick comments:
In `@controllers/certificaterequest/cloudflare.go`:
- Around line 61-92: The loop in cloudflare.go currently uses
time.Sleep(sleepDuration) which ignores ctx; replace the direct sleep with a
ctx-aware wait (use a timer or time.After combined with select on ctx.Done()) so
the reconcile exits immediately if ctx is cancelled. Specifically, in the for
loop around TryFetchResourceRecordUsingPublicDNS, swap the
time.Sleep(time.Duration(sleepDuration)*time.Second) with a select that either
waits the duration or returns/propagates when <-ctx.Done() fires (log the
cancellation via reqLogger and return false or ctx.Err() as appropriate) while
keeping the existing variables sleepDuration, attempt, fqdn and txtValue intact.
In `@controllers/certificaterequest/update_status.go`:
- Around line 72-105: The acmeError function currently returns a zero-value
CertificateRequestCondition to signal "no new condition", which is fragile;
change acmeError to return a pointer and a bool (or just a pointer) — e.g., func
acmeError(reqLogger logr.Logger, cr *certmanv1alpha1.CertificateRequest, err
error) (*certmanv1alpha1.CertificateRequestCondition, bool) — and return (nil,
false) when an existing "acme error" condition is found and ( &newCondition,
true) when constructing a new one; then update the caller in updateStatusError
to use the new signature and append only when ok is true (or when the returned
pointer is non-nil) instead of checking newCondition.Status != "" so the intent
is explicit and not tied to zero-valued fields.
In `@controllers/utils/utils.go`:
- Around line 50-86: Update GetCredentialsJSON and getSecret to accept a
context.Context parameter and use that ctx instead of
context.Background()/context.TODO(); specifically, change GetCredentialsJSON(ctx
context.Context, kubeClient client.Client, namespacesedName
types.NamespacedName) to pass ctx into getSecret and into
google.CredentialsFromJSON(ctx, sa, ...), and change getSecret(ctx
context.Context, kubeClient client.Client, namespacesedName
types.NamespacedName) to call kubeClient.Get(ctx, namespacesedName, secret)
(instead of context.TODO()); then update all call sites (notably
pkg/clients/gcp/dns.go:170 and related tests) to supply the context argument so
cancellation/timeouts propagate.
In `@pkg/clients/aws/route53.go`:
- Around line 248-249: Add a short rationale comment next to the existing
//nolint:gocyclo on the DeleteAcmeChallengeResourceRecords function (and the
other occurrence around lines 354-355) explaining why cyclomatic complexity is
being suppressed and include an action item or issue ID for future refactor;
locate the //nolint:gocyclo on the awsClient methods (e.g.,
DeleteAcmeChallengeResourceRecords) and append a brief note like
“//nolint:gocyclo: complex AWS Route53 logic; refactor tracked in ISSUE-1234” so
maintainers know the reason and have a follow-up to address later.
In `@pkg/localmetrics/crt_sh.go`:
- Around line 19-48: Change GetCountOfCertsIssued to accept a context.Context
parameter (e.g., func GetCountOfCertsIssued(ctx context.Context, domain string,
durationDays int) int) and use that ctx in db.QueryRowContext instead of
context.Background(); update callers UpdateCertsIssuedInLastDayGauge and
UpdateCertsIssuedInLastWeekGauge to pass a context through, then propagate that
context from UpdateMetrics and from main’s ticker loop so the background ticker
can be cancelled on shutdown; ensure the db opening/closing and error paths
remain the same and that you pass the same ctx into any other DB calls used in
this flow.
🪄 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: b36b7144-6e70-4a40-a8ef-15fd55095325
📒 Files selected for processing (24)
controllers/certificaterequest/certificaterequest_controller.gocontrollers/certificaterequest/cloudflare.gocontrollers/certificaterequest/issue_certificate.gocontrollers/certificaterequest/issue_certificate_test.gocontrollers/certificaterequest/revoke_certificate.gocontrollers/certificaterequest/update_status.gocontrollers/certificaterequest/update_status_test.gocontrollers/clusterdeployment/clusterdeployment_controller.gocontrollers/clusterdeployment/clusterdeployment_controller_test.gocontrollers/clusterdeployment/clusterdeployment_delete.gocontrollers/utils/utils.gocontrollers/utils/utils_test.gofips.gomain.gopkg/clients/aws/route53.gopkg/clients/azure/dns_test.gopkg/clients/gcp/dns.gopkg/leclient/lets_encrypt.gopkg/leclient/lets_encrypt_test.gopkg/leclient/utils.gopkg/localmetrics/crt_sh.gopkg/localmetrics/localmetrics.gopkg/localmetrics/localmetrics_test.gotest/e2e/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- pkg/clients/azure/dns_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- controllers/certificaterequest/update_status_test.go
- controllers/certificaterequest/revoke_certificate.go
- test/e2e/utils/utils.go
|
/test validate |
|
@devppratik: The following test 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. |
2 participants
What type of PR is this?
boilerplate update
What this PR does / why we need it?
This PR moves the changes introduced in boilerplate for Agentic SDLC Rollout into MVP for ocm-agent-operator.
Related BP MRs
Which Jira/Github issue(s) this PR fixes?
Part of Rollout for Agentic SDLC -
Summary by CodeRabbit
Chores
Refactor