kms: add readiness check hook to revision controller#2316
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Walkthrough
ChangesRevision readiness check feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto 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
bertinatto
force-pushed
the
kms-plugin-lifecycle-postcondition
branch
from
6507f50 to
7ccf701
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/revisioncontroller/revision_controller_test.go (1)
810-822: ⚡ Quick winConsider adding test coverage for the postcondition functionality.
The existing tests verify signature compatibility by passing
nil, but there are no tests exercising the postcondition behavior (success, failure returningfalse, and failure returning an error). This mirrors the existingTestSyncWithRevisionPreconditiontest structure.Would you like me to generate test cases for the postcondition functionality similar to
TestSyncWithRevisionPrecondition?🤖 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/operator/revisioncontroller/revision_controller_test.go` around lines 810 - 822, Add test coverage for the postcondition functionality in the revision controller by creating new test cases that mirror the existing TestSyncWithRevisionPrecondition test structure. Create tests that exercise the postcondition parameter (currently set to nil in NewRevisionController) by implementing mock postcondition functions that cover three scenarios: successful postcondition execution, postcondition returning false, and postcondition returning an error. This will ensure the postcondition behavior is properly validated alongside the existing precondition tests.
🤖 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.
Nitpick comments:
In `@pkg/operator/revisioncontroller/revision_controller_test.go`:
- Around line 810-822: Add test coverage for the postcondition functionality in
the revision controller by creating new test cases that mirror the existing
TestSyncWithRevisionPrecondition test structure. Create tests that exercise the
postcondition parameter (currently set to nil in NewRevisionController) by
implementing mock postcondition functions that cover three scenarios: successful
postcondition execution, postcondition returning false, and postcondition
returning an error. This will ensure the postcondition behavior is properly
validated alongside the existing precondition tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9666444d-ec88-4170-9693-0ad58ff65b5f
📒 Files selected for processing (4)
pkg/operator/apiserver/controllerset/apiservercontrollerset.gopkg/operator/revisioncontroller/revision_controller.gopkg/operator/revisioncontroller/revision_controller_test.gopkg/operator/staticpod/controllers.go
bertinatto
force-pushed
the
kms-plugin-lifecycle-postcondition
branch
from
7ccf701 to
9261746
Compare
|
This PR looks good to me. But I think we need review from @p0lyn0mial |
bertinatto
force-pushed
the
kms-plugin-lifecycle-postcondition
branch
from
9261746 to
c98b71e
Compare
bertinatto
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
|
||
| // PostconditionFunc validates the assembled revision data after all resources have been copied but before the revision is marked as ready. | ||
| type PostconditionFunc func(ctx context.Context, revision int32) (bool, error) | ||
|
|
There was a problem hiding this comment.
I just realized that Postcondition might not be the best name because the check is done before the operator conditions are set, right before the revision is marked as ready, not after the condition is set. Maybe RevisionReadinessCheck is a better name?
There was a problem hiding this comment.
I went ahead and renamed it
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/revisioncontroller/revision_controller_test.go`:
- Around line 940-941: The error return value from the
GetStaticPodOperatorState() call on staticPodOperatorClient is being discarded
with an underscore, which can mask state-fetch failures and cause the status
assertion to run on invalid data. Capture the error return value instead of
ignoring it, and add a require.NoError() assertion immediately after the
GetStaticPodOperatorState() call to ensure the state was successfully fetched
before proceeding with the status assertion.
🪄 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: ade74ff6-ffa7-4c5f-8ed9-988f11d993ed
📒 Files selected for processing (4)
pkg/operator/apiserver/controllerset/apiservercontrollerset.gopkg/operator/revisioncontroller/revision_controller.gopkg/operator/revisioncontroller/revision_controller_test.gopkg/operator/staticpod/controllers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/apiserver/controllerset/apiservercontrollerset.go
- pkg/operator/revisioncontroller/revision_controller.go
- pkg/operator/staticpod/controllers.go
| _, status, _, _ := staticPodOperatorClient.GetStaticPodOperatorState() | ||
| require.Equal(t, tc.expUpdatedLatestAvailableRevision, status.LatestAvailableRevision) |
There was a problem hiding this comment.
Handle GetStaticPodOperatorState error before asserting on status.
The error return is currently discarded, which can hide state-fetch failures and make the assertion run on invalid data.
As per coding guidelines, "Never ignore error returns".
✅ Suggested fix
- _, status, _, _ := staticPodOperatorClient.GetStaticPodOperatorState()
+ _, status, _, statusErr := staticPodOperatorClient.GetStaticPodOperatorState()
+ require.NoError(t, statusErr)
require.Equal(t, tc.expUpdatedLatestAvailableRevision, status.LatestAvailableRevision)📝 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.
| _, status, _, _ := staticPodOperatorClient.GetStaticPodOperatorState() | |
| require.Equal(t, tc.expUpdatedLatestAvailableRevision, status.LatestAvailableRevision) | |
| _, status, _, statusErr := staticPodOperatorClient.GetStaticPodOperatorState() | |
| require.NoError(t, statusErr) | |
| require.Equal(t, tc.expUpdatedLatestAvailableRevision, status.LatestAvailableRevision) |
🤖 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/operator/revisioncontroller/revision_controller_test.go` around lines 940
- 941, The error return value from the GetStaticPodOperatorState() call on
staticPodOperatorClient is being discarded with an underscore, which can mask
state-fetch failures and cause the status assertion to run on invalid data.
Capture the error return value instead of ignoring it, and add a
require.NoError() assertion immediately after the GetStaticPodOperatorState()
call to ensure the state was successfully fetched before proceeding with the
status assertion.
Source: Coding guidelines
bertinatto
force-pushed
the
kms-plugin-lifecycle-postcondition
branch
from
c98b71e to
991151f
Compare
bertinatto
changed the title
bertinatto
force-pushed
the
kms-plugin-lifecycle-postcondition
branch
2 times, most recently
from
d9327ba to
2aae006
Compare
The idea is to validate revision data before marking ready
bertinatto
force-pushed
the
kms-plugin-lifecycle-postcondition
branch
from
2aae006 to
ae223a7
Compare
|
@bertinatto: 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. |
2 participants
Summary by CodeRabbit
false(default: always ready).