operator: test coverage for ApplyEndpointDefaults #205
Conversation
WalkthroughAdds a new Go test file that validates ApplyEndpointDefaults and ensureEndpointServiceType behaviors, including baseDomain handling, controller/router/ingress/clusterIP selection, replica placeholder usage, and preservation of existing service types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults 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). (7)
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 |
evakhoni
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/operator/internal/controller/jumpstarter/endpoints/defaults_test.go (1)
70-77: Consider verifying the router endpoint's service type as well.The test correctly verifies the
$(replica)placeholder in the address, but based on the source code (defaults.go), the router endpoint should also receive a service type viaensureEndpointServiceType. Consider adding an assertion to verify that the router endpoint gets the expected service type (Route in this case).💡 Suggested enhancement
It("should generate router endpoint with $(replica) placeholder", func() { spec := &operatorv1alpha1.JumpstarterSpec{BaseDomain: "example.com"} ApplyEndpointDefaults(spec, true, true) Expect(spec.Routers.GRPC.Endpoints).To(HaveLen(1)) Expect(spec.Routers.GRPC.Endpoints[0].Address).To(Equal("router-$(replica).example.com")) + Expect(spec.Routers.GRPC.Endpoints[0].Route).NotTo(BeNil()) + Expect(spec.Routers.GRPC.Endpoints[0].Route.Enabled).To(BeTrue()) })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deploy/operator/internal/controller/jumpstarter/endpoints/defaults_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter project uses a custom kind cluster configuration with an expanded NodePort range (3000-32767) and explicit port mappings for ingress (5080/5443) and gRPC services (30010/30011 mapped to 8082/8083).
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter-dev repository uses a custom kind cluster configuration that allows NodePort services to use non-standard ports 5080 and 5443, outside the default Kubernetes NodePort range (30000-32767).
🧬 Code graph analysis (1)
deploy/operator/internal/controller/jumpstarter/endpoints/defaults_test.go (2)
deploy/operator/api/v1alpha1/jumpstarter_types.go (5)
JumpstarterSpec(130-156)ControllerConfig(195-234)GRPCConfig(248-264)ClusterIPConfig(503-515)NodePortConfig(458-477)deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go (1)
ApplyEndpointDefaults(51-85)
⏰ 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). (7)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-test-operator
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: tests
- GitHub Check: deploy-kind
- GitHub Check: lint-go
🔇 Additional comments (4)
deploy/operator/internal/controller/jumpstarter/endpoints/defaults_test.go (4)
1-24: LGTM! Clean test setup.The imports are appropriate and the use of Ginkgo/Gomega is idiomatic for this testing framework.
26-78: Well-structured tests with comprehensive coverage.The test cases properly cover:
- Empty baseDomain skip logic
- Service type selection hierarchy (Route → Ingress → ClusterIP)
- Router endpoint placeholder format
The tests are clear and verify the expected behavior accurately.
80-117: Excellent coverage of existing endpoint scenarios.These tests properly verify:
- Preservation of user-defined endpoints
- Automatic service type assignment for endpoints without one
The assertions correctly validate the expected behavior.
120-163: Comprehensive testing of service type selection logic.The test suite thoroughly covers:
- Preservation of existing service types (NodePort example)
- Automatic selection hierarchy: Route → Ingress → ClusterIP
- All three fallback scenarios
The tests accurately verify the expected behavior and edge cases.
|
@CodeRabbit please assign reviewers mangelajo bkhizgiy bennyz |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 252 @evakhoni I'm unable to assign reviewers programmatically due to workflow restrictions. You'll need to add the reviewers mangelajo, bkhizgiy, and bennyz manually through the GitHub UI:
Alternatively, if you have repository admin access, you can configure the necessary permissions to allow me to manage reviewers through the GitHub CLI. |
|
thanks @bennyz |
evakhoni
force-pushed
the
test-operator-endpoint-defaults
branch
from
64fb2d9 to
7373ba3
Compare
|
End-to-end tests / e2e-tests-release-0-7 (pull_request) failure seems unrelated: @coderabbitai how can we rerun this pipeline? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@evakhoni You're correct that the failure is unrelated to your changes—it's a network timeout while fetching an external Helm chart. You can rerun the workflow using the GitHub CLI: Alternatively, you can rerun it through the GitHub UI:
The 🏁 Script executed: Length of output: 71 @evakhoni I don't have the necessary permissions to rerun workflows programmatically. This is a GitHub API restriction—my integration token lacks the You have two options: Option 1: Use the GitHub UI (easiest)
Option 2: Use your own GitHub CLI gh run rerun 20463354452 --failedThis will work with your personal credentials. Since you mentioned earlier that you also lack certain repository permissions (like assigning reviewers), you may want to reach out to a repository maintainer (like @mangelajo or @bennyz) to either rerun the workflow or grant you the necessary permissions for managing PRs in this repository. |
indeed 🤷♂️ @bennyz to the rescue ;) |
3 participants
follow-up #202:
operator: sensible defaults to allow minimal configuration Jumpstarter resource creation.this PR is adding unit test coverage
Coverage
ApplyEndpointDefaultsfunction:baseDomainis empty$(replica)placeholderensureEndpointServiceTypehelper function:Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.