route: refactor route package to use common builder

[klaskosk]

This commit switches the route package to use the common EmbeddableBuilder as the basis for the Builder struct in the package. This provides eliminates the need to have specific methods for common operations like Get, Create, and Delete.

This package is a good example for future refactors, since it includes the EmbeddableBuilder, several mixins, specific methods which cannot be common functions, and a NewBuilder with special parameters.

In addition to the refactor to use the common package, this commit improves the unit tests even for functions which are not part of the common builder. Additional cases have been added to improve overall coverage.

Assisted-by: Cursor

Summary by CodeRabbit

• Refactor

  • Consolidated the Route builder by embedding shared generic mixins, removing duplicate CRUD/validation logic and standardizing error handling and builder wiring.

• Tests

  • Reworked Route builder tests to use the shared test suite and parallel subtests, improving consistency, clearer validation assertions, and maintainability.

[coderabbitai]

📝 Walkthrough

Walkthrough

Route builder refactored to embed common mixins and delegate CRUD/validation to shared implementations; tests converted to suite-based patterns and assertions updated to use builder error accessors. Route type handling was removed from mock client initialization, changing which objects are included in fake clients.

Changes

Cohort / File(s)	Summary
Mock Client Initialization pkg/clients/clients.go	Removed the case *routev1.Route branch in GetModifiableTestClients, so Route objects from tcp.K8sMockObjects are no longer appended to the runtime/dynamic fake client construction paths.
Route Builder Implementation pkg/route/route.go	Replaced explicit builder fields (Definition, Object, errorMsg, apiClient) with embedded generics: common.EmbeddableBuilder, common.EmbeddableCreator, common.EmbeddableDeleteReturner. Added AttachMixins() and GetGVK(). Constructor, Pull, mutators, and error handling now use common helpers and SetError/GetError. Removed local CRUD/validate implementations.
Route Builder Tests pkg/route/route_test.go	Rewrote tests to use testhelper suite utilities (NewNamespacedPullTestConfig, NewTestSuite), replaced direct errorMsg checks with GetError(), added helper builders, organized cases into parallel subtests, and added Test_Builder_Methods suite.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely captures the main refactoring effort: migrating the route package to use the common EmbeddableBuilder framework, which aligns with the primary changes shown in the file summaries.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (3)

pkg/route/route.go (3)

107-111: Prefer intstr.FromString() for constructing IntOrString values.

Same as with port numbers, use the constructor function for proper field initialization:

♻️ Suggested refactor

 	if builder.Definition.Spec.Port == nil {
 		builder.Definition.Spec.Port = new(routev1.RoutePort)
 	}

-	builder.Definition.Spec.Port.TargetPort = intstr.IntOrString{StrVal: portName}
+	builder.Definition.Spec.Port.TargetPort = intstr.FromString(portName)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/route/route.go` around lines 107 - 111, The code constructs an
intstr.IntOrString manually for builder.Definition.Spec.Port.TargetPort; replace
that manual construction with the helper intstr.FromString to ensure proper
initialization (i.e., set builder.Definition.Spec.Port.TargetPort =
intstr.FromString(portName)); keep the existing guard that initializes
builder.Definition.Spec.Port (routev1.RoutePort) if nil and then assign
TargetPort via intstr.FromString(portName).

---

62-68: Consider accepting context as a parameter.

Using context.TODO() works but prevents callers from controlling cancellation/timeouts. If other builders in this codebase follow the same pattern, this is acceptable for consistency. Otherwise, consider updating the function signature to accept a context parameter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/route/route.go` around lines 62 - 68, The Pull function currently uses
context.TODO(), preventing callers from controlling cancellation/timeouts;
change the Pull signature to accept a context.Context parameter (e.g., func
Pull(ctx context.Context, apiClient *clients.Settings, name, nsname string)
(*Builder, error)), replace context.TODO() with the incoming ctx when calling
common.PullNamespacedBuilder, and update all callers of route.Pull to pass their
existing context (or context.Background()/derived contexts where appropriate) so
cancellation and deadlines propagate correctly; ensure any references to Pull in
tests are updated to provide a context as well.

