Skip to content

Conversation

lqlive
Copy link
Contributor

@lqlive lqlive commented Apr 28, 2025

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 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 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 tg123 requested a review from Copilot April 28, 2025 17:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member

@tg123 tg123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding [deprecated] to older style?

@k8s-ci-robot 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
@lqlive lqlive marked this pull request as ready for review April 29, 2025 10:40
@k8s-ci-robot 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 k8s-ci-robot requested a review from tg123 April 29, 2025 10:40
@lqlive
Copy link
Contributor Author

lqlive commented Apr 29, 2025

adding [deprecated] to older style?

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

@lqlive
Copy link
Contributor Author

lqlive commented Apr 29, 2025

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
3f751ac230ed7fe95927fa275bb5863

Copy link
Member

@tg123 tg123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tg123 tg123 requested a review from Copilot April 30, 2025 09:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>();

@lqlive
Copy link
Contributor Author

lqlive commented May 1, 2025

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!

@lqlive
Copy link
Contributor Author

lqlive commented May 6, 2025

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

Warning style code cleared

Copy link
Member

@tg123 tg123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, some comments

and also, could you please add some basic pod CRUD tests in E2E?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayrloong, tg123

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2025
@k8s-ci-robot k8s-ci-robot merged commit f1125e9 into kubernetes-client:master May 22, 2025
11 of 14 checks passed
@lqlive
Copy link
Contributor Author

lqlive commented May 22, 2025

Thank you very much for your help @tg123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants