🌱update endpoint of endpointsExposure in serverConfiguration #404
Conversation
WalkthroughCRD OpenAPI schema and Go API types were updated to add two new endpoint exposure types ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-02T11:59:50.703ZApplied to files:
🧬 Code graph analysis (1)operator/v1/zz_generated.deepcopy.go (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). (2)
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
🧹 Nitpick comments (1)
operator/v1/types_clustermanager.go (1)
205-208: Consider the naming mismatch between field name and usage.The
hostnamefield (type*HostnameConfig) is now used forloadBalancerandrouteendpoint types, not justhostname. While renaming would be a breaking change, consider documenting this broader usage more explicitly or planning a future API version that uses a more generic field name likeconfigorendpointConfig.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(4 hunks)operator/v1/types_clustermanager.go(1 hunks)
⏰ 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). (1)
- GitHub Check: verify
🔇 Additional comments (3)
operator/v1/types_clustermanager.go (1)
228-234: LGTM!The new
EndpointTypeLoadBalancerandEndpointTypeRouteconstants are well-defined and follow the existing naming conventions.operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (2)
597-628: HTTPS endpoint configuration is consistent with gRPC.The HTTPS endpoint schema correctly mirrors the gRPC endpoint structure with the same field definitions, descriptions, and enum values. This consistency is good for API usability.
565-596: Verified: CRD schema correctly reflects Go types but validation ambiguity confirmed.The review comment is accurate. Verification confirms:
- CRD consistency: The schema matches the Go struct definitions in
operator/v1/types_clustermanager.go- Validation ambiguity confirmed: The
Hostfield inHostnameConfigstruct is marked+optional(line 211), yet its description states "Host must be set if the type is hostname" (line 218-219)- No enforcement found: No CEL validation rules or webhook validators were found to enforce the conditional requirement at runtime
The API will accept requests where
type=hostnamebuthostis empty, creating potential runtime errors. This inherited issue from the Go types has been accurately captured in the CRD schema without resolution.
zhiweiyin318
force-pushed
the
grpc-config
branch
from
ca11ed9 to
3356518
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
operator/v1/types_clustermanager.go (1)
210-220: Consider renamingHostnameConfigto better reflect its broader scope.The struct name
HostnameConfigis now misleading since this configuration applies to all endpoint types (hostname, loadBalancer, route), not just hostname. A more accurate name likeEndpointConfigorEndpointHostConfigwould better convey its purpose across all exposure types.While the Host field description has been improved with softer language ("should be set" instead of "must be set"), the struct name still suggests hostname-specific usage, which may confuse API users.
Based on learnings (this concern was previously raised in past reviews).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml(4 hunks)operator/v1/types_clustermanager.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:59:50.703Z
Learnt from: qiujian16
Repo: open-cluster-management-io/api PR: 392
File: operator/v1/types_clustermanager.go:178-195
Timestamp: 2025-09-02T11:59:50.703Z
Learning: XValidation markers in kubebuilder require Kubernetes 1.25+ and should not be added to the open-cluster-management-io/api project until minimum Kubernetes version requirements are established to avoid breaking compatibility with older clusters.
Applied to files:
operator/v1/types_clustermanager.go
⏰ 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). (2)
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (3)
operator/v1/types_clustermanager.go (1)
229-234: LGTM!The new endpoint exposure type constants are well-defined with clear comments and consistent naming.
operator/v1/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml (2)
569-596: LGTM!The gRPC endpoint schema updates correctly reflect the Go type changes:
- Extended enum to include
loadBalancerandroute- Updated descriptions for
hostname,caBundle, andhostfields- Removed the
requiredconstraint on thehostfield, aligning with the+optionalmarker in the Go struct
600-628: LGTM!The HTTPS endpoint schema updates mirror the gRPC endpoint changes and maintain consistency across both endpoint types. The extended enum and updated field descriptions properly reflect the broadened endpoint exposure options.
zhiweiyin318
force-pushed
the
grpc-config
branch
from
3356518 to
c0b783f
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, zhiweiyin318 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Zhiwei Yin <[email protected]>
zhiweiyin318
force-pushed
the
grpc-config
branch
from
c0b783f to
e5fed78
Compare
|
/lgtm |
openshift-merge-bot
bot
merged commit 3e1bb92
into
open-cluster-management-io:main
3 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Documentation