context consistency: ensure all context are TODO() #242
Conversation
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @sebrandon1. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
openshift-merge-robot
added
the
needs-rebase
sebrandon1
force-pushed
the
ctx_consistency
branch
from
969e017 to
eee7773
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/ok-to-test |
sebrandon1
force-pushed
the
ctx_consistency
branch
from
eee7773 to
f4dfb9c
Compare
sebrandon1
force-pushed
the
ctx_consistency
branch
from
f4dfb9c to
31307d1
Compare
|
/retest |
1 similar comment
|
/retest |
sebrandon1
force-pushed
the
ctx_consistency
branch
from
31307d1 to
ff341f3
Compare
|
/test all |
|
Hi @sebrandon1, you may need to rebase onto the latesr commit. There have been some changes in |
sebrandon1
force-pushed
the
ctx_consistency
branch
2 times, most recently
from
ef1d2e1 to
4ed1587
Compare
There was a problem hiding this comment.
This file can be safely removed. Then the broken CI jobs should pass.
(for your context: This file has been removed in a recent refactor PR)
sebrandon1
force-pushed
the
ctx_consistency
branch
from
4ed1587 to
8d0dc18
Compare
WalkthroughReplaces many uses of context.Background() with context.TODO() across controllers, tests, and utilities; updates an example comment; and adds a new end-to-end test file introducing multiple cert-related E2E tests and helpers. No exported/public signatures altered. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/e2e/cert_manager_deployment_test.go (1)
1-454: Please remove this file (was deleted upstream and breaks CI).Prior review already flagged this file as removable due to a recent refactor; re‑adding it will fail CI. Either drop it from this PR or rebase on latest master where it’s gone.
Run to confirm the file was removed in master:
#!/bin/bash git fetch origin master git diff --name-status origin/master -- test/e2e/cert_manager_deployment_test.go
🧹 Nitpick comments (9)
test/e2e/certificates_test.go (1)
214-221: Use the test-scoped ctx instead of context.TODO() for polling.Propagate the same ctx you initialized in BeforeAll so cancellations/timeouts are honored consistently.
Apply this diff in the four sites:
- err := wait.PollUntilContextTimeout(context.TODO(), slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { - err := wait.PollUntilContextTimeout(context.TODO(), slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { - err := wait.PollUntilContextTimeout(context.TODO(), slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { - err := wait.PollUntilContextTimeout(context.TODO(), slowPollInterval, highTimeout, true, func(context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, slowPollInterval, highTimeout, true, func(context.Context) (bool, error) {Also applies to: 311-318, 517-524, 796-813
pkg/operator/informers/externalversions/factory.go (1)
191-191: Fix incorrect example: TODO() doesn’t return a CancelFunc.The sample should use WithCancel; current snippet won’t compile if copied.
Apply this diff in the comment:
-// ctx, cancel := context.TODO() +// ctx, cancel := context.WithCancel(context.TODO())pkg/controller/istiocsr/controller.go (1)
114-151: DRY the repeated context.TODO() calls in GetInformer.Minor readability nit: define ctx once and reuse.
Apply this diff:
func BuildCustomClient(mgr ctrl.Manager) (client.Client, error) { @@ customCache, err := cache.New(mgr.GetConfig(), customCacheOpts) if err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &v1alpha1.IstioCSR{}); err != nil { + ctx := context.TODO() + if _, err = customCache.GetInformer(ctx, &v1alpha1.IstioCSR{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &certmanagerv1.Certificate{}); err != nil { + if _, err = customCache.GetInformer(ctx, &certmanagerv1.Certificate{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &appsv1.Deployment{}); err != nil { + if _, err = customCache.GetInformer(ctx, &appsv1.Deployment{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &rbacv1.ClusterRole{}); err != nil { + if _, err = customCache.GetInformer(ctx, &rbacv1.ClusterRole{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &rbacv1.ClusterRoleBinding{}); err != nil { + if _, err = customCache.GetInformer(ctx, &rbacv1.ClusterRoleBinding{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &rbacv1.Role{}); err != nil { + if _, err = customCache.GetInformer(ctx, &rbacv1.Role{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &rbacv1.RoleBinding{}); err != nil { + if _, err = customCache.GetInformer(ctx, &rbacv1.RoleBinding{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &corev1.Service{}); err != nil { + if _, err = customCache.GetInformer(ctx, &corev1.Service{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &corev1.ServiceAccount{}); err != nil { + if _, err = customCache.GetInformer(ctx, &corev1.ServiceAccount{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &corev1.Secret{}); err != nil { + if _, err = customCache.GetInformer(ctx, &corev1.Secret{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &corev1.ConfigMap{}); err != nil { + if _, err = customCache.GetInformer(ctx, &corev1.ConfigMap{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &certmanagerv1.Issuer{}); err != nil { + if _, err = customCache.GetInformer(ctx, &certmanagerv1.Issuer{}); err != nil { return nil, err } - if _, err = customCache.GetInformer(context.TODO(), &certmanagerv1.ClusterIssuer{}); err != nil { + if _, err = customCache.GetInformer(ctx, &certmanagerv1.ClusterIssuer{}); err != nil { return nil, err }test/library/utils.go (1)
59-69: Use the same ctx consistently inside DeleteTestingNS.You define ctx on Line 59, but call PollUntilContextTimeout with context.TODO() on Line 69. Use the local ctx for consistency and easier future cancellation/threading from callers.
- if err := wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, 30*time.Second, true, func(context.Context) (bool, error) { + if err := wait.PollUntilContextTimeout(ctx, 1*time.Second, 30*time.Second, true, func(context.Context) (bool, error) {test/e2e/cert_manager_deployment_test.go (5)
134-151: Handle secret GET errors explicitly in poll.You log TLS-config failure but ignore err from the Secret GET, which can hide real failures. Mirror the NotFound handling you used elsewhere.
- secret, err := loader.KubeClient.CoreV1().Secrets(ingress.ObjectMeta.Namespace).Get(ctx, "ingress-prod-secret", metav1.GetOptions{}) - tlsConfig, isvalid := library.GetTLSConfig(secret) - if !isvalid { - t.Logf("Unable to retrieve the TLS config: %v", err) - return false, nil - } + secret, err := loader.KubeClient.CoreV1().Secrets(ingress.ObjectMeta.Namespace).Get(ctx, "ingress-prod-secret", metav1.GetOptions{}) + if errors.IsNotFound(err) { + t.Logf("TLS secret not found yet") + return false, nil + } + if err != nil { + return false, err + } + tlsConfig, isvalid := library.GetTLSConfig(secret) + if !isvalid { + t.Logf("Unable to build TLS config from secret") + return false, nil + }
92-117: Go style: prefer camelCase variable names.ingress_host and path_type are non‑idiomatic; rename to ingressHost and pathType.
- ingress_host := "eaic." + appsDomain - path_type := networkingv1.PathTypePrefix + ingressHost := "eaic." + appsDomain + pathType := networkingv1.PathTypePrefix ... - Host: ingress_host, + Host: ingressHost, ... - PathType: &path_type, + PathType: &pathType, ... - Hosts: []string{ingress_host}, + Hosts: []string{ingressHost}, ... - secret, err := loader.KubeClient.CoreV1().Secrets(ingress.ObjectMeta.Namespace).Get(ctx, "ingress-prod-secret", metav1.GetOptions{}) + secret, err := loader.KubeClient.CoreV1().Secrets(ingress.ObjectMeta.Namespace).Get(ctx, "ingress-prod-secret", metav1.GetOptions{}) ... - is_host_correct, err := library.VerifyHostname(ingress_host, tlsConfig.Clone()) + isHostCorrect, err := library.VerifyHostname(ingressHost, tlsConfig.Clone()) ... - is_not_expired, err := library.VerifyExpiry(ingress_host+":443", tlsConfig.Clone()) + isNotExpired, err := library.VerifyExpiry(ingressHost+":443", tlsConfig.Clone()) - return is_host_correct && is_not_expired, nil + return isHostCorrect && isNotExpired, nilAlso applies to: 124-151
327-329: Fix typos in helper names (“Controlle” -> “Controller”).Aligns with type names and improves readability. Update call sites accordingly.
- addValidControlleDeploymentConfig(updatedOperator) + addValidControllerDeploymentConfig(updatedOperator) ... -func addValidControlleDeploymentConfig(operator *v1alpha1.CertManager) { +func addValidControllerDeploymentConfig(operator *v1alpha1.CertManager) { @@ } - -func addInvalidControlleOverrideEnv(operator *v1alpha1.CertManager) { +func addInvalidControllerOverrideEnv(operator *v1alpha1.CertManager) {And later:
- addInvalidControlleOverrideEnv(updatedOperator) + addInvalidControllerOverrideEnv(updatedOperator)Also applies to: 396-416, 417-426
244-249: Minor: use require.NoErrorf when passing a formatted message.Current call uses NoError with a format string.
- require.NoError(t, err, "failed to create certificate: %v", err) + require.NoErrorf(t, err, "failed to create certificate: %v", err)
264-305: Avoid long sleeps inside wait.PollUntilContextTimeout.Sleeping within the poll function stalls the poller and can confuse timeouts. Compute the target renewal time and wait until then outside the poll loop, or increase PollInterval and remove the internal sleep.
Example approach (conceptual):
- Compute renewAt := expiryTime.Add(-renewBefore).Add(30*time.Second)
- time.Sleep(time.Until(renewAt))
- Then run a short PollImmediate to assert renewal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
pkg/controller/deployment/deployment_helper_test.go(2 hunks)pkg/controller/istiocsr/certificates_test.go(1 hunks)pkg/controller/istiocsr/controller.go(2 hunks)pkg/controller/istiocsr/controller_test.go(1 hunks)pkg/controller/istiocsr/test_utils.go(1 hunks)pkg/operator/informers/externalversions/factory.go(1 hunks)test/e2e/cert_manager_deployment_test.go(1 hunks)test/e2e/certificates_test.go(2 hunks)test/e2e/overrides_test.go(2 hunks)test/library/utils.go(2 hunks)
🔇 Additional comments (11)
test/e2e/certificates_test.go (2)
55-55: LGTM on context choice in BeforeAll.Using context.TODO() here is consistent with the PR objective.
865-865: LGTM on context choice in Self-signed BeforeAll.Matches the repo‑wide TODO() standardization.
test/e2e/overrides_test.go (2)
23-24: LGTM: switched to context.TODO().Consistent with the PR goal.
599-600: LGTM: switched to context.TODO().Consistent with the PR goal.
pkg/controller/istiocsr/test_utils.go (1)
39-43: LGTM: test reconciler uses context.TODO().Aligned with the project‑wide convention.
pkg/controller/istiocsr/controller_test.go (1)
395-399: LGTM: Reconcile invoked with context.TODO().Matches the standardized usage.
pkg/controller/istiocsr/certificates_test.go (1)
221-226: LGTM: Get with context.TODO().Consistent with the PR objective.
pkg/controller/deployment/deployment_helper_test.go (2)
337-339: LGTM: WithCancel(context.TODO()).Consistent with repo‑wide standardization.
859-861: LGTM: WithCancel(context.TODO()).Consistent with repo‑wide standardization.
pkg/controller/istiocsr/controller.go (1)
66-71: LGTM: controller root ctx set to context.TODO().Aligns with the chosen convention. Ensure request‑scoped work uses the method ctx parameter, not r.ctx.
test/library/utils.go (1)
46-46: Context normalization to context.TODO() looks good.Change aligns with PR goal and is safe here because the poll has its own timeout.
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) | ||
| defer loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) | ||
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "deployment.yaml"), ns.Name) | ||
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "deployment.yaml"), ns.Name) | ||
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "service.yaml"), ns.Name) | ||
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "service.yaml"), ns.Name) |
There was a problem hiding this comment.
Bug: defer calls CreateFromFile instead of DeleteFromFile.
This will leak test resources and fight with later cleanup.
- defer loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name)
+ defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name)📝 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.
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) | |
| defer loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) | |
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "deployment.yaml"), ns.Name) | |
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "deployment.yaml"), ns.Name) | |
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "service.yaml"), ns.Name) | |
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "service.yaml"), ns.Name) | |
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) | |
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) | |
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "deployment.yaml"), ns.Name) | |
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "deployment.yaml"), ns.Name) | |
| loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "service.yaml"), ns.Name) | |
| defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "acme", "service.yaml"), ns.Name) |
🤖 Prompt for AI Agents
In test/e2e/cert_manager_deployment_test.go around lines 169 to 174, the
deferred cleanup for the self_signed certificate uses loader.CreateFromFile
instead of loader.DeleteFromFile causing resource leaks; change the deferred
call to loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata",
"self_signed", "certificate.yaml"), ns.Name) so the certificate created for the
test is properly removed during teardown.
| flag := false | ||
| for _, cond := range operator.Status.Conditions { | ||
| if cond.Type == "cert-manager-controller-deploymentAvailable" { | ||
| flag = cond.Status == opv1.ConditionTrue | ||
| } | ||
|
|
||
| if cond.Type == "cert-manager-controller-deploymentDegraded" { | ||
| flag = cond.Status == opv1.ConditionFalse | ||
| } | ||
|
|
||
| if cond.Type == "cert-manager-controller-deploymentProgressing" { | ||
| flag = cond.Status == opv1.ConditionFalse | ||
| } | ||
| } |
There was a problem hiding this comment.
Logic bug: only the last matched condition wins; require all three to be correct.
Currently flag gets overwritten per condition. Compute booleans and AND them.
- flag := false
- for _, cond := range operator.Status.Conditions {
- if cond.Type == "cert-manager-controller-deploymentAvailable" {
- flag = cond.Status == opv1.ConditionTrue
- }
-
- if cond.Type == "cert-manager-controller-deploymentDegraded" {
- flag = cond.Status == opv1.ConditionFalse
- }
-
- if cond.Type == "cert-manager-controller-deploymentProgressing" {
- flag = cond.Status == opv1.ConditionFalse
- }
- }
+ availableTrue, degradedFalse, progressingFalse := false, false, false
+ for _, cond := range operator.Status.Conditions {
+ switch cond.Type {
+ case "cert-manager-controller-deploymentAvailable":
+ availableTrue = cond.Status == opv1.ConditionTrue
+ case "cert-manager-controller-deploymentDegraded":
+ degradedFalse = cond.Status == opv1.ConditionFalse
+ case "cert-manager-controller-deploymentProgressing":
+ progressingFalse = cond.Status == opv1.ConditionFalse
+ }
+ }
+ flag := availableTrue && degradedFalse && progressingFalse📝 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.
| flag := false | |
| for _, cond := range operator.Status.Conditions { | |
| if cond.Type == "cert-manager-controller-deploymentAvailable" { | |
| flag = cond.Status == opv1.ConditionTrue | |
| } | |
| if cond.Type == "cert-manager-controller-deploymentDegraded" { | |
| flag = cond.Status == opv1.ConditionFalse | |
| } | |
| if cond.Type == "cert-manager-controller-deploymentProgressing" { | |
| flag = cond.Status == opv1.ConditionFalse | |
| } | |
| } | |
| availableTrue, degradedFalse, progressingFalse := false, false, false | |
| for _, cond := range operator.Status.Conditions { | |
| switch cond.Type { | |
| case "cert-manager-controller-deploymentAvailable": | |
| availableTrue = cond.Status == opv1.ConditionTrue | |
| case "cert-manager-controller-deploymentDegraded": | |
| degradedFalse = cond.Status == opv1.ConditionFalse | |
| case "cert-manager-controller-deploymentProgressing": | |
| progressingFalse = cond.Status == opv1.ConditionFalse | |
| } | |
| } | |
| flag := availableTrue && degradedFalse && progressingFalse |
🤖 Prompt for AI Agents
In test/e2e/cert_manager_deployment_test.go around lines 375 to 388, the loop
overwrites a single flag for each matched condition so the last matched
condition determines the result; instead, introduce three separate booleans
(e.g., availableOK, degradedOK, progressingOK), set each when its corresponding
condition is seen (available should be ConditionTrue, degraded and progressing
should be ConditionFalse), then after the loop assert that all three are true by
combining them with a logical AND; ensure uninitialized/missing conditions are
treated as failures.
sebrandon1
force-pushed
the
ctx_consistency
branch
from
8d0dc18 to
e8c5eff
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/cert_manager_deployment_test.go (2)
169-171: Bug: defer calls CreateFromFile instead of DeleteFromFile (leaks resources).This will leave the certificate behind and can break later tests/teardown.
- defer loader.CreateFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name) + defer loader.DeleteFromFile(testassets.ReadFile, filepath.Join("testdata", "self_signed", "certificate.yaml"), ns.Name)
375-388: Logic bug: last matched condition wins; require all three to be correct.Use three booleans and AND them.
- flag := false - for _, cond := range operator.Status.Conditions { - if cond.Type == "cert-manager-controller-deploymentAvailable" { - flag = cond.Status == opv1.ConditionTrue - } - if cond.Type == "cert-manager-controller-deploymentDegraded" { - flag = cond.Status == opv1.ConditionFalse - } - if cond.Type == "cert-manager-controller-deploymentProgressing" { - flag = cond.Status == opv1.ConditionFalse - } - } + availableTrue, degradedFalse, progressingFalse := false, false, false + for _, cond := range operator.Status.Conditions { + switch cond.Type { + case "cert-manager-controller-deploymentAvailable": + availableTrue = cond.Status == opv1.ConditionTrue + case "cert-manager-controller-deploymentDegraded": + degradedFalse = cond.Status == opv1.ConditionFalse + case "cert-manager-controller-deploymentProgressing": + progressingFalse = cond.Status == opv1.ConditionFalse + } + } + flag := availableTrue && degradedFalse && progressingFalse
🧹 Nitpick comments (4)
pkg/controller/istiocsr/controller.go (1)
117-117: Top-level ctx: prefer Background() or manager root ctx; consider removing stored ctx.context.TODO() is a placeholder. For a long-lived struct field, use context.Background() or a manager/root ctx, and pass request-scoped ctx down from Reconcile. If r.ctx is unused here, consider deleting the field to avoid ambiguity. Please verify usage before changing.
test/e2e/cert_manager_deployment_test.go (3)
92-117: Go naming: use camelCase for local vars.Rename ingress_host -> ingressHost and path_type -> pathType.
- ingress_host := "eaic." + appsDomain - path_type := networkingv1.PathTypePrefix + ingressHost := "eaic." + appsDomain + pathType := networkingv1.PathTypePrefix @@ - Host: ingress_host, + Host: ingressHost, @@ - PathType: &path_type, + PathType: &pathType, @@ - Hosts: []string{ingress_host}, + Hosts: []string{ingressHost},
136-140: Avoid logging a stale err when TLS config not ready.err here is from the prior Secret Get; log a clear message instead.
- tlsConfig, isvalid := library.GetTLSConfig(secret) - if !isvalid { - t.Logf("Unable to retrieve the TLS config: %v", err) + tlsConfig, isvalid := library.GetTLSConfig(secret) + if !isvalid { + t.Logf("TLS config not yet available in secret %s/%s", ingress.ObjectMeta.Namespace, "ingress-prod-secret") return false, nil }
396-417: Typo in helper names; fix and update call sites.s/Controlle/Controller/ for clarity.
- func addValidControlleDeploymentConfig(operator *v1alpha1.CertManager) { + func addValidControllerDeploymentConfig(operator *v1alpha1.CertManager) { @@ - func addInvalidControlleOverrideEnv(operator *v1alpha1.CertManager) { + func addInvalidControllerOverrideEnv(operator *v1alpha1.CertManager) {And update call sites:
- addValidControlleDeploymentConfig(updatedOperator) + addValidControllerDeploymentConfig(updatedOperator) @@ - addInvalidControlleOverrideEnv(updatedOperator) + addInvalidControllerOverrideEnv(updatedOperator)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
pkg/controller/deployment/deployment_helper_test.go(2 hunks)pkg/controller/istiocsr/certificates_test.go(1 hunks)pkg/controller/istiocsr/controller.go(1 hunks)pkg/controller/istiocsr/controller_test.go(1 hunks)pkg/controller/istiocsr/test_utils.go(1 hunks)pkg/operator/informers/externalversions/factory.go(1 hunks)test/e2e/cert_manager_deployment_test.go(1 hunks)test/e2e/certificates_test.go(2 hunks)test/e2e/overrides_test.go(2 hunks)test/library/utils.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/operator/informers/externalversions/factory.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/controller/istiocsr/certificates_test.go
- pkg/controller/istiocsr/controller_test.go
- pkg/controller/istiocsr/test_utils.go
- test/e2e/certificates_test.go
- test/library/utils.go
- pkg/controller/deployment/deployment_helper_test.go
🔇 Additional comments (3)
test/e2e/overrides_test.go (2)
735-737: LGTM on switching to context.TODO().
30-31: Review statement on context usage for consistency.The remaining
context.Background()calls at lines 624, 642, 667, 685, 710, and 728 should be reviewed for consistency. Since lines 30-31 were changed tocontext.TODO(), clarify whether thepollTillDeploymentAvailable()calls should similarly usecontext.TODO()for consistency, or document whycontext.Background()is appropriate for polling operations.test/e2e/cert_manager_deployment_test.go (1)
1-10: Confirm intent to (re)introduce this e2e file; prior refactor removed it.Past review notes say this file was removed and its presence breaks CI (go:embed patterns may fail if testdata is absent). If unintended, delete the file to unblock CI; otherwise ensure embedded testdata exists under test/e2e/testdata/.
| // include grace time for certificate rotation and for secret change propagation to | ||
| // the pod, to the sleep time. | ||
| timeUntil := (time.Until(expiryTime) - renewBeforeDuration) + time.Second*30 | ||
| time.Sleep(timeUntil) | ||
|
|
There was a problem hiding this comment.
Don’t sleep inside PollUntilContextTimeout; it blocks and ignores ctx.
time.Sleep within the condition function can exceed TestTimeout and defeats cancellation. Compute a deadline and let polling cadence drive.
- // include grace time for certificate rotation and for secret change propagation to
- // the pod, to the sleep time.
- timeUntil := (time.Until(expiryTime) - renewBeforeDuration) + time.Second*30
- time.Sleep(timeUntil)
+ // Wait until we're past the renewal window (+30s grace) using the poll cadence.
+ deadline := expiryTime.Add(-renewBeforeDuration).Add(30 * time.Second)
+ if time.Now().Before(deadline) {
+ return false, nil
+ }If preferred, compute deadline once before the poll and capture it via closure:
- err = wait.PollUntilContextTimeout(ctx, PollInterval, TestTimeout, true, func(ctx context.Context) (bool, error) {
+ deadline := expiryTime.Add(-renewBeforeDuration).Add(30 * time.Second)
+ err = wait.PollUntilContextTimeout(ctx, PollInterval, TestTimeout, true, func(ctx context.Context) (bool, error) {
+ if time.Now().Before(deadline) {
+ return false, nil
+ }
// ... rest unchanged ...📝 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.
| // include grace time for certificate rotation and for secret change propagation to | |
| // the pod, to the sleep time. | |
| timeUntil := (time.Until(expiryTime) - renewBeforeDuration) + time.Second*30 | |
| time.Sleep(timeUntil) | |
| // Wait until we're past the renewal window (+30s grace) using the poll cadence. | |
| deadline := expiryTime.Add(-renewBeforeDuration).Add(30 * time.Second) | |
| if time.Now().Before(deadline) { | |
| return false, nil | |
| } |
🤖 Prompt for AI Agents
In test/e2e/cert_manager_deployment_test.go around lines 283 to 287, remove the
direct time.Sleep call inside the PollUntilContextTimeout condition because it
blocks and ignores ctx; instead compute a deadline before entering the poll
(deadline := time.Now().Add((time.Until(expiryTime) - renewBeforeDuration) +
30*time.Second)) and capture that deadline in the closure, then inside the poll
check time.Until(deadline) or return done/error so the poller’s cadence and
context cancellation control waiting; ensure you use the poll interval passed to
PollUntilContextTimeout rather than sleeping.
|
@sebrandon1: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
There were
14instances ofcontext.Backgroundand20instances ofcontext.TODO(). Since there were moreTODO()references, I'm ensuring all of the references to context are the same.Similar to: