CM-786: Updates controller override arg list to allow certificate-request-minimum-backoff-duration and fmt changes#407
Conversation
Signed-off-by: Bharath B <bhb@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references CM-786 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 task to target the "4.22.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. |
WalkthroughRefactored cert-manager test setup into shared helpers; added support for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
|
@bharath-b-rh: This pull request references CM-786 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 task to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/certmanager/deployment_helper_test.go (1)
56-58: Handle tombstones inDeleteFuncto avoid panic-prone casts.Delete events may arrive as
cache.DeletedFinalStateUnknown; direct type assertion can panic in that case.Suggested robust delete handler
DeleteFunc: func(obj any) { - ch <- obj.(*v1alpha1.CertManager) + switch o := obj.(type) { + case *v1alpha1.CertManager: + ch <- o + case cache.DeletedFinalStateUnknown: + if cm, ok := o.Obj.(*v1alpha1.CertManager); ok { + ch <- cm + } + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/deployment_helper_test.go` around lines 56 - 58, The DeleteFunc currently does a direct cast to *v1alpha1.CertManager which can panic when the informer sends a cache.DeletedFinalStateUnknown tombstone; update DeleteFunc to first check whether obj is a cache.DeletedFinalStateUnknown (or has an Object field) and extract the actual object before the type assertion, falling back to a safe type assertion for *v1alpha1.CertManager and only sending to ch when the cast succeeds; reference the DeleteFunc handler, the ch channel, the v1alpha1.CertManager type and cache.DeletedFinalStateUnknown in your change.pkg/controller/certmanager/deployment_overrides_validation_test.go (1)
489-515: Make cleanup resilient to early assertion failures in subtests.If a subtest fails before the explicit delete block,
"cluster"can remain and cascade failures into subsequent cases. Registering cleanup right after create keeps subtests isolated.Suggested cleanup pattern
_, err := fakeClient.OperatorV1alpha1().CertManagers().Create(ctx, &tc.certManagerObj, metav1.CreateOptions{}) require.NoError(t, err) +t.Cleanup(func() { + err := fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + t.Errorf("cleanup delete failed: %v", err) + return + } + select { + case <-certManagerChan: + case <-time.After(wait.ForeverTestTimeout): + t.Errorf("Informer did not get the deleted cert manager object during cleanup") + } +}) @@ -err = fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name, metav1.DeleteOptions{}) -require.NoError(t, err) - -select { -case <-certManagerChan: -case <-time.After(wait.ForeverTestTimeout): - t.Fatal("Informer did not get the deleted cert manager object") -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/deployment_overrides_validation_test.go` around lines 489 - 515, After creating the test CertManager object, register a t.Cleanup that deletes the created object and waits for the informer to observe the deletion instead of performing the explicit delete at the end of the subtest; specifically, immediately after calling fakeClient.OperatorV1alpha1().CertManagers().Create(ctx, &tc.certManagerObj, ...), call t.Cleanup to run the delete via fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name, ...) and then wait on certManagerChan (using the same wait.ForeverTestTimeout) so the informer sees the removal; this keeps the resource cleanup resilient to early assertion failures and preserves the existing logic that waits on certManagerChan, with no other changes to withContainerArgsValidateHook, certManagerInformers, or tc.deploymentName usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/certmanager/deployment_helper_test.go`:
- Around line 62-64: The informer bootstrap currently can block forever because
the result of cache.WaitForCacheSync is ignored and the receive on
watcherStarted has no timeout; update the test to use a bounded context (e.g.
ctxWithTimeout := context.WithTimeout(ctx, someReasonableDuration)) and then
pass ctxWithTimeout.Done() to informer.Informer().Run and
cache.WaitForCacheSync, check the bool result returned by cache.WaitForCacheSync
(fail the test if it returns false), and replace the plain <-watcherStarted with
a select that waits for either watcherStarted or the context timeout so the test
fails fast instead of hanging; refer to informer.Informer().Run,
cache.WaitForCacheSync, and watcherStarted when making these changes.
---
Nitpick comments:
In `@pkg/controller/certmanager/deployment_helper_test.go`:
- Around line 56-58: The DeleteFunc currently does a direct cast to
*v1alpha1.CertManager which can panic when the informer sends a
cache.DeletedFinalStateUnknown tombstone; update DeleteFunc to first check
whether obj is a cache.DeletedFinalStateUnknown (or has an Object field) and
extract the actual object before the type assertion, falling back to a safe type
assertion for *v1alpha1.CertManager and only sending to ch when the cast
succeeds; reference the DeleteFunc handler, the ch channel, the
v1alpha1.CertManager type and cache.DeletedFinalStateUnknown in your change.
In `@pkg/controller/certmanager/deployment_overrides_validation_test.go`:
- Around line 489-515: After creating the test CertManager object, register a
t.Cleanup that deletes the created object and waits for the informer to observe
the deletion instead of performing the explicit delete at the end of the
subtest; specifically, immediately after calling
fakeClient.OperatorV1alpha1().CertManagers().Create(ctx, &tc.certManagerObj,
...), call t.Cleanup to run the delete via
fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, tc.certManagerObj.Name,
...) and then wait on certManagerChan (using the same wait.ForeverTestTimeout)
so the informer sees the removal; this keeps the resource cleanup resilient to
early assertion failures and preserves the existing logic that waits on
certManagerChan, with no other changes to withContainerArgsValidateHook,
certManagerInformers, or tc.deploymentName usage.
🪄 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: Pro Plus
Run ID: 0cbf340d-0ca0-4ba2-b76c-a22e2c1d0331
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
pkg/controller/certmanager/deployment_helper_test.gopkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/certmanager/deployment_overrides_validation_test.gopkg/controller/common/client_test.gopkg/controller/istiocsr/utils_test.gopkg/controller/trustmanager/deployments_test.go
…uest-minimum-backoff-duration Signed-off-by: Bharath B <bhb@redhat.com>
|
@bharath-b-rh: This pull request references CM-786 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 task to target the "4.22.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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/controller/certmanager/deployment_helper_test.go (1)
70-75:⚠️ Potential issue | 🟠 Major
WaitForCacheSynccan still hang indefinitely without a bounded context.Line 71 still waits on
ctx.Done()(fromt.Context()), so if sync never completes this path can block forever before the timeoutselectis reached.🔧 Proposed hardening
+ ctxWithTimeout, cancel := context.WithTimeout(ctx, wait.ForeverTestTimeout) + t.Cleanup(cancel) - go informer.Informer().Run(ctx.Done()) - require.True(t, cache.WaitForCacheSync(ctx.Done(), informer.Informer().HasSynced), "failed to sync CertManager informer cache") + go informer.Informer().Run(ctxWithTimeout.Done()) + require.True(t, cache.WaitForCacheSync(ctxWithTimeout.Done(), informer.Informer().HasSynced), "failed to sync CertManager informer cache") select { case <-watcherStarted: - case <-time.After(wait.ForeverTestTimeout): + case <-ctxWithTimeout.Done(): t.Fatal("watch reactor did not start") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/deployment_helper_test.go` around lines 70 - 75, The test can hang because cache.WaitForCacheSync is still waiting on the unbounded t.Context() (ctx.Done()); change the test to use a bounded context when calling cache.WaitForCacheSync: create a context with timeout via context.WithTimeout (e.g., syncCtx, cancel := context.WithTimeout(ctx, wait.ForeverTestTimeout)) and pass syncCtx.Done() to cache.WaitForCacheSync, defer cancel(), so the sync call will return within the test timeout and the subsequent select on watcherStarted will behave as expected; keep the informer.Informer().Run(ctx.Done()) call as-is.
🧹 Nitpick comments (1)
pkg/controller/certmanager/deployment_helper_test.go (1)
93-103: Assert received informer events match the expected object.The helper currently accepts any event from a shared channel; validating
obj.Namemakes failures less flaky and easier to diagnose.🧪 Proposed tightening
t.Cleanup(func() { err := fakeClient.OperatorV1alpha1().CertManagers().Delete(ctx, obj.Name, metav1.DeleteOptions{}) if err != nil && !apierrors.IsNotFound(err) { t.Errorf("cert-manager delete failed: %v", err) return } select { - case <-events: + case deleted := <-events: + require.Equal(t, obj.Name, deleted.Name, "unexpected delete event object") case <-time.After(wait.ForeverTestTimeout): t.Errorf("Informer did not get the deleted cert manager object during cleanup") } }) select { - case <-events: + case added := <-events: + require.Equal(t, obj.Name, added.Name, "unexpected add event object") case <-time.After(wait.ForeverTestTimeout): t.Fatal("Informer did not get the added cert manager object") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/deployment_helper_test.go` around lines 93 - 103, Replace the loose reads from the shared events channel with assertions that the received event contains the expected object's Name: when you read from events in deployment_helper_test.go (both the deletion and addition checks), receive the event value, cast or convert it to the expected Kubernetes object type or metav1.Object, and assert its Name equals the expected test object's Name (failing the test with a clear message if it does not); keep the existing timeout handling but change t.Errorf/t.Fatal messages to include the actual received name for easier diagnosis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controller/certmanager/deployment_helper_test.go`:
- Around line 70-75: The test can hang because cache.WaitForCacheSync is still
waiting on the unbounded t.Context() (ctx.Done()); change the test to use a
bounded context when calling cache.WaitForCacheSync: create a context with
timeout via context.WithTimeout (e.g., syncCtx, cancel :=
context.WithTimeout(ctx, wait.ForeverTestTimeout)) and pass syncCtx.Done() to
cache.WaitForCacheSync, defer cancel(), so the sync call will return within the
test timeout and the subsequent select on watcherStarted will behave as
expected; keep the informer.Informer().Run(ctx.Done()) call as-is.
---
Nitpick comments:
In `@pkg/controller/certmanager/deployment_helper_test.go`:
- Around line 93-103: Replace the loose reads from the shared events channel
with assertions that the received event contains the expected object's Name:
when you read from events in deployment_helper_test.go (both the deletion and
addition checks), receive the event value, cast or convert it to the expected
Kubernetes object type or metav1.Object, and assert its Name equals the expected
test object's Name (failing the test with a clear message if it does not); keep
the existing timeout handling but change t.Errorf/t.Fatal messages to include
the actual received name for easier diagnosis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6303ce5b-6c6e-4f97-bf9b-17396dccaeeb
📒 Files selected for processing (3)
pkg/controller/certmanager/deployment_helper_test.gopkg/controller/certmanager/deployment_overrides_validation.gopkg/controller/certmanager/deployment_overrides_validation_test.go
chiragkyal
left a comment
chiragkyal
left a comment
There was a problem hiding this comment.
Only a few nits which caught my eye, nothing major.
Also, won't it be better to have some e2e cases as well?
/lgtm
| AddFunc: func(obj any) { | ||
| ch <- obj.(*v1alpha1.CertManager) |
There was a problem hiding this comment.
We missed a log line here, which was present earlier.
t.Logf("cert manager obj added: %s", certManagerObj.Name)
There was a problem hiding this comment.
yeah removed the info level logs since these unit tests, and retained only error related logs.
| AddFunc: func(obj any) { | ||
| ch <- obj.(*v1alpha1.CertManager) | ||
| }, | ||
| DeleteFunc: func(obj any) { |
| wantErrMsg: `validation failed due to unsupported arg "--metrics-listen-address"="0.0.0.0:9402"`, | ||
| }, | ||
| { | ||
| name: "webhook rejects certificate-request-minimum-backoff-duration", |
There was a problem hiding this comment.
If we use certificate-request-minimum-backoff-duration flag in the above test, we can cover both cases in a single test right?
There was a problem hiding this comment.
yeah, I had added tests only for certificate-request-minimum-backoff-duration initially and later updated it for other args, an additional scenario has got added.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, chiragkyal 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 |
e2e test scenarios is already present for other args, so didn't add a new one for another item in the list. |
|
@snarayan-redhat Please help with updating the release notes for the new arg support under new features. Thank you! |
I feel it would be better to have all the args covered into the list. |
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
@bharath-b-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. |
|
Please refer doc for the tests. |
openshift-ci
bot
added
the
qe-approved
|
@bharath-b-rh: This pull request references CM-786 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 task to target the "4.22.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. |
|
/cherrypick cert-manager-1.19 |
|
@bharath-b-rh: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/label docs-approved |
openshift-ci
bot
added
the
docs-approved
|
@bharath-b-rh: new pull request created: #408 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 kubernetes-sigs/prow repository. |
5 participants
The PR has following changes:
make fmtsuggested changesmake update-vendorsuggeste changesSummary by CodeRabbit
New Features
Tests
Style / Refactor