CCO-738, CCO-739: Set operator condition to Progressing when pod identity webhook pod are updating#978
CCO-738, CCO-739: Set operator condition to Progressing when pod identity webhook pod are updating#978dlom wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@dlom: This pull request references CCO-738 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 "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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-ci
bot
changed the title
|
@dlom: This pull request references CCO-738 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 "4.22.0" version, but no target version was set. This pull request references CCO-739 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 "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. |
WalkthroughAdded context.Context parameters to many Handler/GetConditions methods and propagated ctx through status reconciliation and client calls; introduced pod-identity-webhook pod-status checks, synchronization for condition access, and new condition generation; updated call sites and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant StatusController as StatusController
participant PodHandler as PodIdentityHandler
participant KubeAPI as KubernetesAPI
participant StatusStore as ClusterOperatorStatus
rect rgba(63,81,181,0.5)
StatusController->>PodHandler: GetConditions(ctx, logger)
end
rect rgba(0,150,136,0.5)
PodHandler->>KubeAPI: CheckPodStatus(ctx) — Get Deployment & PDB
KubeAPI-->>PodHandler: Deployment & PDB status
PodHandler->>PodHandler: compute Available / Progressing
PodHandler-->>StatusController: conditions
end
rect rgba(255,152,0,0.5)
StatusController->>StatusStore: Create/Update(ctx, ClusterOperator with conditions)
StatusStore-->>StatusController: ack / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@dlom: This pull request references CCO-738 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 "4.22.0" version, but no target version was set. This pull request references CCO-739 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 "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 (3)
pkg/operator/status/status_controller.go (1)
191-212: Context not fully propagated to Kubernetes API calls.While
syncStatusnow accepts actxparameter and correctly passes it tohandler.GetConditions(ctx, logger)at line 212, the Kubernetes client calls withinsyncStatusstill usecontext.TODO():
- Line 195:
kubeClient.Get(context.TODO(), ...)- Line 269:
kubeClient.Create(context.TODO(), ...)- Line 285:
kubeClient.Status().Update(context.TODO(), ...)For consistent context propagation (supporting cancellation, deadlines, and tracing), these should use the passed
ctx.♻️ Suggested changes
- err := kubeClient.Get(context.TODO(), types.NamespacedName{Name: constants.CloudCredClusterOperatorName}, co) + err := kubeClient.Get(ctx, types.NamespacedName{Name: constants.CloudCredClusterOperatorName}, co)- if err := kubeClient.Create(context.TODO(), co); err != nil { + if err := kubeClient.Create(ctx, co); err != nil {- if err := kubeClient.Status().Update(context.TODO(), co); err != nil { + if err := kubeClient.Status().Update(ctx, co); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/status/status_controller.go` around lines 191 - 212, syncStatus currently mixes the passed context with context.TODO() in Kubernetes client calls; replace the context.TODO() usages in the kubeClient.Get, kubeClient.Create, and kubeClient.Status().Update invocations inside syncStatus with the function parameter ctx so all API calls use the same propagated context (note: handler.GetConditions already uses ctx).pkg/operator/podidentity/podidentitywebhook_controller.go (1)
403-424: Consider using theoperatorNamespaceconstant and clarify nil replicas behavior.
Line 407 hardcodes
"openshift-cloud-credential-operator"but the constantoperatorNamespaceis already defined at line 42.When
deployment.Spec.Replicasis nil, returning an emptywebhookPodStatus{}(all zeros) may causeAvailable()to returnfalseeven though Kubernetes defaults nil replicas to 1 and the deployment might actually be available.♻️ Suggested improvement
func (r *staticResourceReconciler) CheckPodStatus(ctx context.Context) (*webhookPodStatus, error) { deployment := &appsv1.Deployment{} err := r.client.Get(ctx, client.ObjectKey{ Name: "pod-identity-webhook", - Namespace: "openshift-cloud-credential-operator", + Namespace: operatorNamespace, }, deployment) if err != nil { return nil, err } if deployment.Spec.Replicas == nil { - return &webhookPodStatus{}, nil + // Kubernetes defaults nil replicas to 1 + return &webhookPodStatus{ + desiredReplicas: 1, + availableReplicas: deployment.Status.AvailableReplicas, + updatedReplicas: deployment.Status.UpdatedReplicas, + }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/podidentity/podidentitywebhook_controller.go` around lines 403 - 424, In CheckPodStatus, replace the hardcoded namespace string with the existing operatorNamespace constant and handle nil deployment.Spec.Replicas by treating it as Kubernetes default of 1 instead of returning an empty webhookPodStatus; when deployment.Spec.Replicas == nil, set desiredReplicas to 1 and populate availableReplicas/updatedReplicas from deployment.Status so that Available() reflects the real deployment state (refer to function CheckPodStatus and the webhookPodStatus struct).pkg/operator/podidentity/podidentitywebhook_controller_test.go (1)
344-466: Good test coverage, consider adding a "deployment not found" test case.The tests thoroughly cover various deployment status scenarios. Consider adding a test case where the deployment doesn't exist to verify error handling in
CheckPodStatus.📝 Additional test case
{ name: "Deployment not found", deployment: nil, expectErr: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/podidentity/podidentitywebhook_controller_test.go` around lines 344 - 466, Add a test case to TestCheckPodStatus to verify behavior when the Deployment is missing: include an entry with name "Deployment not found", deployment: nil, and expectErr: true; ensure the test loop keeps the existing logic that builds existing runtime.Objects from test.deployment, so when deployment is nil the fake client has no Deployment and CheckPodStatus (on staticResourceReconciler) should return a non-nil error which the test asserts.
🤖 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/operator/podidentity/podidentitywebhook_controller.go`:
- Around line 435-455: The current code assigns conditions := r.conditions which
can alias the underlying slice and cause unexpected mutation when append is
used; replace that with a non-aliased copy (e.g., conditions :=
append([]configv1.ClusterOperatorStatusCondition(nil), r.conditions...)) so
appends do not modify r.conditions, and remove the trailing "\n" characters from
the Message fields in the ClusterOperatorStatusCondition entries (the ones built
when !podStatus.Available() and when podStatus.Progressing()) so messages are
clean and consistent.
---
Nitpick comments:
In `@pkg/operator/podidentity/podidentitywebhook_controller_test.go`:
- Around line 344-466: Add a test case to TestCheckPodStatus to verify behavior
when the Deployment is missing: include an entry with name "Deployment not
found", deployment: nil, and expectErr: true; ensure the test loop keeps the
existing logic that builds existing runtime.Objects from test.deployment, so
when deployment is nil the fake client has no Deployment and CheckPodStatus (on
staticResourceReconciler) should return a non-nil error which the test asserts.
In `@pkg/operator/podidentity/podidentitywebhook_controller.go`:
- Around line 403-424: In CheckPodStatus, replace the hardcoded namespace string
with the existing operatorNamespace constant and handle nil
deployment.Spec.Replicas by treating it as Kubernetes default of 1 instead of
returning an empty webhookPodStatus; when deployment.Spec.Replicas == nil, set
desiredReplicas to 1 and populate availableReplicas/updatedReplicas from
deployment.Status so that Available() reflects the real deployment state (refer
to function CheckPodStatus and the webhookPodStatus struct).
In `@pkg/operator/status/status_controller.go`:
- Around line 191-212: syncStatus currently mixes the passed context with
context.TODO() in Kubernetes client calls; replace the context.TODO() usages in
the kubeClient.Get, kubeClient.Create, and kubeClient.Status().Update
invocations inside syncStatus with the function parameter ctx so all API calls
use the same propagated context (note: handler.GetConditions already uses ctx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b15769e-7d22-4cfe-82d7-5dccbd847557
📒 Files selected for processing (14)
pkg/operator/cleanup/status.gopkg/operator/credentialsrequest/credentialsrequest_controller.gopkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_label_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.gopkg/operator/credentialsrequest/status.gopkg/operator/loglevel/status.gopkg/operator/podidentity/podidentitywebhook_controller.gopkg/operator/podidentity/podidentitywebhook_controller_test.gopkg/operator/secretannotator/status/status.gopkg/operator/status/status_controller.gopkg/operator/status/status_controller_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #978 +/- ##
==========================================
+ Coverage 46.31% 46.46% +0.14%
==========================================
Files 98 98
Lines 12225 12309 +84
==========================================
+ Hits 5662 5719 +57
- Misses 5913 5940 +27
Partials 650 650
🚀 New features to boost your workflow:
|
dlom
force-pushed
the
CCO-738
branch
2 times, most recently
from
6274a2d to
7c3a20b
Compare
|
@dlom: This pull request references CCO-738 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 "4.22.0" version, but no target version was set. This pull request references CCO-739 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 "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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/operator/podidentity/podidentitywebhook_controller_test.go (1)
471-641: Good test coverage, consider adding error case.The test cases thoroughly verify condition generation logic, including preservation of existing conditions.
Consider adding a test case for when the deployment is not found to verify
GetConditionserror handling behavior:{ name: "Deployment not found - returns error", deployment: nil, expectErr: true, },This would document the expected behavior during bootstrap when the deployment hasn't been created yet.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/podidentity/podidentitywebhook_controller_test.go` around lines 471 - 641, Add a new test case to TestGetConditions to cover the "deployment not found" error path: update the tests slice (in TestGetConditions) with an entry where name is "Deployment not found - returns error", deployment is nil, and expectErr is true; this will exercise staticResourceReconciler.GetConditions and assert that when no Deployment exists the function returns an error (keep other fields zero-values and ensure the fake client is built with no runtime objects so GetConditions returns the expected error).pkg/operator/credentialsrequest/status.go (1)
45-51: Consider propagating context toGetRelatedObjects.
GetRelatedObjectsusescontext.TODO()when callinggetOperatorState, whileGetConditionsproperly receives and propagates context. This inconsistency meansGetRelatedObjectswon't respect cancellation or deadlines.If the
Handlerinterface'sGetRelatedObjectssignature is updated in a follow-up change, this can be addressed then. Otherwise, consider adding a TODO comment to track this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/credentialsrequest/status.go` around lines 45 - 51, GetRelatedObjects currently calls getOperatorState with context.TODO(), which prevents honoring cancellations/deadlines; update GetRelatedObjects to either accept and pass a context (preferred: change signature to accept ctx and call getOperatorState(ctx, logger)) or, if the Handler interface can't change now, add a clear TODO comment inside GetRelatedObjects noting that context.TODO() is temporary and must be replaced with a propagated context once the Handler/GetRelatedObjects signature is updated to accept context; reference GetRelatedObjects and getOperatorState in the comment.
🤖 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/operator/status/status_controller_test.go`:
- Around line 458-460: The assertion in the loop currently compares
test.expected.RelatedObjects[i] to itself which always passes; change the
assertion to compare the expected element to the actual result element (e.g.,
replace assert.Equal(t, expected, test.expected.RelatedObjects[i]) with
assert.Equal(t, expected, test.actual.RelatedObjects[i]) or the equivalent
actual/result field used in the test) so the loop verifies actual output against
test.expected.RelatedObjects; update the assert call inside the for loop
accordingly.
---
Nitpick comments:
In `@pkg/operator/credentialsrequest/status.go`:
- Around line 45-51: GetRelatedObjects currently calls getOperatorState with
context.TODO(), which prevents honoring cancellations/deadlines; update
GetRelatedObjects to either accept and pass a context (preferred: change
signature to accept ctx and call getOperatorState(ctx, logger)) or, if the
Handler interface can't change now, add a clear TODO comment inside
GetRelatedObjects noting that context.TODO() is temporary and must be replaced
with a propagated context once the Handler/GetRelatedObjects signature is
updated to accept context; reference GetRelatedObjects and getOperatorState in
the comment.
In `@pkg/operator/podidentity/podidentitywebhook_controller_test.go`:
- Around line 471-641: Add a new test case to TestGetConditions to cover the
"deployment not found" error path: update the tests slice (in TestGetConditions)
with an entry where name is "Deployment not found - returns error", deployment
is nil, and expectErr is true; this will exercise
staticResourceReconciler.GetConditions and assert that when no Deployment exists
the function returns an error (keep other fields zero-values and ensure the fake
client is built with no runtime objects so GetConditions returns the expected
error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a71ebea7-a4a4-424a-b6a3-b92543e045a7
📒 Files selected for processing (14)
pkg/operator/cleanup/status.gopkg/operator/credentialsrequest/credentialsrequest_controller.gopkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_label_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.gopkg/operator/credentialsrequest/status.gopkg/operator/loglevel/status.gopkg/operator/podidentity/podidentitywebhook_controller.gopkg/operator/podidentity/podidentitywebhook_controller_test.gopkg/operator/secretannotator/status/status.gopkg/operator/status/status_controller.gopkg/operator/status/status_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_label_test.go
|
@dlom: This pull request references CCO-738 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 "4.22.0" version, but no target version was set. This pull request references CCO-739 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 "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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/status/status_controller.go (1)
256-265:⚠️ Potential issue | 🟡 MinorProgressing condition may have inconsistent Reason/Message when version changes.
When the version changes,
progressing.Statusis set toConditionTrue, but theReasonandMessagefields are not updated. If no handler explicitly set Progressing, this results inStatus=TruewithReason="AsExpected"andMessage="All is well", which is semantically inconsistent.Consider setting an appropriate Reason and Message when forcing Progressing to True due to a version change.
🔧 Proposed fix
if !reflect.DeepEqual(oldVersions, co.Status.Versions) { logger.WithFields(log.Fields{ "old": oldVersions, "new": co.Status.Versions, }).Info("version has changed, updating progressing condition lastTransitionTime") progressing, _ := findClusterOperatorCondition(co.Status.Conditions, configv1.OperatorProgressing) // We know this should be there. progressing.LastTransitionTime = metav1.Now() progressing.Status = configv1.ConditionTrue + progressing.Reason = "VersionChanged" + progressing.Message = "Operator version is updating" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/status/status_controller.go` around lines 256 - 265, When detecting a version change (comparing oldVersions and co.Status.Versions) the code updates progressing.LastTransitionTime and progressing.Status but does not update progressing.Reason and progressing.Message, causing an inconsistent Progressing=True with stale "AsExpected"/"All is well" text; modify the block that uses findClusterOperatorCondition (the progressing variable) to also set progressing.Reason and progressing.Message to reflect the forced progress state (e.g. Reason="VersionChange" and a short Message like "Cluster operator is progressing due to version update"), ensuring you still call metav1.Now() and set progressing.Status = configv1.ConditionTrue and that you handle the returned progressing pointer from findClusterOperatorCondition safely.
🤖 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/operator/podidentity/podidentitywebhook_controller.go`:
- Around line 429-434: GetConditions currently aborts on CheckPodStatus errors
which causes Reconcile-cached conditions (e.g.,
reasonStaticResourceReconcileFailed stored in r.conditions) to be dropped;
instead, when CheckPodStatus(ctx) returns an error, log it but do not return
early — merge or return the existing r.conditions so syncStatus can see the
degraded state; in other words, in GetConditions keep using r.conditions as the
base, only augment with podStatus when CheckPodStatus succeeds, and ensure the
function returns the conditions (and nil error) rather than nil,error when pod
status lookup fails.
- Around line 389-424: The Progressing check is too loose; update
webhookPodStatus and its usage so rollout isn't marked complete while old surge
pods still exist: add a field to webhookPodStatus (e.g., totalReplicas or
replicas) and populate it in CheckPodStatus from deployment.Status.Replicas,
then change Progressing() to return true unless both updatedReplicas ==
desiredReplicas AND totalReplicas == desiredReplicas (i.e., Progressing returns
!(wps.updatedReplicas == wps.desiredReplicas && wps.replicas ==
wps.desiredReplicas)), updating references to webhookPodStatus, Progressing(),
and CheckPodStatus accordingly.
- Around line 436-437: The copy of r.conditions in GetConditions() protects
against slice aliasing but not concurrent header reads/writes; add a mutex
(e.g., sync.RWMutex) to the reconciler struct (the type that owns r.conditions,
e.g., PodIdentityWebhookReconciler), use r.mu.RLock()/RUnlock() around the
read+copy in GetConditions(), and use r.mu.Lock()/Unlock() around any writes or
full replacements of r.conditions in Reconcile() (and any other methods that
mutate it) so the slice header cannot be concurrently read while being replaced.
---
Outside diff comments:
In `@pkg/operator/status/status_controller.go`:
- Around line 256-265: When detecting a version change (comparing oldVersions
and co.Status.Versions) the code updates progressing.LastTransitionTime and
progressing.Status but does not update progressing.Reason and
progressing.Message, causing an inconsistent Progressing=True with stale
"AsExpected"/"All is well" text; modify the block that uses
findClusterOperatorCondition (the progressing variable) to also set
progressing.Reason and progressing.Message to reflect the forced progress state
(e.g. Reason="VersionChange" and a short Message like "Cluster operator is
progressing due to version update"), ensuring you still call metav1.Now() and
set progressing.Status = configv1.ConditionTrue and that you handle the returned
progressing pointer from findClusterOperatorCondition safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2750233-7005-4a95-a212-07fa73f48031
📒 Files selected for processing (14)
pkg/operator/cleanup/status.gopkg/operator/credentialsrequest/credentialsrequest_controller.gopkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_label_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.gopkg/operator/credentialsrequest/status.gopkg/operator/loglevel/status.gopkg/operator/podidentity/podidentitywebhook_controller.gopkg/operator/podidentity/podidentitywebhook_controller_test.gopkg/operator/secretannotator/status/status.gopkg/operator/status/status_controller.gopkg/operator/status/status_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/operator/credentialsrequest/credentialsrequest_controller.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go
The Pod Identity Webhook controller will now report Available: False when there are no pods available on the deployment, and Progressing: True when not all pods match the latest deployment spec (motivating factor: when the image on the deployment spec has been updated)
|
@dlom: This pull request references CCO-738 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 "4.22.0" version, but no target version was set. This pull request references CCO-739 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 "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.
🧹 Nitpick comments (1)
pkg/operator/podidentity/podidentitywebhook_controller_test.go (1)
679-696: Consider usingint32parameters directly.The helper function accepts
intparameters and casts them toint32. While this works for typical test values, usingint32parameters directly would be more type-safe and align with the Kubernetes API types.Suggested improvement
-func testDeploymentWithReplicas(desired, available, updated, total int) *appsv1.Deployment { +func testDeploymentWithReplicas(desired, available, updated, total int32) *appsv1.Deployment { int32Ptr := func(i int32) *int32 { return &i } return &appsv1.Deployment{ ObjectMeta: v1.ObjectMeta{ Name: "pod-identity-webhook", Namespace: "openshift-cloud-credential-operator", }, Spec: appsv1.DeploymentSpec{ - Replicas: int32Ptr(int32(desired)), + Replicas: int32Ptr(desired), }, Status: appsv1.DeploymentStatus{ - AvailableReplicas: int32(available), - UpdatedReplicas: int32(updated), - Replicas: int32(total), + AvailableReplicas: available, + UpdatedReplicas: updated, + Replicas: total, }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/podidentity/podidentitywebhook_controller_test.go` around lines 679 - 696, The helper testDeploymentWithReplicas currently takes int parameters and casts them to int32; change its signature to accept int32 parameters (desired, available, updated, total int32) and update the int32Ptr helper to accept an int32 directly, remove unnecessary casts inside Spec/Status, and update any test call sites that pass ints (e.g., convert literals to int32 or add explicit int32() where the helper is invoked) so types align with Kubernetes API types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/podidentity/podidentitywebhook_controller_test.go`:
- Around line 679-696: The helper testDeploymentWithReplicas currently takes int
parameters and casts them to int32; change its signature to accept int32
parameters (desired, available, updated, total int32) and update the int32Ptr
helper to accept an int32 directly, remove unnecessary casts inside Spec/Status,
and update any test call sites that pass ints (e.g., convert literals to int32
or add explicit int32() where the helper is invoked) so types align with
Kubernetes API types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bce23ab9-feca-4c7e-998b-a1404e2a4bcc
📒 Files selected for processing (14)
pkg/operator/cleanup/status.gopkg/operator/credentialsrequest/credentialsrequest_controller.gopkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_label_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.gopkg/operator/credentialsrequest/status.gopkg/operator/loglevel/status.gopkg/operator/podidentity/podidentitywebhook_controller.gopkg/operator/podidentity/podidentitywebhook_controller_test.gopkg/operator/secretannotator/status/status.gopkg/operator/status/status_controller.gopkg/operator/status/status_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/credentialsrequest/credentialsrequest_controller_label_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.go
- pkg/operator/cleanup/status.go
|
@dlom: The following tests 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. |
3 participants
xref: CCO-738
xref: CCO-739
/assign @jstuever
the Pod Identity Webhook controller will now check all pods owned by its deployment for their image version, and indicate progressing if the number of pods doesn't match the desired replicas, or if any of the pods have an older image
Summary by CodeRabbit
New Features
Improvements
Tests