Open
Conversation
hardikdr
force-pushed
the
enh/improve-bootserver-tests
branch
from
79263df to
9699701
Compare
hardikdr
force-pushed
the
enh/improve-bootserver-tests
branch
3 times, most recently
from
20f7de9 to
716d059
Compare
afritzler
requested changes
Dec 18, 2025
Contributor
WalkthroughAdded 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
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/bootserver_suit_test.go`:
- Around line 21-22: Replace the hardcoded testServerAddr and testServerURL with
an ephemeral bind: create a listener (e.g., net.Listen("tcp", ":0")) in the test
setup, capture the actual bound address via listener.Addr().String(), assign
that to testServerAddr and build testServerURL from it before starting the
server goroutine (or use httptest.NewServer to get an ephemeral URL), and ensure
the server uses the provided listener so the bind cannot fail due to port
collision; update any places referencing testServerAddr/testServerURL
accordingly.
- Around line 48-51: The anonymous polling function passed to Eventually uses
http.Get but never closes the returned *http.Response, leaking resources; modify
that closure (the func used in Eventually) to capture the response with resp,
err := http.Get(...), and when resp is non-nil ensure resp.Body.Close() is
called before returning (e.g., close immediately after error check or via a
short-lived defer), and return err as before so the reachability check still
works; update the closure in bootserver_suit_test.go accordingly.
- Line 10: Remove the unused import "github.com/go-logr/logr" (or reference the
declared logr variable) and start the boot server in a background goroutine
before the Eventually check: spawn a goroutine that invokes the Boot Server
start/serve method (e.g. bootServer.Serve/Start or whatever method
constructs/starts the HTTP server) using testServerAddr, ipxeServiceURL,
k8sClient and defaultUKIURL (the variables already declared) so the server is
listening while Eventually polls; ensure any returned error from the start call
is logged or sent to a channel for test failure reporting.
---
Duplicate comments:
In `@server/bootserver_suit_test.go`:
- Around line 40-46: No change required: the field indexer for
NetworkIdentifierIndexKey has been correctly registered on HTTPBootConfig by
indexing Spec.NetworkIdentifiers in the fake client builder; ensure the
WithIndex call in the test (that registers
bootv1alpha1.NetworkIdentifierIndexKey and uses func(obj client.Object) []string
{ config := obj.(*bootv1alpha1.HTTPBootConfig); return
config.Spec.NetworkIdentifiers }) remains unchanged so client.MatchingFields
lookups in handleHTTPBoot continue to work against the fake k8s client.
hardikdr
force-pushed
the
enh/improve-bootserver-tests
branch
from
9471d8b to
0116a0b
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/bootserver_test.go (1)
46-47: Redundant assertion:BeEmpty()andEqual("")are equivalent for strings.
SatisfyAny(BeEmpty(), Equal(""))can be simplified to justBeEmpty().- Expect(body.SystemUUID).To(SatisfyAny(BeEmpty(), Equal(""))) + Expect(body.SystemUUID).To(BeEmpty())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/bootserver_test.go` around lines 46 - 47, The assertion in the test uses a redundant SatisfyAny(BeEmpty(), Equal("")) for body.SystemUUID; replace that expectation with a single BeEmpty() matcher in the test (look for the Expect(body.SystemUUID).To(...) line in bootserver_test.go) to simplify and remove the duplicate Equal("") branch.server/bootserver.go (1)
51-97:http.DefaultServeMuxusage limits testability and safety.
RunBootServerregisters routes onhttp.DefaultServeMuxviahttp.HandleFunc. This means:
- Routes accumulate globally — calling
RunBootServertwice (e.g., in parallel tests) silently double-registers handlers.- Other code in the process that also uses
http.DefaultServeMuxcan interfere.Consider accepting or creating a dedicated
*http.ServeMuxand passing it tohttp.ListenAndServe. This also makes it easier to write isolated tests withhttptest.NewServer(mux).♻️ Sketch
-func RunBootServer(ipxeServerAddr string, ipxeServiceURL string, k8sClient client.Client, log logr.Logger, defaultUKIURL string) error { - http.HandleFunc("/ipxe/", func(w http.ResponseWriter, r *http.Request) { +func RunBootServer(ipxeServerAddr string, ipxeServiceURL string, k8sClient client.Client, log logr.Logger, defaultUKIURL string) error { + mux := http.NewServeMux() + mux.HandleFunc("/ipxe/", func(w http.ResponseWriter, r *http.Request) { handleIPXE(w, r, k8sClient, log, ipxeServiceURL) }) - http.HandleFunc("/httpboot", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/httpboot", func(w http.ResponseWriter, r *http.Request) { handleHTTPBoot(w, r, k8sClient, log, defaultUKIURL) }) - http.HandleFunc("/ignition/", func(w http.ResponseWriter, r *http.Request) { + mux.HandleFunc("/ignition/", func(w http.ResponseWriter, r *http.Request) { // ... }) log.Info("Starting boot server", "address", ipxeServerAddr) - if err := http.ListenAndServe(ipxeServerAddr, nil); err != nil { + if err := http.ListenAndServe(ipxeServerAddr, mux); err != nil { log.Error(err, "failed to start boot server") return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/bootserver.go` around lines 51 - 97, RunBootServer currently registers handlers on the global http.DefaultServeMux via http.HandleFunc which makes tests flaky and handlers leak; change RunBootServer to accept a *http.ServeMux (or create a new mux inside when nil) and replace all http.HandleFunc calls with mux.HandleFunc, then pass that mux to http.ListenAndServe (or use it in httptest.NewServer in tests); update callers to provide a mux or rely on the function creating one when a nil argument is passed so tests can supply isolated httptest servers and production code uses its own dedicated ServeMux.cmd/main.go (1)
306-311: Consider usingmgr.Addinstead of a bare goroutine + panic for the boot server.The controller-runtime
Managersupports adding customRunnables viamgr.Add(manager.RunnableFunc(...)). This integrates the boot server lifecycle with the manager's shutdown/health signals, so a boot-server failure triggers a graceful manager shutdown rather than an abruptpaniccrash with no cleanup. This provides better lifecycle consistency and error handling.♻️ Example refactor
mgr.Add(manager.RunnableFunc(func(ctx context.Context) error { return bootserver.RunBootServer(bootserverAddr, ipxeServiceURL, mgr.GetClient(), serverLog.WithName("bootserver"), *defaultHttpUKIURL) }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main.go` around lines 306 - 311, Replace the bare goroutine + panic pattern that starts the boot server with adding it to the controller-runtime Manager so its lifecycle and errors are handled gracefully: remove the go func that calls bootserver.RunBootServer and instead register a manager.Runnable (use manager.RunnableFunc) with mgr.Add that calls bootserver.RunBootServer and returns its error; this ensures bootserver failures propagate as returned errors to the manager (mgr) for graceful shutdown rather than causing a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/bootserver_suit_test.go`:
- Around line 22-23: Replace the hardcoded testServerAddr=":30003" and
testServerURL="http://localhost:30003" by creating a net.Listener on ":0" to get
an ephemeral port, then set testServerAddr from the listener.Addr().String() and
build testServerURL from that actual bound address; start the server with
http.Serve or (&http.Server{}).Serve(listener) instead of ListenAndServe, and
update any test helpers that read testServerAddr/testServerURL so Eventually and
Listen failures use the real ephemeral port.
- Around line 41-43: The fake k8s client used in the test doesn't register the
NetworkIdentifierIndexKey indexer, so handleHTTPBoot's
k8sClient.List(client.MatchingFields{NetworkIdentifierIndexKey: ip}) errors out
and the test follows the error path; fix by registering the same field indexer
on the test fake client used to build k8sClient (i.e., extend the
fake.NewClientBuilder() setup) with the identical index function used in
production for HTTPBootConfig so that NetworkIdentifierIndexKey resolves
correctly and List(...) can find matching HTTPBootConfig objects.
In `@server/bootserver_test.go`:
- Around line 98-105: The Secret creation call currently ignores the returned
error; change the `_ = k8sClient.Create(context.Background(), secret)` to
capture and assert the result (e.g., `err :=
k8sClient.Create(context.Background(), secret)` followed by a test assertion
like `Expect(err).NotTo(HaveOccurred())` or `require.NoError(t, err)`) so
failures surface and the test cannot pass due to a silent create error; update
the code around the secret variable and the k8sClient.Create call accordingly.
---
Nitpick comments:
In `@cmd/main.go`:
- Around line 306-311: Replace the bare goroutine + panic pattern that starts
the boot server with adding it to the controller-runtime Manager so its
lifecycle and errors are handled gracefully: remove the go func that calls
bootserver.RunBootServer and instead register a manager.Runnable (use
manager.RunnableFunc) with mgr.Add that calls bootserver.RunBootServer and
returns its error; this ensures bootserver failures propagate as returned errors
to the manager (mgr) for graceful shutdown rather than causing a panic.
In `@server/bootserver_test.go`:
- Around line 46-47: The assertion in the test uses a redundant
SatisfyAny(BeEmpty(), Equal("")) for body.SystemUUID; replace that expectation
with a single BeEmpty() matcher in the test (look for the
Expect(body.SystemUUID).To(...) line in bootserver_test.go) to simplify and
remove the duplicate Equal("") branch.
In `@server/bootserver.go`:
- Around line 51-97: RunBootServer currently registers handlers on the global
http.DefaultServeMux via http.HandleFunc which makes tests flaky and handlers
leak; change RunBootServer to accept a *http.ServeMux (or create a new mux
inside when nil) and replace all http.HandleFunc calls with mux.HandleFunc, then
pass that mux to http.ListenAndServe (or use it in httptest.NewServer in tests);
update callers to provide a mux or rely on the function creating one when a nil
argument is passed so tests can supply isolated httptest servers and production
code uses its own dedicated ServeMux.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
Refactor
Bug Fixes