Conversation
hardikdr
force-pushed
the
enh/improve-bootserver-tests
branch
from
79263df to
9699701
Compare
hardikdr
force-pushed
the
enh/improve-bootserver-tests
branch
2 times, most recently
from
397c1ba to
20f7de9
Compare
hardikdr
force-pushed
the
enh/improve-bootserver-tests
branch
from
20f7de9 to
716d059
Compare
WalkthroughIntroduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/bootserver_suit_test.go`:
- Around line 40-47: The fake k8s client created with fake.NewClientBuilder() is
missing the field indexer for NetworkIdentifierIndexKey, so List calls using
client.MatchingFields{bootv1alpha1.NetworkIdentifierIndexKey: ip} in
handleHTTPBoot won't match; fix by configuring the fake builder with
WithIndex(bootv1alpha1.NetworkIdentifierIndexKey, func(obj client.Object)
[]string { /* return the field value used by the real indexer, e.g., extract
NetworkIdentifier from BootConfig/httpBootConfig */ }) before Build(), so the
k8sClient used by RunBootServer has the same field index behavior as the real
client.
In `@server/bootserver_test.go`:
- Around line 99-105: The call to k8sClient.Create(context.Background(), secret)
ignores its returned error; update the test to capture the error (e.g. err :=
k8sClient.Create(...)) and fail the test if creation fails (use
t.Fatalf/t.Fatalf or require.NoError(t, err)) so the Secret named "bad-type" was
actually created before continuing; ensure you reference the same secret
variable and context.Background() call when checking the error.
🧹 Nitpick comments (2)
server/bootserver_suit_test.go (1)
20-28: Consider using dynamic port allocation and adding cleanup.The hardcoded port
:30003could cause test failures if the port is already in use. Consider using:0to let the OS assign an available port, then retrieve the actual address. Additionally, there's noAfterSuiteto gracefully shut down the server.server/bootserver_test.go (1)
46-48: Simplify redundant assertion.
SatisfyAny(BeEmpty(), Equal(""))is redundant for strings sinceBeEmpty()already matches an empty string. Consider simplifying to justBeEmpty().♻️ Proposed fix
By("not setting a SystemUUID in the default case") - Expect(body.SystemUUID).To(SatisfyAny(BeEmpty(), Equal(""))) + Expect(body.SystemUUID).To(BeEmpty())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/v1alpha1/constants.gocmd/main.goserver/bootserver.goserver/bootserver_suit_test.goserver/bootserver_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/main.go (1)
api/v1alpha1/constants.go (1)
NetworkIdentifierIndexKey(11-11)
server/bootserver_suit_test.go (2)
api/v1alpha1/groupversion_info.go (1)
AddToScheme(22-22)server/bootserver.go (1)
RunBootServer(51-95)
server/bootserver_test.go (2)
api/v1alpha1/ipxebootconfig_types.go (1)
IPXEBootConfig(73-79)server/bootserver.go (1)
SetStatusCondition(446-474)
server/bootserver.go (1)
api/v1alpha1/constants.go (1)
NetworkIdentifierIndexKey(11-11)
⏰ 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). (6)
- GitHub Check: lint
- GitHub Check: Run on Ubuntu
- GitHub Check: Test
- GitHub Check: Run on Ubuntu
- GitHub Check: Run on Ubuntu
- GitHub Check: Run on Ubuntu
🔇 Additional comments (6)
api/v1alpha1/constants.go (1)
7-13: LGTM!The new
NetworkIdentifierIndexKeyconstant follows the established naming conventions and provides a clear, documented index key for network identifiers (IP addresses and MAC addresses). The aligned comments maintain good readability.server/bootserver.go (1)
394-405: LGTM!The update from
SystemIPIndexKeytoNetworkIdentifierIndexKeyaligns with the new constant and the updated indexer incmd/main.go. The field indexer forNetworkIdentifierIndexKeyis properly configured viaIndexHTTPBootConfigByNetworkIDs.cmd/main.go (1)
353-363: LGTM!The indexer correctly uses the new
NetworkIdentifierIndexKeyconstant, and the returned values (HTTPBootConfig.Spec.NetworkIdentifiers) properly align with the indexed field path.server/bootserver_test.go (3)
25-49: Good test coverage for the default httpboot behavior.The test correctly validates the default response structure, including status code, content type, and the expected default UKI URL when no
HTTPBootConfigmatches.
51-71: Good coverage forrenderIgnitionfunction.The tests effectively cover both the happy path (valid Butane YAML conversion) and the error path (invalid YAML handling).
73-117: Good error path coverage forSetStatusCondition.The tests properly validate error handling for unknown condition types and unsupported resource types, verifying the expected error messages.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| k8sClient = fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| Build() | ||
|
|
||
| go func() { | ||
| defer GinkgoRecover() | ||
| RunBootServer(testServerAddr, ipxeServiceURL, k8sClient, logr.Discard(), defaultUKIURL) | ||
| }() |
There was a problem hiding this comment.
Fake client lacks field indexer for NetworkIdentifierIndexKey.
The fake client built with fake.NewClientBuilder() doesn't have field indexers configured. When handleHTTPBoot calls k8sClient.List(ctx, &httpBootConfigs, client.MatchingFields{bootv1alpha1.NetworkIdentifierIndexKey: ip}), the index lookup will not work as expected.
You need to configure the fake client with the index using WithIndex:
🔧 Proposed fix
k8sClient = fake.NewClientBuilder().
WithScheme(scheme).
+ WithIndex(&bootv1alpha1.HTTPBootConfig{}, bootv1alpha1.NetworkIdentifierIndexKey, func(obj client.Object) []string {
+ config := obj.(*bootv1alpha1.HTTPBootConfig)
+ return config.Spec.NetworkIdentifiers
+ }).
Build()📝 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.
| k8sClient = fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| Build() | |
| go func() { | |
| defer GinkgoRecover() | |
| RunBootServer(testServerAddr, ipxeServiceURL, k8sClient, logr.Discard(), defaultUKIURL) | |
| }() | |
| k8sClient = fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithIndex(&bootv1alpha1.HTTPBootConfig{}, bootv1alpha1.NetworkIdentifierIndexKey, func(obj client.Object) []string { | |
| config := obj.(*bootv1alpha1.HTTPBootConfig) | |
| return config.Spec.NetworkIdentifiers | |
| }). | |
| Build() |
🤖 Prompt for AI Agents
In `@server/bootserver_suit_test.go` around lines 40 - 47, The fake k8s client
created with fake.NewClientBuilder() is missing the field indexer for
NetworkIdentifierIndexKey, so List calls using
client.MatchingFields{bootv1alpha1.NetworkIdentifierIndexKey: ip} in
handleHTTPBoot won't match; fix by configuring the fake builder with
WithIndex(bootv1alpha1.NetworkIdentifierIndexKey, func(obj client.Object)
[]string { /* return the field value used by the real indexer, e.g., extract
NetworkIdentifier from BootConfig/httpBootConfig */ }) before Build(), so the
k8sClient used by RunBootServer has the same field index behavior as the real
client.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Proposed Changes
Fixes #
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.