operator: enable lint and fix lint issues #182
Conversation
|
Warning Rate limit exceeded@mangelajo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds lint-operator CI job for deploy/operator directory, removes logging parameter from configMapNeedsUpdate for consistency, refactors service creation logic into dedicated helper methods, updates e2e tests from Endpoints to EndpointSlices API, and simplifies serviceAccountToken signature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (1)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
170-189: Consider early return when both Ingress and Route are disabled.The function could return early if both Ingress and Route are disabled, avoiding unnecessary processing.
Apply this diff:
func (r *Reconciler) createIngressAndRouteForController(ctx context.Context, owner metav1.Object, endpoint *operatorv1alpha1.Endpoint, servicePort corev1.ServicePort, baseLabels map[string]string) error { + // Early return if both are disabled + if (endpoint.Ingress == nil || !endpoint.Ingress.Enabled) && + (endpoint.Route == nil || !endpoint.Route.Enabled) { + return nil + } + // Ingress resource (uses ClusterIP service) if endpoint.Ingress != nil && endpoint.Ingress.Enabled {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/lint.yaml(1 hunks)deploy/operator/internal/controller/jumpstarter/compare.go(1 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go(7 hunks)deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go(1 hunks)deploy/operator/test/e2e/e2e_test.go(9 hunks)deploy/operator/test/e2e/utils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.godeploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter-controller#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.
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
🧬 Code graph analysis (1)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (2)
deploy/operator/api/v1alpha1/jumpstarter_types.go (1)
Endpoint(380-414)deploy/helm/jumpstarter/charts/jumpstarter-controller/model.py (2)
Ingress(74-82)Route(85-90)
🪛 actionlint (1.7.8)
.github/workflows/lint.yaml
50-50: the runner of "actions/setup-go@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (8)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: lint-operator
- GitHub Check: lint-go
- GitHub Check: deploy-kind
- GitHub Check: e2e-test-operator
- GitHub Check: tests
🔇 Additional comments (9)
deploy/operator/test/e2e/utils.go (1)
25-26: LGTM! Appropriate use of nolint for test convenience.The
nolint:staticcheckdirectives correctly suppress warnings for dot imports, which are conventional and widely accepted in Ginkgo/Gomega test suites for improved readability.deploy/operator/test/e2e/e2e_test.go (4)
110-110: LGTM! Appropriate use of nolint for cleanup code.Suppressing error checking on
Close()for log streams in test cleanup paths is reasonable, as these errors are not actionable and the comment clearly explains the rationale.
247-265: LGTM! EndpointSlice migration properly implemented.The migration from the deprecated Endpoints API to EndpointSlice is correctly implemented with proper nil-pointer checks on
port.Port.
601-625: Verify error handling removal in serviceAccountToken().The function signature changed from
(string, error)tostring, removing explicit error handling from the return. The error handling is now implicit within theEventuallyblock at line 623, which will retry until success or timeout.While this approach works for tests (relying on Gomega's eventual consistency), ensure this pattern is intentional and that timeout failures provide clear diagnostics.
125-125: I need to verify the deprecation status ofLastTimestampin the Kubernetes API to assess the validity of this review comment.No changes needed; LastTimestamp is appropriate for this test code.
While
LastTimestampis deprecated in favor ofseries.lastObservedTimefor newevents.k8s.io/v1Events, the code uses the core/v1 Event API whereLastTimestampremains a valid, populated field. In the newer events API,deprecatedLastTimestampmaintains backward compatibility with core.v1 Event type. This test code is simply formatting event output for debugging purposes, where using the available field is reasonable and won't cause issues.deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (2)
142-168: LGTM! Clean refactoring into modular helper methods.The refactoring successfully extracts service creation logic into dedicated helper methods, improving code organization and maintainability. Each helper has a single, clear responsibility.
262-288: LGTM! Consistent refactoring applied to router endpoints.The router endpoint reconciliation follows the same clean modular pattern as the controller endpoints, maintaining consistency across the codebase.
deploy/operator/internal/controller/jumpstarter/compare.go (1)
47-47: LGTM! Removed unused logger parameter.Removing the unused
logparameter simplifies the function signature and aligns it with other comparison functions in this file that also don't require logging.deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go (1)
425-425: LGTM! Call site updated to match new signature.The function call correctly reflects the updated
configMapNeedsUpdatesignature without the logger parameter.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Refactor
Tests
Chores