---

79-83: Use intstr.FromInt32() for constructing IntOrString values.

While intstr.IntOrString{IntVal: port} works functionally (the Type field is optional), the idiomatic approach is to use the constructor function which properly sets all fields:

♻️ Suggested refactor

 	if builder.Definition.Spec.Port == nil {
 		builder.Definition.Spec.Port = new(routev1.RoutePort)
 	}

-	builder.Definition.Spec.Port.TargetPort = intstr.IntOrString{IntVal: port}
+	builder.Definition.Spec.Port.TargetPort = intstr.FromInt32(port)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/route/route.go` around lines 79 - 83, The code sets TargetPort via a
struct literal which omits Type; update the assignment to use the idiomatic
constructor intstr.FromInt32 to ensure the IntOrString is properly initialized:
when manipulating builder.Definition.Spec.Port (type routev1.RoutePort) set
TargetPort with intstr.FromInt32(port) instead of intstr.IntOrString{IntVal:
port}; ensure the nil-check that creates builder.Definition.Spec.Port remains
unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/route/route.go`:
- Around line 107-111: The code constructs an intstr.IntOrString manually for
builder.Definition.Spec.Port.TargetPort; replace that manual construction with
the helper intstr.FromString to ensure proper initialization (i.e., set
builder.Definition.Spec.Port.TargetPort = intstr.FromString(portName)); keep the
existing guard that initializes builder.Definition.Spec.Port (routev1.RoutePort)
if nil and then assign TargetPort via intstr.FromString(portName).
- Around line 62-68: The Pull function currently uses context.TODO(), preventing
callers from controlling cancellation/timeouts; change the Pull signature to
accept a context.Context parameter (e.g., func Pull(ctx context.Context,
apiClient *clients.Settings, name, nsname string) (*Builder, error)), replace
context.TODO() with the incoming ctx when calling common.PullNamespacedBuilder,
and update all callers of route.Pull to pass their existing context (or
context.Background()/derived contexts where appropriate) so cancellation and
deadlines propagate correctly; ensure any references to Pull in tests are
updated to provide a context as well.
- Around line 79-83: The code sets TargetPort via a struct literal which omits
Type; update the assignment to use the idiomatic constructor intstr.FromInt32 to
ensure the IntOrString is properly initialized: when manipulating
builder.Definition.Spec.Port (type routev1.RoutePort) set TargetPort with
intstr.FromInt32(port) instead of intstr.IntOrString{IntVal: port}; ensure the
nil-check that creates builder.Definition.Spec.Port remains unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f8adeae9-ead7-4f59-85bb-b6fca1aeaf95

📥 Commits

Reviewing files that changed from the base of the PR and between 32569bc and ec7c64e.

📒 Files selected for processing (3)

• pkg/clients/clients.go
• pkg/route/route.go
• pkg/route/route_test.go

💤 Files with no reviewable changes (1)

• pkg/clients/clients.go

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/route/route.go`:
- Around line 72-74: Check and preserve the builder's existing sticky error
before mutating: at the start of each mutator (e.g., WithTargetPortNumber,
WithHostDomain, WithWildCardPolicy and any similar mutator using
common.Validate(builder)), early-return the builder if builder.GetError() != nil
to avoid further mutation; then when calling common.Validate(builder), if it
returns err, call builder.SetError(err) and return the builder (do not drop the
validation error). This ensures existing errors are not overwritten and
validation failures are persisted back onto the builder.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ed523f5c-b949-4c0f-b096-530d2cb2f3b4

📥 Commits

Reviewing files that changed from the base of the PR and between ec7c64e and 6ce6a32.

📒 Files selected for processing (3)

• pkg/clients/clients.go
• pkg/route/route.go
• pkg/route/route_test.go

💤 Files with no reviewable changes (1)

• pkg/clients/clients.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/route/route_test.go