Implementing a more modular API #1627

ayrloong · 2025-04-28T11:46:35Z

This is the basic implementation based on this issue #1622

@tg123 Can you help me check if the overall structure is in compliance?

k8s-ci-robot · 2025-04-28T11:46:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ayrloong
Once this PR has been reviewed and has the lgtm label, please assign brendandburns for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull Request Overview

This PR implements a more modular API in response to the referenced issue by reorganizing and extending the client set generation functionality while also fixing a minor parameter naming typo.

Corrected the websocket protocol parameter typo in test and client code.
Introduced a new ClientSetGenerator to modularize client generation.
Adjusted access modifiers (from protected to internal) for key HTTP methods and classes.

Reviewed Changes

Copilot reviewed 10 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/KubernetesClient.Tests/PodExecTests.cs	Fixed a typo in the websocket protocol parameter name.
src/LibKubernetesGenerator/KubernetesClientSourceGenerator.cs	Minor formatting adjustments during container registrations.
src/LibKubernetesGenerator/ClientSetGenerator.cs	Added client set generation functionality with parameter renaming logic for duplicates.
src/KubernetesClient/Kubernetes.cs	Changed method access modifier from protected to internal.
src/KubernetesClient/Kubernetes.WebSocket.cs	Fixed parameter typo for the websocket protocol name.
src/KubernetesClient/GenericClient.cs	Introduced a minor format change with no functional impact.
src/KubernetesClient/ClientSets/ResourceClient.cs	Added a new abstract resource client.
src/KubernetesClient/ClientSets/ClientSet.cs	Introduced a new partial ClientSet class for client grouping.
src/KubernetesClient/AbstractKubernetes.cs	Updated access modifiers for core HTTP methods and inner classes.
examples/clientset/Program.cs	Added an example to illustrate how the new client set can be used.

Files not reviewed (7)

examples/clientset/clientset.csproj: Language not supported
src/KubernetesClient.Aot/KubernetesClient.Aot.csproj: Language not supported
src/KubernetesClient.Classic/KubernetesClient.Classic.csproj: Language not supported
src/LibKubernetesGenerator/templates/Client.cs.template: Language not supported
src/LibKubernetesGenerator/templates/ClientExtensions.cs.template: Language not supported
src/LibKubernetesGenerator/templates/ClientSet.cs.template: Language not supported
src/LibKubernetesGenerator/templates/GroupClient.cs.template: Language not supported

Comments suppressed due to low confidence (2)

src/LibKubernetesGenerator/ClientSetGenerator.cs:30

[nitpick] Consider renaming the variable 'i' to a more descriptive name (e.g. 'dupCounter') to improve code clarity when handling duplicate parameter names.

var i = 1;

tests/KubernetesClient.Tests/PodExecTests.cs:69

The parameter name 'webSocketSubProtocol' is now correctly spelled. Verify that all references and tests have been updated accordingly.

webSocketSubProtocol: WebSocketProtocol.ChannelWebSocketProtocol,

tg123

adding [deprecated] to older style?

tg123 · 2025-04-28T17:11:31Z

examples/clientset/Program.cs

+            IKubernetes client = new Kubernetes(config);
+
+            ClientSet clientSet = new ClientSet(client);
+            var list = await clientSet.CoreV1.Pod.ListNamespacedPodAsync("default").ConfigureAwait(false);


ListAsync?

i prefer the style in your proposal, it is simpler

I used x-kuberneter-action to define the method name. Can you help me see if there is any problem with this?https://github.com/kubernetes-client/csharp/pull/1627/files#diff-9501625d7da40bd1b40cb3b43406f26d00f5aa65c65f4877b1eaec2d40f792a0R169

src/KubernetesClient/Kubernetes.WebSocket.cs

src/LibKubernetesGenerator/KubernetesClientSourceGenerator.cs

src/LibKubernetesGenerator/templates/Client.cs.template

ayrloong · 2025-04-29T12:11:01Z

adding [deprecated] to older style?

I think it can be marked as obsolete/deprecated after a few versions

ayrloong · 2025-04-29T12:26:06Z

Currently, the WithHttpMessagesAsync method remains unimplemented. Implementing it at this stage could introduce excessive complexity to the entire class. Based on current observations, WithHttpMessagesAsync appears to be primarily utilized by the Watch operation.

I've been tracking the Kubernetes OpenAPI Swagger specifications in the GitHub repository and noticed the recent addition of a Watch endpoint. However, the official release timeline for this feature remains unclear. If the implementation is significantly delayed, we might need to manually implement a Watch method as a temporary workaround.

In the latest commit, I've introduced the Humanizer library to handle pluralization transformations. Given that it appears to provide duplicate functionality with CaseExtensions, I recommend removing CaseExtensions to maintain a single source of truth for string manipulation.

https://raw.githubusercontent.com/kubernetes/kubernetes/refs/heads/master/api/openapi-spec/swagger.json

tg123 · 2025-04-30T09:47:56Z

src/KubernetesClient/ClientSets/ResourceClient.cs

