Refactor `BIOSVersion` webhook #575

afritzler · 2025-12-18T12:34:08Z

Proposed Changes

Refactor BIOSVersion webhook
Updated tests

Summary by CodeRabbit

Refactor
- Consolidated test cleanup into centralized after-each hooks for more reliable teardown and cleaner test flows.
- Adjusted validator struct embedding and internal naming to streamline validation logic.
Tests
- Updated many webhook and controller tests to use centralized cleanup and ensure a consistently clean test state after each case.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- Refactor `BIOSVersion` webhook - Updated tests

afritzler · 2025-12-19T14:05:11Z

@coderabbitai review

coderabbitai · 2025-12-19T14:05:19Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-12-19T14:05:23Z

Walkthrough

Consolidates per-test DeferCleanup deletions into AfterEach cleanup hooks calling controller.EnsureCleanState(), and refactors BIOSVersionCustomValidator to embed client.Client (updating calls and validation helpers); tests updated to use GenerateName-based metadata and rely on centralized cleanup.

Changes

Cohort / File(s)	Summary
Test cleanup refactoring `internal/cmd/console/console_test.go`, `internal/webhook/v1alpha1/bmcsettings_webhook_test.go`, `internal/webhook/v1alpha1/bmcversion_webhook_test.go`, `internal/webhook/v1alpha1/endpoint_webhook_test.go`, `internal/webhook/v1alpha1/server_webhook_test.go`	Replace per-test `DeferCleanup` deletions with shared `AfterEach` hooks that delete all relevant resources and call `controller.EnsureCleanState()`. Add `controller` imports and remove individual deferred cleanup calls. Minor test flow adjustments where needed.
BIOS version webhook refactor `internal/webhook/v1alpha1/biosversion_webhook.go`, `internal/webhook/v1alpha1/biosversion_webhook_test.go`	`BIOSVersionCustomValidator` now embeds `client.Client` (use `v.List(...)` etc.). Rename logging var to `versionLog`, update internal variable names (e.g., `version`, `versions`), adjust validation/error paths (serverRef name path), change duplicate-check helper signature/logic, and update tests to use `GenerateName` and centralized cleanup.
Test helper list checks update `internal/controller/test_helper.go`	Expand and rename resource list checks: replace/rename several list types and variables (endpoints, bmcSecrets, claims, bmcVersionSets, bmcVersions, biosVersions, biosSettingsSets, biosSettingsList, maintenances, servers) to validate emptiness across a broader set of API types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pay attention to the embedded client.Client usage in biosversion_webhook.go (method call sites changed from v.Client.List → v.List).
Verify test cleanup semantics: AfterEach deletions + controller.EnsureCleanState() replace many DeferCleanup usages—ensure no test interdependence remains.
Review checkForDuplicateBIOSVersionRefToServer logic and updated error paths for correctness.

Poem

🐇 I hopped through tests at break of dawn,
Tossed DeferCleanup carrots on the lawn,
AfterEach broom in paw, I sweep and cheer,
EnsureCleanState hums — the cluster’s clear!
Embedded client snug in code, I smile and prance.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Description check	❓ Inconclusive	The PR description is minimal and lacks sufficient detail about the changes. While it mentions refactoring the BIOSVersion webhook and updating tests, it does not provide meaningful context about what was refactored, why, or what the impacts are.	Expand the description to explain the specific refactoring changes (e.g., struct field changes, cleanup approach), rationale, and impacts on tests and other affected files.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title "Refactor `BIOSVersion` webhook" is specific and directly reflects the main change in the PR, which focuses on refactoring the BIOSVersion webhook implementation.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch enh/biosvers-webhook

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 29649ae and a21bb3e.

📒 Files selected for processing (3)

internal/controller/test_helper.go (1 hunks)
internal/webhook/v1alpha1/biosversion_webhook.go (2 hunks)
internal/webhook/v1alpha1/biosversion_webhook_test.go (2 hunks)

🧰 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 (9)

internal/webhook/v1alpha1/biosversion_webhook_test.go (1)

7-193: LGTM! Test refactoring is well-structured.

The test file has been successfully refactored to use centralized cleanup via AfterEach and controller.EnsureCleanState(). The tests comprehensively cover:

Duplicate ServerRef validation on create and update

Non-duplicate scenarios

InProgress state blocking with force-update override

Delete validation with InProgress state

The use of GenerateName and direct validator calls is appropriate for webhook testing.

internal/controller/test_helper.go (2)

40-41: Past review comment has been addressed.

The BIOSVersionList verification has been added to EnsureCleanState() as previously requested. This ensures that BIOSVersion resources are properly cleaned up during test execution.

19-53: LGTM! Comprehensive resource cleanup verification.

The EnsureCleanState() function now verifies that all relevant resource types are cleaned up, including the newly added BIOSVersionList check. The variable naming updates improve clarity while maintaining the same verification logic.

internal/webhook/v1alpha1/biosversion_webhook.go (6)

25-25: LGTM! Logger rename improves clarity.

