Refactor BIOSVersion webhook
#575
Conversation
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
120e510 to
7d1a12b
Compare
- Refactor `BIOSVersion` webhook - Updated tests
afritzler
force-pushed
the
enh/biosvers-webhook
branch
3 times, most recently
from
7c52de4 to
f5b59a6
Compare
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
f5b59a6 to
29649ae
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughCentralizes test cleanup into AfterEach hooks that delete resource lists and call Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code graph analysis (3)internal/webhook/v1alpha1/biosversion_webhook_test.go (5)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
internal/controller/test_helper.go (11)
🔇 Additional comments (8)
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: 3
🧹 Nitpick comments (1)
internal/webhook/v1alpha1/server_webhook_test.go (1)
46-52: Inconsistent pattern: use type pointer instead of instance.Line 48 passes
server(the instance) toDeleteAllOf, whereas all other webhook tests use the type pointer pattern (e.g.,&metalv1alpha1.Server{}). While both work functionally, using the instance is inconsistent and could cause issues ifserverwere unexpectedly nil.🔎 Suggested fix for consistency
AfterEach(func(ctx context.Context) { By("Deleting the server") - Expect(k8sClient.DeleteAllOf(ctx, server)).To(Succeed()) + Expect(k8sClient.DeleteAllOf(ctx, &metalv1alpha1.Server{})).To(Succeed()) By("Ensuring clean state") controller.EnsureCleanState() })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/cmd/console/console_test.go(2 hunks)internal/webhook/v1alpha1/biosversion_webhook.go(2 hunks)internal/webhook/v1alpha1/biosversion_webhook_test.go(2 hunks)internal/webhook/v1alpha1/bmcsettings_webhook_test.go(2 hunks)internal/webhook/v1alpha1/bmcversion_webhook_test.go(2 hunks)internal/webhook/v1alpha1/endpoint_webhook_test.go(2 hunks)internal/webhook/v1alpha1/server_webhook_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
internal/webhook/v1alpha1/biosversion_webhook_test.go (5)
api/v1alpha1/biosversion_types.go (6)
BIOSVersion(130-136)BIOSVersionSpec(51-63)BIOSVersionTemplate(32-48)ImageSpec(65-78)BIOSVersionStateInProgress(19-19)BIOSVersionStateCompleted(21-21)internal/controller/test_helper.go (1)
EnsureCleanState(15-51)api/v1alpha1/servermaintenance_types.go (2)
ServerMaintenancePolicy(51-51)ServerMaintenancePolicyEnforced(57-57)api/v1alpha1/common_types.go (1)
ObjectReference(16-27)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationForceUpdateInProgress(44-44)
internal/webhook/v1alpha1/endpoint_webhook_test.go (2)
api/v1alpha1/endpoint_types.go (1)
Endpoint(35-41)internal/controller/test_helper.go (1)
EnsureCleanState(15-51)
internal/cmd/console/console_test.go (1)
internal/controller/test_helper.go (1)
EnsureCleanState(15-51)
internal/webhook/v1alpha1/server_webhook_test.go (1)
internal/controller/test_helper.go (1)
EnsureCleanState(15-51)
internal/webhook/v1alpha1/bmcversion_webhook_test.go (2)
api/v1alpha1/bmcversion_types.go (1)
BMCVersion(83-89)internal/controller/test_helper.go (1)
EnsureCleanState(15-51)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (3)
BIOSVersion(130-136)BIOSVersionList(141-145)BIOSVersionStateInProgress(19-19)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
internal/webhook/v1alpha1/bmcsettings_webhook_test.go (2)
api/v1alpha1/bmcsettings_types.go (1)
BMCSettings(80-86)internal/controller/test_helper.go (1)
EnsureCleanState(15-51)
🔇 Additional comments (9)
internal/webhook/v1alpha1/biosversion_webhook.go (5)
25-25: LGTM: Improved log variable naming.The rename from
biosversionlogtoversionLogfollows Go naming conventions and improves readability.
40-42: LGTM: Idiomatic use of embedded client.Embedding
client.Clientpromotes its methods and enables cleaner method calls (v.List()vsv.Client.List()). This is a common and recommended pattern in controller-runtime.
47-59: LGTM: Clean validation logic.The variable renames (
version,versions) and use of the promotedv.List()method improve code consistency. The validation logic correctly checks for duplicate ServerRef references.
62-89: LGTM: Correct update validation logic.The validation properly prevents updates to in-progress BIOS versions (unless force annotation is present) and checks for duplicate ServerRef references. The use of
newVersionin the duplicate check correctly handles the case where the object being updated is in the list.
105-130: LGTM: Well-structured duplicate detection.The refactored function correctly identifies duplicate ServerRef references with proper early returns and clear error messages. The logic correctly handles both create and update scenarios by skipping self-comparison via name matching.
internal/webhook/v1alpha1/bmcsettings_webhook_test.go (1)
45-51: LGTM!The AfterEach cleanup correctly deletes all
BMCSettingsresources and verifies cleanup viacontroller.EnsureCleanState(), which includesBMCSettingsListverification.internal/webhook/v1alpha1/bmcversion_webhook_test.go (1)
47-53: LGTM!The AfterEach cleanup properly handles
BMCVersionresource deletion and state verification.internal/cmd/console/console_test.go (1)
20-28: LGTM!The AfterEach cleanup correctly handles all three resource types (
Server,BMC,BMCSecret) created in the tests, andEnsureCleanState()verifies all of them.internal/webhook/v1alpha1/biosversion_webhook_test.go (1)
50-193: LGTM on test logic!The refactored tests comprehensively cover:
- Create/update validation for duplicate
ServerRef- In-progress state blocking with force override support
- Delete prevention during in-progress state
The tests properly reset state to
Completedbefore cleanup, ensuringAfterEachcan succeed.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @afritzler. * #575 (comment) The following files were modified: * `internal/controller/test_helper.go` * `internal/webhook/v1alpha1/biosversion_webhook.go`
Nuckal777
left a comment
Nuckal777
left a comment
There was a problem hiding this comment.
Thanks. 👍 Just some nitpicks from my side.
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
a21bb3e to
6b81d2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/webhook/v1alpha1/biosversion_webhook_test.go:
- Around line 181-193: Test comment has a typo and the delete assertion is
inconsistent with other tests; update the comment "Deleting the BIOSSettings V1
should fail" to "Deleting the BIOSVersion V1 should fail" and replace the
integration-style call Expect(k8sClient.Delete(ctx,
biosVersionV1)).To(Not(Succeed())) with a validator call for consistency, e.g.,
call validator.ValidateDelete(ctx, biosVersionV1) and assert it returns the
expected error (or document why k8sClient.Delete is necessary if integration
behavior is required); locate symbols biosVersionV1, k8sClient.Delete, and
validator.ValidateDelete (and the surrounding UpdateStatus helper) to apply the
changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/webhook/v1alpha1/biosversion_webhook_test.go (4)
api/v1alpha1/biosversion_types.go (4)
BIOSVersion(130-136)BIOSVersionSpec(51-63)BIOSVersionTemplate(32-48)ImageSpec(65-78)internal/controller/test_helper.go (1)
EnsureCleanState(15-57)api/v1alpha1/servermaintenance_types.go (2)
ServerMaintenancePolicy(51-51)ServerMaintenancePolicyEnforced(57-57)api/v1alpha1/common_types.go (1)
ObjectReference(16-27)
internal/controller/test_helper.go (10)
api/v1alpha1/endpoint_types.go (1)
EndpointList(46-50)api/v1alpha1/bmc_types.go (1)
BMCList(259-263)api/v1alpha1/bmcsecret_types.go (1)
BMCSecretList(62-66)api/v1alpha1/bmcsettings_types.go (1)
BMCSettingsList(91-95)api/v1alpha1/bmcversionset_types.go (1)
BMCVersionSetList(60-64)api/v1alpha1/bmcversion_types.go (1)
BMCVersionList(94-98)api/v1alpha1/biosversion_types.go (1)
BIOSVersionList(141-145)api/v1alpha1/biossettingsset_types.go (1)
BIOSSettingsSetList(60-64)api/v1alpha1/biossettings_types.go (1)
BIOSSettingsList(151-155)api/v1alpha1/servermaintenance_types.go (1)
ServerMaintenanceList(99-103)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (3)
BIOSVersion(130-136)BIOSVersionList(141-145)BIOSVersionStateInProgress(19-19)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run test chart
- GitHub Check: Run test e2e
- GitHub Check: Run linter
- GitHub Check: Run tests
🔇 Additional comments (7)
internal/controller/test_helper.go (4)
19-29: LGTM!The new resource list checks for
EndpointList,BMCList,BMCSecretList, andServerClaimListare correctly implemented and follow the established pattern with consistent naming conventions.
34-38: LGTM!The
BMCVersionSetListandBMCVersionListchecks are correctly implemented with consistent variable naming.
40-47: LGTM!The BIOS-related resource checks are correctly implemented. The
biosSettingsListnaming is appropriate given thatBIOSSettingsis already plural, as previously discussed.
49-53: LGTM!The
ServerMaintenanceListandServerListchecks complete the comprehensive resource cleanup verification with consistent naming.internal/webhook/v1alpha1/biosversion_webhook.go (1)
25-129: LGTM! Clean refactoring with improved code quality.The refactoring improves the codebase in several ways:
- Embedding
client.Client(line 41) enables more idiomatic method calls (v.List()vs.v.Client.List())- Variable renaming (
biosversion→version,biosVersionList→versions) is more concise and clearer- Early return for nil
ServerRef(lines 106-108) is good defensive coding- All validation logic remains correct and consistent
internal/webhook/v1alpha1/biosversion_webhook_test.go (2)
42-48: LGTM! Cleanup approach is correct.The centralized
AfterEachcleanup usingDeleteAllOfandcontroller.EnsureCleanState()aligns with the pattern used across other webhook tests in this PR.
50-153: LGTM! Test structure is clear and comprehensive.The tests properly cover:
- Duplicate
ServerRefvalidation on create and update- Non-duplicate scenarios
- Different field duplication (which should be allowed)
All tests consistently use the validator directly and have clear descriptions.
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
6b81d2f to
f482e18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/webhook/v1alpha1/biosversion_webhook_test.go:
- Line 155: Update the test description string passed to the It(...) call in the
BIOSVersion webhook tests: change "Should no allow update a BIOSVersion is in
progress, but should allow to force update it" to "Should not allow update a
BIOSVersion that is in progress, but should allow forcing the update" (or
similar grammatically-correct phrasing) so the test description reads clearly;
locate the It(...) invocation in the biosversion_webhook_test.go test block and
replace only the string literal.
🧹 Nitpick comments (1)
internal/webhook/v1alpha1/biosversion_webhook.go (1)
118-122: Minor: Inconsistent capitalization in error message.The message uses lowercase "server" initially but uppercase "Server" later. Consider making it consistent.
Proposed fix
- err := fmt.Errorf("server (%s) referred in %s is duplicate of Server (%s) referred in %s", + err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s", version.Spec.ServerRef.Name, version.Name, bv.Spec.ServerRef.Name, bv.Name, )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/test_helper.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/webhook/v1alpha1/biosversion_webhook_test.go (5)
api/v1alpha1/biosversion_types.go (6)
BIOSVersion(130-136)BIOSVersionSpec(51-63)BIOSVersionTemplate(32-48)ImageSpec(65-78)BIOSVersionStateInProgress(19-19)BIOSVersionStateCompleted(21-21)internal/controller/test_helper.go (1)
EnsureCleanState(15-57)api/v1alpha1/servermaintenance_types.go (2)
ServerMaintenancePolicy(51-51)ServerMaintenancePolicyEnforced(57-57)api/v1alpha1/common_types.go (1)
ObjectReference(16-27)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationForceUpdateInProgress(44-44)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (3)
BIOSVersion(130-136)BIOSVersionList(141-145)BIOSVersionStateInProgress(19-19)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run test chart
- GitHub Check: Run tests
- GitHub Check: Run test e2e
🔇 Additional comments (6)
internal/webhook/v1alpha1/biosversion_webhook_test.go (2)
42-48: LGTM - Centralized cleanup pattern looks good.The
AfterEachblock properly cleans up BIOSVersion resources and callsEnsureCleanState()to ensure test isolation. This aligns with the cleanup pattern used in other webhook test files.
50-153: LGTM - Comprehensive ServerRef duplication tests.Good coverage of both create and update validation scenarios. Tests properly verify that:
- Duplicate
ServerRefis rejected on create/update- Non-duplicate
ServerRefis allowed- Duplication in other fields (like
Image) doesn't trigger rejectioninternal/webhook/v1alpha1/biosversion_webhook.go (4)
40-41: Good refactor using embedded client.Embedding
client.Clientdirectly is idiomatic Go and enables cleaner method calls (e.g.,v.List()instead ofv.Client.List()).
47-88: LGTM - Validation logic is sound.Both
ValidateCreateandValidateUpdateproperly:
- Cast and validate the object type
- Check for duplicate
ServerRefacross existing resourcesValidateUpdateadditionally blocks updates when in-progress (unless force annotation is present)
99-100: LGTM - Grammar fix applied.The error message now reads correctly: "The BIOS version is in progress and cannot be deleted".
105-129: LGTM - Duplicate check implementation is correct.Good defensive coding with:
- Early return when
ServerRefis nil (line 106-108)- Self-exclusion to avoid false positives during updates (line 111-113)
- Proper field path construction matching JSON tags (
spec.serverRef.name)
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
f482e18 to
27142c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/webhook/v1alpha1/biosversion_webhook_test.go:
- Line 155: Update the test description string inside the It(...) block in
biosversion_webhook_test.go to fix the grammar; replace "Should no allow update
a BIOSVersion is in progress, but should allow to force update it" with a
corrected sentence such as "Should not allow updating a BIOSVersion that is in
progress, but should allow forcing an update" so the test description is clear
and grammatically correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/test_helper.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (3)
BIOSVersion(130-136)BIOSVersionList(141-145)BIOSVersionStateInProgress(19-19)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
internal/webhook/v1alpha1/biosversion_webhook_test.go (5)
api/v1alpha1/biosversion_types.go (4)
BIOSVersion(130-136)BIOSVersionSpec(51-63)BIOSVersionTemplate(32-48)ImageSpec(65-78)internal/controller/test_helper.go (1)
EnsureCleanState(15-57)api/v1alpha1/servermaintenance_types.go (2)
ServerMaintenancePolicy(51-51)ServerMaintenancePolicyEnforced(57-57)api/v1alpha1/common_types.go (1)
ObjectReference(16-27)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationForceUpdateInProgress(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run test chart
- GitHub Check: Run tests
- GitHub Check: Run test e2e
🔇 Additional comments (7)
internal/webhook/v1alpha1/biosversion_webhook_test.go (1)
181-193: Fix comment typo and consider test consistency.Two issues remain from previous review:
Line 187: Comment says "BIOSSettings V1" but should say "BIOSVersion V1" (copy-paste error)
Line 188: Inconsistent test approach - this test uses
k8sClient.Delete()while all other validation tests call validator methods directly (see lines 65, 106, 129, 152, 169, 173). Consider usingvalidator.ValidateDelete(ctx, biosVersionV1)for consistency with the test suite pattern.📝 Proposed fix
- By("Deleting the BIOSSettings V1 should fail") - Expect(k8sClient.Delete(ctx, biosVersionV1)).To(Not(Succeed())) + By("Deleting the BIOSVersion V1 should fail") + Expect(validator.ValidateDelete(ctx, biosVersionV1)).Error().To(HaveOccurred())Likely an incorrect or invalid review comment.
internal/webhook/v1alpha1/biosversion_webhook.go (6)
25-25: LGTM! Clean refactoring of logging variable and client embedding.The rename from
biosversionlogtoversionLogimproves clarity, and embeddingclient.Client(line 41) follows idiomatic Go patterns. The embedded client is correctly used throughout (lines 55, 84).Also applies to: 41-41
47-59: LGTM! ValidateCreate method refactored with improved naming.Variable naming (
version,versions) is more concise and consistent. The list operation correctly uses the embedded client pattern.
62-89: LGTM! ValidateUpdate method refactored with clearer variable names.The rename to
newVersionandoldVersionimproves code readability. The in-progress validation logic correctly checks the old version's state while applying restrictions to the new version.
92-103: LGTM! ValidateDelete method refactored consistently.Variable naming aligns with the other validation methods. The grammar fix from the previous review has been correctly applied (line 100).
105-108: LGTM! Defensive nil check prevents potential panic.The nil check for
version.Spec.ServerRef(lines 106-108) is essential defensive programming. Without it, line 117 would panic when attempting to accessversion.Spec.ServerRef.Name. Returning nil when ServerRef is nil is correct—there's no duplicate to check.
110-130: LGTM! Correct field path casing and sound duplicate detection logic.The name comparison (lines 111-113) correctly handles both create and update scenarios—it skips the object being validated during updates (preventing false positives) and is harmless during creates (when the object isn't in the list yet).
The field path change from
"ServerRef"to"serverRef"(line 126) is important: Kubernetes field paths in error messages must use JSON field names (lowerCamelCase convention) rather than Go struct names (PascalCase). This ensures error messages correctly identify the problematic field in API requests.
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
27142c6 to
c1634b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/webhook/v1alpha1/biosversion_webhook_test.go (1)
50-66: Minor grammar issue in test descriptions.The article "an" before "BIOSVersion" should be "a" since "BIOS" starts with a consonant sound (/b/). This appears in multiple
By()statements throughout the file.📝 Proposed fix for this and similar occurrences
By("Creating an BIOSVersion") + By("Creating a BIOSVersion")Apply similar fixes to lines 25, 51, 69, 87, 103, 110, 126, 133, and 149.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/test_helper.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (3)
BIOSVersion(130-136)BIOSVersionList(141-145)BIOSVersionStateInProgress(19-19)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run test chart
- GitHub Check: Run test e2e
- GitHub Check: Run tests
🔇 Additional comments (5)
internal/webhook/v1alpha1/biosversion_webhook_test.go (2)
42-48: Cleanup logic looks correct.The centralized
AfterEachcleanup properly deletes all BIOSVersion resources and invokescontroller.EnsureCleanState()to ensure a clean test environment.
155-179: In-progress validation test is well structured.The test properly validates that updates are blocked when in
InProgressstate with a maintenance reference, and allows updates when the force annotation is present. The state transitions are correctly tested.internal/webhook/v1alpha1/biosversion_webhook.go (3)
40-42: Embeddedclient.Clientis a valid pattern.Using an embedded field allows calling
v.List(ctx, versions)directly without thev.Client.List()syntax. Note that line 30 still uses{Client: mgr.GetClient()}which works fine with embedded types in Go.
105-129: Improved duplicate check with proper nil guards.The refactored function now includes:
- Early return if
version.Spec.ServerRefis nil (lines 106-108)- Self-exclusion check to skip the current version during updates (lines 111-113)
- Nil check for each item's
ServerRef(lines 114-116)These guards prevent nil pointer dereferences and ensure correct behavior during both create and update operations.
73-80: Update validation logic correctly handles in-progress state.The condition properly checks:
- Old version is in
InProgressstate- Force update annotation is not present
- ServerMaintenanceRef is set
This ensures users cannot accidentally modify an in-progress BIOS update while still allowing force overrides when needed.
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
c1634b3 to
99de57b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/webhook/v1alpha1/biosversion_webhook_test.go (1)
181-193: Consider using validator method for consistency.This test uses
k8sClient.Delete(ctx, biosVersionV1)(line 188) while all other tests in this file call validator methods directly (e.g., lines 65, 106, 129, 152). For consistency and clearer unit-test focus, consider usingvalidator.ValidateDelete(ctx, biosVersionV1)instead.If full webhook integration testing is specifically needed here, this is fine—but document why.
♻️ Proposed fix for consistency
- By("Deleting the BIOSVersion V1 should fail") - Expect(k8sClient.Delete(ctx, biosVersionV1)).To(Not(Succeed())) + By("Validating deletion of BIOSVersion V1 should fail") + Expect(validator.ValidateDelete(ctx, biosVersionV1)).Error().To(HaveOccurred())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (2)
BIOSVersion(130-136)BIOSVersionList(141-145)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
internal/controller/test_helper.go (12)
api/v1alpha1/endpoint_types.go (1)
EndpointList(46-50)api/v1alpha1/bmc_types.go (1)
BMCList(259-263)api/v1alpha1/bmcsecret_types.go (1)
BMCSecretList(62-66)api/v1alpha1/serverclaim_types.go (1)
ServerClaimList(80-84)api/v1alpha1/bmcsettings_types.go (1)
BMCSettingsList(91-95)api/v1alpha1/bmcversionset_types.go (1)
BMCVersionSetList(60-64)api/v1alpha1/bmcversion_types.go (1)
BMCVersionList(94-98)api/v1alpha1/biosversion_types.go (1)
BIOSVersionList(141-145)api/v1alpha1/biossettingsset_types.go (1)
BIOSSettingsSetList(60-64)api/v1alpha1/biossettings_types.go (1)
BIOSSettingsList(151-155)api/v1alpha1/servermaintenance_types.go (1)
ServerMaintenanceList(99-103)api/v1alpha1/server_types.go (1)
ServerList(439-443)
🔇 Additional comments (8)
internal/webhook/v1alpha1/biosversion_webhook_test.go (2)
42-48: LGTM!The AfterEach cleanup is well-structured and comprehensive. Using
DeleteAllOffollowed bycontroller.EnsureCleanState()ensures proper test isolation.
50-180: LGTM!The test cases are well-structured with clear scenarios:
- Duplicate and non-duplicate ServerRef validation
- Update validation for in-progress BIOSVersions
- Force update with annotations
The use of GenerateName ensures proper test isolation.
internal/controller/test_helper.go (1)
19-53: LGTM!The
EnsureCleanStatefunction comprehensively validates that all resource lists are empty. The addition ofServerClaimList(lines 28-29) andBIOSVersionList(lines 40-41) properly extends the cleanup verification to cover the new resource types introduced in this refactor.internal/webhook/v1alpha1/biosversion_webhook.go (5)
25-42: LGTM!Good refactoring:
- Logger renamed to
versionLogfor clarity- Embedding
client.Client(line 41) is idiomatic and cleaner than a named field
47-59: LGTM!The refactored
ValidateCreateis cleaner with improved naming (version,versions) and correctly uses the embedded client viav.List().
62-89: LGTM!The
ValidateUpdaterefactoring improves clarity withnewVersion/oldVersionnaming. The in-progress state check correctly references the old state and validates against force-update annotations.
92-103: LGTM!The
ValidateDeleterefactoring uses consistent naming and has a clear, grammatically correct error message (line 100).
105-129: LGTM!Excellent refactoring of the duplicate check function:
- Nil check for
version.Spec.ServerRef(lines 106-108) prevents panics- Skip logic for same-name resources (lines 111-112) prevents false positives
- Field path uses proper camelCase
"serverRef"and"name"(line 126), aligning with JSON field naming
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
99de57b to
8ec8269
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/webhook/v1alpha1/biosversion_webhook.go
🔇 Additional comments (7)
internal/webhook/v1alpha1/biosversion_webhook_test.go (6)
50-66: LGTM!The test correctly validates that duplicate
spec.serverRefvalues are rejected during creation.
68-84: LGTM!The test correctly verifies that a BIOSVersion with a unique
spec.serverRefcan be successfully created. Usingk8sClient.Createhere (rather than justvalidator.ValidateCreate) appropriately tests the end-to-end creation flow.
86-107: LGTM!The test correctly validates that updating to a duplicate
spec.serverRefis rejected.
109-130: LGTM!The test correctly verifies that duplicate values in fields other than
spec.serverRef(such asspec.image) are allowed.
132-153: LGTM!The test correctly validates that updating to a unique
spec.serverRefis allowed.
181-193: LGTM!The test correctly validates that deletion is blocked when BIOSVersion is in InProgress state.
internal/controller/test_helper.go (1)
19-53: LGTM!The expanded cleanup verification correctly covers all relevant CRD types including the newly added
EndpointList,BMCSecretList,ServerClaimList,BIOSVersionList,ServerMaintenanceList, andServerList. The implementation is consistent and follows the established pattern.
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
8ec8269 to
9ca3765
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/webhook/v1alpha1/biosversion_webhook_test.go (1)
86-107: Minor grammar issue in comments.Lines 87 and 103 use "an BIOSVersion" which should be "a BIOSVersion" (since "BIOS" is pronounced with a consonant sound).
📝 Proposed fix
- By("Creating an BIOSVersion with different ServerRef") + By("Creating a BIOSVersion with different ServerRef") ... - By("Updating an BIOSVersion V2 to conflicting spec.serverRef") + By("Updating BIOSVersion V2 to conflicting spec.serverRef")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/webhook/v1alpha1/biosversion_webhook_test.go (5)
api/v1alpha1/biosversion_types.go (6)
BIOSVersion(130-136)BIOSVersionSpec(51-63)BIOSVersionTemplate(32-48)ImageSpec(65-78)BIOSVersionStateInProgress(19-19)BIOSVersionStateCompleted(21-21)internal/controller/test_helper.go (1)
EnsureCleanState(15-57)api/v1alpha1/servermaintenance_types.go (2)
ServerMaintenancePolicy(51-51)ServerMaintenancePolicyEnforced(57-57)api/v1alpha1/common_types.go (1)
ObjectReference(16-27)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationForceUpdateInProgress(44-44)
internal/controller/test_helper.go (11)
api/v1alpha1/endpoint_types.go (1)
EndpointList(46-50)api/v1alpha1/bmc_types.go (1)
BMCList(259-263)api/v1alpha1/bmcsecret_types.go (1)
BMCSecretList(62-66)api/v1alpha1/serverclaim_types.go (1)
ServerClaimList(80-84)api/v1alpha1/bmcversionset_types.go (1)
BMCVersionSetList(60-64)api/v1alpha1/bmcversion_types.go (1)
BMCVersionList(94-98)api/v1alpha1/biosversion_types.go (1)
BIOSVersionList(141-145)api/v1alpha1/biossettingsset_types.go (1)
BIOSSettingsSetList(60-64)api/v1alpha1/biossettings_types.go (1)
BIOSSettingsList(151-155)api/v1alpha1/servermaintenance_types.go (1)
ServerMaintenanceList(99-103)api/v1alpha1/server_types.go (1)
ServerList(439-443)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (3)
BIOSVersion(130-136)BIOSVersionList(141-145)BIOSVersionStateInProgress(19-19)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run test e2e
- GitHub Check: Run tests
- GitHub Check: Run test chart
🔇 Additional comments (15)
internal/webhook/v1alpha1/biosversion_webhook_test.go (8)
6-15: LGTM!The imports are correctly organized, and the new
controllerpackage import is appropriately added to support theEnsureCleanState()call in theAfterEachblock.
17-40: LGTM!The
BeforeEachsetup is well-structured with:
- Proper validator initialization with embedded client
- Use of
GenerateNamefor test isolation- Clean resource creation pattern
42-48: LGTM!The
AfterEachcleanup is properly implemented with accurate comments and comprehensive state verification viaEnsureCleanState().
50-66: LGTM!The test correctly validates that duplicate
spec.serverRefvalues are denied during creation. The direct validator call is appropriate for unit testing.
68-84: LGTM!The test correctly validates that non-duplicate
spec.serverRefvalues allow creation. Usingk8sClient.Createhere appropriately tests the full webhook integration for the success path.
109-130: LGTM!The test correctly validates that updating non-
serverReffields (even if they become duplicates) is allowed. The validation logic appropriately focuses only onserverRefuniqueness.
132-153: LGTM!The test correctly validates that updating
serverRefto a non-duplicate value is allowed.
155-179: LGTM!The test correctly validates:
- Updates are denied when
BIOSVersionis inInProgressstate- Updates are allowed with the
ForceUpdateInProgressannotation- State transitions are properly tested
The previous comment about the misleading "finalizer" reference has been addressed.
internal/controller/test_helper.go (1)
19-53: LGTM! Clean and consistent refactoring.The variable naming improvements (e.g.,
bmcs,endpoints,versions) are more concise and idiomatic. All resource types are checked consistently using the same pattern, and the additions ofEndpointList,ServerClaimList, andBIOSVersionListalign with the webhook refactoring objectives.internal/webhook/v1alpha1/biosversion_webhook.go (6)
25-25: LGTM! Improved logger naming.The rename from
biosversionlogtoversionLogis more idiomatic and consistent with Go naming conventions.
40-42: LGTM! Embedding the client simplifies the API.Embedding
client.Clientinstead of using a named field allows direct method calls (e.g.,v.List) without the extra.Clientindirection, which is a common and cleaner pattern for validators.
47-59: LGTM! Clean refactoring with improved naming.The variable renames (
version,versions) are more concise while maintaining clarity. The logic remains correct and the embedded client is used appropriately.
62-89: LGTM! Update validation logic is sound.The refactored code correctly checks the old version's state while using the new version for error construction. Variable naming is clear and consistent throughout.
92-103: LGTM! Delete validation is correct.The validation properly checks the in-progress state and uses the improved error message that was addressed in the previous review.
105-130: LGTM! Improved duplicate checking with proper nil safety.The additions significantly improve robustness:
- Lines 106-108: Nil check for
version.Spec.ServerRefprevents panics- Lines 111-113: Skipping self-comparison is essential for update validation
- Line 114: Nil check for
bv.Spec.ServerRefprevents panics during iteration- Line 126: Field path updated to lowerCamelCase (
"serverRef","name") aligns with JSON naming conventions
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
9ca3765 to
ceda80d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
internal/webhook/v1alpha1/biosversion_webhook_test.go (6)
50-66: Consider more concise comment wording.The test logic is correct. The comment on line 50 could be more concise: "Should deny creation if spec.serverRef is duplicate" (instead of "a spec.serverRef field is duplicate").
68-84: Consider consistent testing approach.Line 83 uses
k8sClient.Createfor integration testing, while other tests (e.g., line 65) usevalidator.ValidateCreatefor unit testing. For consistency across the test suite, consider using the validator method directly:- Expect(k8sClient.Create(ctx, biosVersionV2)).To(Succeed()) + Expect(validator.ValidateCreate(ctx, biosVersionV2)).Error().ToNot(HaveOccurred())This maintains consistency with the unit-test approach used elsewhere in the file.
86-107: Minor comment improvements.Two optional improvements for clarity:
- Line 86: Simplify to "Should deny update if spec.serverRef is duplicate"
- Line 103: Grammar fix - "Updating a BIOSVersion" (not "an BIOSVersion")
109-130: Clarify comment on line 126.The comment mentions "conflicting spec.biosVersionSpec" but the code actually updates
spec.image(line 128). Consider updating the comment to be more specific:- By("Updating an BIOSVersion V2 to conflicting spec.biosVersionSpec") + By("Updating BIOSVersion V2 to duplicate spec.image")Also fixes the grammar ("an BIOSVersion" → "BIOSVersion").
132-153: Minor grammar fix.Line 149: "Updating a BIOSVersion" (not "an BIOSVersion").
155-179: Minor style improvement.Line 161: For consistency with other "By" statements that use gerunds (e.g., "Creating", "Updating", "Patching"), consider "Adding ServerMaintenance reference" instead of "Add ServerMaintenance reference".
internal/webhook/v1alpha1/biosversion_webhook.go (1)
105-130: LGTM: Critical improvements to duplicate detection logic.This refactor includes two important enhancements:
Lines 106-108: The nil check for
version.Spec.ServerRefis excellent defensive coding that prevents potential nil pointer dereferences.Lines 111-113: The self-comparison skip (
version.Name == bv.Name) is critical forValidateUpdateto work correctly—without it, updating a BIOSVersion would always fail with a false duplicate detection sinceValidateUpdatefetches all BIOSVersions and checks the updated version against the entire list.Line 126: The field path uses camelCase
"serverRef"and"name", which correctly matches the JSON tags inBIOSVersionSpec.Optional: Consider consistent capitalization in error message.
Lines 118-119 use inconsistent capitalization:
"server (%s) referred in %s is duplicate of Server (%s)". For polish, consider using consistent capitalization (either all lowercase "server" or title case "Server").✨ Optional: Consistent capitalization
- err := fmt.Errorf("server (%s) referred in %s is duplicate of Server (%s) referred in %s", + err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s",
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/test_helper.gointernal/webhook/v1alpha1/biosversion_webhook.gointernal/webhook/v1alpha1/biosversion_webhook_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/webhook/v1alpha1/biosversion_webhook_test.go (5)
api/v1alpha1/biosversion_types.go (6)
BIOSVersion(130-136)BIOSVersionSpec(51-63)BIOSVersionTemplate(32-48)ImageSpec(65-78)BIOSVersionStateInProgress(19-19)BIOSVersionStateCompleted(21-21)internal/controller/test_helper.go (1)
EnsureCleanState(15-57)api/v1alpha1/servermaintenance_types.go (2)
ServerMaintenancePolicy(51-51)ServerMaintenancePolicyEnforced(57-57)api/v1alpha1/common_types.go (1)
ObjectReference(16-27)api/v1alpha1/constants.go (2)
OperationAnnotation(21-21)OperationAnnotationForceUpdateInProgress(44-44)
internal/webhook/v1alpha1/biosversion_webhook.go (2)
api/v1alpha1/biosversion_types.go (2)
BIOSVersion(130-136)BIOSVersionList(141-145)internal/webhook/v1alpha1/helper.go (2)
ShouldAllowForceUpdateInProgress(12-18)ShouldAllowForceDeleteInProgress(21-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run tests
- GitHub Check: Run test chart
- GitHub Check: Run test e2e
🔇 Additional comments (10)
internal/controller/test_helper.go (3)
19-29: LGTM! Enhanced test coverage with new resource types.The addition of
EndpointListandServerClaimListchecks improves the cleanup verification. The variable naming (endpoints,bmcs,bmcSecrets,claims) follows a consistent lowercase pattern.
34-41: LGTM! BIOSVersionList check added and naming consistency improved.The new
biosVersionscheck forBIOSVersionListaligns well with the PR's BIOSVersion webhook refactoring objective. Variable renamings (bmcVersionSets,bmcVersions,biosVersions) enhance consistency across the function.
46-47: Naming pattern acknowledged.The
Listsuffix onbiosSettingsList(andbmcSettingsListat line 31) follows the rationale discussed in previous reviews: since "Settings" is already plural, the suffix distinguishes the list from individual items. This pattern is consistent and reasonable.internal/webhook/v1alpha1/biosversion_webhook_test.go (2)
42-48: LGTM - Clean AfterEach implementation.The cleanup logic properly deletes all BIOSVersion resources and ensures clean state. The comment accurately reflects the operations being performed.
181-193: LGTM - InProgress deletion test is correct.The test properly validates that deletion fails when the BIOSVersion is in InProgress state, and correctly resets to Completed state to allow AfterEach cleanup to succeed.
internal/webhook/v1alpha1/biosversion_webhook.go (5)
25-25: LGTM: Improved logger naming.The rename to
versionLogis more concise and follows common Go conventions.
40-42: LGTM: Excellent use of embedded client.The embedded
client.Clientfield is a cleaner pattern that allows direct method calls (v.List) instead ofv.Client.List, improving code readability throughout the validator.
47-59: LGTM: Clean variable naming and consistent refactoring.The changes improve readability with more concise variable names (
version,versions) and work correctly with the embedded client pattern.
62-89: LGTM: Clearer variable naming for update validation.The
newVersionandoldVersionnaming is more explicit and improves code clarity. The validation logic correctly handles in-progress state checks and duplicate detection.
91-103: LGTM: Consistent refactoring with previous fix applied.The variable renaming is consistent with the rest of the file, and the grammar fix from the previous review has been correctly applied.
afritzler
force-pushed
the
enh/biosvers-webhook
branch
from
ceda80d to
2b7ed78
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Proposed Changes
BIOSVersionwebhookSummary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.