+
+    public ResourceClient(IKubernetes kubernetes)
+    {
+        Client = (Kubernetes)kubernetes;


did not quite get here,
anyway to avoid cast, why not force input Kubernetes?

Given that IKubernetes is typically registered in the DI container, I relied on dependency injection instead of manually providing Kubernetes

tg123

cloud you please also cleanup new style warnings?
i will take a deeper look at large files, please allow me more time to review

thanks

Copilot

Pull Request Overview

This PR implements a more modular API by updating naming consistency, adding new source generator components, and introducing new client set implementations along with corresponding tests and examples.

Corrects inconsistent parameter naming (e.g., webSocketSubProtol to webSocketSubProtocol)
Registers and implements a new ClientSetGenerator to modularize client creation
Adds new client set classes and an example demonstrating usage

Reviewed Changes

Copilot reviewed 8 out of 16 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/KubernetesClient.Tests/PodExecTests.cs	Fixes a typo in a parameter name for consistency in websocket protocol usage.
src/LibKubernetesGenerator/KubernetesClientSourceGenerator.cs	Registers the new ClientSetGenerator for source generation.
src/LibKubernetesGenerator/GeneralNameHelper.cs	Introduces GetActionName and updates script imports accordingly.
src/LibKubernetesGenerator/ClientSetGenerator.cs	Adds a new generator class for client sets with parameter deduplication logic; consider clarifying variable naming.
src/KubernetesClient/Kubernetes.WebSocket.cs	Updates parameter naming to ensure consistency with naming conventions.
src/KubernetesClient/ClientSets/ResourceClient.cs	Adds an abstract base class for resource clients.
src/KubernetesClient/ClientSets/ClientSet.cs	Adds a partial class for client set management.
examples/clientset/Program.cs	Provides an example to demonstrate the usage of the new client set API.

Files not reviewed (8)

Directory.Packages.props: Language not supported
examples/clientset/clientset.csproj: Language not supported
src/KubernetesClient.Aot/KubernetesClient.Aot.csproj: Language not supported
src/KubernetesClient.Classic/KubernetesClient.Classic.csproj: Language not supported
src/LibKubernetesGenerator/LibKubernetesGenerator.target: Language not supported
src/LibKubernetesGenerator/templates/Client.cs.template: Language not supported
src/LibKubernetesGenerator/templates/ClientSet.cs.template: Language not supported
src/LibKubernetesGenerator/templates/GroupClient.cs.template: Language not supported

Comments suppressed due to low confidence (1)

src/LibKubernetesGenerator/ClientSetGenerator.cs:29

[nitpick] The variable name 'name' is ambiguous in this context as it represents a collection of parameter names. Consider renaming it to 'seenParameterNames' or a similarly descriptive identifier.

var name = new HashSet<string>();

ayrloong · 2025-05-01T05:33:48Z

cloud you please also cleanup new style warnings? i will take a deeper look at large files, please allow me more time to review

thanks

I'm on vacation this week so might not get to these changes right away. I'll go through them all once I'm back next week. Really appreciate you taking the time to review!

ayrloong added 4 commits April 18, 2025 17:44

fix typo

128bc43

Merge branch 'kubernetes-client:master' into master

f021a01

Merge branch 'kubernetes-client:master' into master

19cbf1d

Add modular API

2cf1a53

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2025

k8s-ci-robot requested review from brendandburns and tg123 April 28, 2025 11:46

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 28, 2025

tg123 requested a review from Copilot April 28, 2025 17:07

Copilot AI reviewed Apr 28, 2025

View reviewed changes

tg123 reviewed Apr 28, 2025

View reviewed changes

Use x-kubernetes-action instead of operationId to generate method names

0bd3dad

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 29, 2025

ayrloong marked this pull request as ready for review April 29, 2025 10:40

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2025

k8s-ci-robot requested a review from tg123 April 29, 2025 10:40

fix

7a3f951

tg123 reviewed Apr 30, 2025

View reviewed changes

tg123 requested a review from Copilot April 30, 2025 09:50

Copilot AI reviewed Apr 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing a more modular API #1627

Implementing a more modular API #1627

ayrloong commented Apr 28, 2025

k8s-ci-robot commented Apr 28, 2025

Copilot AI left a comment

tg123 left a comment

tg123 Apr 28, 2025

ayrloong Apr 29, 2025 •

edited

Loading

ayrloong commented Apr 29, 2025 •

edited

Loading

ayrloong commented Apr 29, 2025 •

edited

Loading

tg123 Apr 30, 2025

ayrloong May 1, 2025

tg123 left a comment

Copilot AI left a comment

ayrloong commented May 1, 2025

Implementing a more modular API #1627

Are you sure you want to change the base?

Implementing a more modular API #1627

Conversation

ayrloong commented Apr 28, 2025

k8s-ci-robot commented Apr 28, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

tg123 left a comment

Choose a reason for hiding this comment

tg123 Apr 28, 2025

Choose a reason for hiding this comment

ayrloong Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

ayrloong commented Apr 29, 2025 • edited Loading

ayrloong commented Apr 29, 2025 • edited Loading

tg123 Apr 30, 2025

Choose a reason for hiding this comment

ayrloong May 1, 2025

Choose a reason for hiding this comment

tg123 left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

ayrloong commented May 1, 2025

ayrloong Apr 29, 2025 •

edited

Loading

ayrloong commented Apr 29, 2025 •

edited

Loading

ayrloong commented Apr 29, 2025 •

edited

Loading