Renaming the logger from biosversionlog to versionLog is a minor improvement that maintains consistency with the variable naming convention used throughout the refactored code.

40-42: LGTM! Embedding client.Client follows Go best practices.

Embedding client.Client instead of using a named field is an idiomatic Go pattern that allows calling methods like v.List() directly on the validator, improving code readability.

47-59: LGTM! ValidateCreate refactored correctly.

The refactoring maintains the same validation logic while improving variable naming (version instead of biosversion) and utilizing the embedded client pattern (v.List() instead of v.Client.List()).

62-89: LGTM! ValidateUpdate preserves critical validation logic.

The update validation correctly:

Blocks updates when in InProgress state (unless force-update annotation is present)

Requires ServerMaintenanceRef to be set for the InProgress check

Validates for duplicate ServerRef across all BIOSVersions

Variable naming improvements enhance readability without changing the logic.

92-103: LGTM! ValidateDelete improved with clearer error message.

The delete validation correctly blocks deletion when in InProgress state unless the force-delete annotation is present. The error message has been improved to be grammatically correct and clear.

105-129: LGTM! Duplicate check logic is robust.

The refactored checkForDuplicateBIOSVersionRefToServer function includes excellent defensive coding:

Early return if ServerRef is nil (prevents nil pointer dereferences)

Skips self-comparison by checking names

Properly handles nil ServerRef in existing resources

Constructs field error with correct path: spec.serverRef.name

The logic correctly identifies duplicate ServerRef values across different BIOSVersion resources.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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) to DeleteAllOf, 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 if server were 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1d36a58 and 29649ae.

📒 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 biosversionlog to versionLog follows Go naming conventions and improves readability.

40-42: LGTM: Idiomatic use of embedded client.

Embedding client.Client promotes its methods and enables cleaner method calls (v.List() vs v.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 promoted v.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 newVersion in 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 BMCSettings resources and verifies cleanup via controller.EnsureCleanState(), which includes BMCSettingsList verification.

internal/webhook/v1alpha1/bmcversion_webhook_test.go (1)

47-53: LGTM!

The AfterEach cleanup properly handles BMCVersion resource 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, and EnsureCleanState() 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 Completed before cleanup, ensuring AfterEach can succeed.

internal/webhook/v1alpha1/biosversion_webhook_test.go

internal/webhook/v1alpha1/biosversion_webhook.go

internal/webhook/v1alpha1/endpoint_webhook_test.go

afritzler · 2025-12-19T14:58:18Z

@coderabbitai generate docstrings

coderabbitai · 2025-12-19T14:58:27Z

✅ Actions performed

Initiated docstring generation; will generate only if new commits exist.

coderabbitai · 2025-12-19T14:58:29Z

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #577

@afritzler

Docstrings generation was requested by @afritzler. * #575 (comment) The following files were modified: * `internal/controller/test_helper.go` * `internal/webhook/v1alpha1/biosversion_webhook.go`

afritzler requested a review from a team as a code owner December 18, 2025 12:34

github-project-automation bot added this to Roadmap Dec 18, 2025

github-actions bot added enhancement New feature or request size/L labels Dec 18, 2025

afritzler force-pushed the enh/biosvers-webhook branch from 120e510 to 7d1a12b Compare December 18, 2025 12:36

Refactor BIOSVersion webhook

b5c7e20

- Refactor `BIOSVersion` webhook - Updated tests

afritzler force-pushed the enh/biosvers-webhook branch 3 times, most recently from 7c52de4 to f5b59a6 Compare December 18, 2025 17:00

Ensure cleanup in all webhook tests

29649ae

afritzler force-pushed the enh/biosvers-webhook branch from f5b59a6 to 29649ae Compare December 18, 2025 17:11

coderabbitai bot reviewed Dec 19, 2025

View reviewed changes

internal/webhook/v1alpha1/biosversion_webhook_test.go Show resolved Hide resolved

internal/webhook/v1alpha1/biosversion_webhook.go Outdated Show resolved Hide resolved

internal/webhook/v1alpha1/endpoint_webhook_test.go Show resolved Hide resolved

Address PR review comments

a21bb3e

github-actions bot added size/XL and removed size/L labels Dec 19, 2025

coderabbitai bot mentioned this pull request Dec 19, 2025

📝 Add docstrings to enh/biosvers-webhook #577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor `BIOSVersion` webhook #575

Refactor `BIOSVersion` webhook #575

Uh oh!

afritzler commented Dec 18, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

afritzler commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afritzler commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Refactor BIOSVersion webhook #575

Are you sure you want to change the base?

Refactor BIOSVersion webhook #575

Uh oh!

Conversation

afritzler commented Dec 18, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Summary by CodeRabbit

Uh oh!

afritzler commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afritzler commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025

Uh oh!

coderabbitai bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Refactor `BIOSVersion` webhook #575

Refactor `BIOSVersion` webhook #575

afritzler commented Dec 18, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 19, 2025 •

edited

Loading

coderabbitai bot commented Dec 19, 2025 •

edited

Loading