-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support GroupMembers API Pagination for Client #5533
Conversation
40548ad
to
82bf6f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we extend this to ClusterGroupMemberExpansion as well?
pkg/client/clientset/versioned/typed/controlplane/v1beta2/groupmembers_expansion.go
Outdated
Show resolved
Hide resolved
// IP is the IP address of the Endpoints associated with the GroupMember. | ||
IPs []IPAddress | ||
// Ports is the list NamedPort of the GroupMember. | ||
Ports []NamedPort | ||
// Node maintains the reference to the Node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change not related to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it not related, just found the order was not consistent with type in v1beta2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
|
||
func convertToV1beta2Members(members []controlplane.GroupMember) []v1beta2.GroupMember { | ||
var result []v1beta2.GroupMember | ||
for _, item := range members { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Schema.Convert
to do the conversion for code reuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to conversion function in v1beta2
.
|
||
func convertToControlplaneMembers(members []v1beta2.GroupMember) []controlplane.GroupMember { | ||
var result []controlplane.GroupMember | ||
for _, item := range members { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -489,17 +489,7 @@ func RegisterConversions(s *runtime.Scheme) error { | |||
|
|||
func autoConvert_v1beta2_AddressGroup_To_controlplane_AddressGroup(in *AddressGroup, out *controlplane.AddressGroup, s conversion.Scope) error { | |||
out.ObjectMeta = in.ObjectMeta | |||
if in.GroupMembers != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this caused by the move of Node *NodeReference
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, it's generated code, surprised to see this shortened conversion.
29357d7
to
09ee5e6
Compare
Follow up for Group membership API to provide paginated Get function for client. Signed-off-by: Qiyue Yao <yaoq@vmware.com>
09ee5e6
to
84dc488
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
GroupMembers API was introduced in #5380 , pagination was supported for proxy server, but not for client. There are usecases for client-go pagination. This solution utilizes the
GroupMembersExpansion
to provide a new methodGetPaginated
.