-
Notifications
You must be signed in to change notification settings - Fork 306
Implement Kubectl Get using generic client for any Kubernetes resource #1685
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: brendandburns <[email protected]>
Co-authored-by: brendandburns <[email protected]>
|
@copilot can you refactor this to use the generic client csharp/examples/generic/Generic.cs Line 7 in 9566a3f
|
Co-authored-by: brendandburns <[email protected]>
Refactored to use GenericClient with a single generic Usage: var pod = kubectl.Get<V1Pod>("my-pod", "default");
var namespace = kubectl.Get<V1Namespace>("kube-system");
var deployment = kubectl.Get<V1Deployment>("my-deploy", "prod");Commit: 2147fb4 |
brendandburns
left a comment
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
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, Copilot 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 |
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.
Pull Request Overview
This PR implements kubectl get functionality for the KubernetesClient.Kubectl package, enabling retrieval of any Kubernetes resource by name using a generic client approach. The implementation follows existing patterns in the codebase (similar to Version and Cordon methods) with separate async and synchronous methods.
Key changes:
- Generic
GetAsync<T>()andGet<T>()methods that work with any Kubernetes resource type marked withKubernetesEntityAttribute - Automatic detection of resource metadata (group, version, plural name) from the type
- Intelligent handling of both namespaced and cluster-scoped resources
- Comprehensive E2E tests for Pod, Service, Deployment, and Namespace resources
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/KubernetesClient.Kubectl/Beta/AsyncKubectl.Get.cs | Implements async generic GetAsync<T>() method using GenericClient to retrieve resources |
| src/KubernetesClient.Kubectl/Beta/Kubectl.Get.cs | Provides synchronous wrapper Get<T>() method following existing patterns |
| tests/Kubectl.Tests/KubectlTests.Get.cs | E2E tests using MinikubeFact for Pod, Service, Deployment, and Namespace resources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var genericClient = new GenericClient(client, metadata.Group, metadata.ApiVersion, metadata.PluralName, disposeClient: false); | ||
|
|
||
| if (@namespace != null) | ||
| { | ||
| return await genericClient.ReadNamespacedAsync<T>(@namespace, name, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| return await genericClient.ReadAsync<T>(name, cancellationToken).ConfigureAwait(false); | ||
| } |
Copilot
AI
Nov 18, 2025
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.
The GenericClient instance is not disposed after use. While disposeClient: false prevents disposal of the underlying Kubernetes client, the GenericClient itself should be disposed to follow proper IDisposable patterns. Consider wrapping it in a using statement or calling Dispose() after the Read operation.
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } |
Copilot
AI
Nov 18, 2025
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.
Generic catch clause.
| catch | ||
| { | ||
| // Ignore cleanup errors | ||
| } |
Copilot
AI
Nov 18, 2025
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.
Generic catch clause.
| catch | ||
| { | ||
| // Ignore cleanup errors |
Copilot
AI
Nov 18, 2025
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.
Generic catch clause.
| catch | |
| { | |
| // Ignore cleanup errors | |
| catch (Exception ex) | |
| { | |
| // Ignore cleanup errors, but log for visibility | |
| Console.WriteLine($"Cleanup error in DeleteNamespacedDeployment: {ex}"); |
| if (@namespace != null) | ||
| { | ||
| return await genericClient.ReadNamespacedAsync<T>(@namespace, name, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| else | ||
| { | ||
| return await genericClient.ReadAsync<T>(name, cancellationToken).ConfigureAwait(false); | ||
| } |
Copilot
AI
Nov 18, 2025
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.
Both branches of this 'if' statement return - consider using '?' to express intent better.
| if (@namespace != null) | |
| { | |
| return await genericClient.ReadNamespacedAsync<T>(@namespace, name, cancellationToken).ConfigureAwait(false); | |
| } | |
| else | |
| { | |
| return await genericClient.ReadAsync<T>(name, cancellationToken).ConfigureAwait(false); | |
| } | |
| return @namespace != null | |
| ? await genericClient.ReadNamespacedAsync<T>(@namespace, name, cancellationToken).ConfigureAwait(false) | |
| : await genericClient.ReadAsync<T>(name, cancellationToken).ConfigureAwait(false); |
|
@copilot could you please rebase |
…ator, specific exception handling Co-authored-by: tg123 <[email protected]>
|
New changes are detected. LGTM label has been removed. |
…oseconds (#1687) * Initial plan * Fix datetime serialization to always output 6 decimal places for fractional seconds Co-authored-by: tg123 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: tg123 <[email protected]>
* Initial plan * Add .NET 10 support to all projects and workflows Co-authored-by: tg123 <[email protected]> * Fix datetime serialization to always output 6 decimal places for microseconds (#1687) * Initial plan * Fix datetime serialization to always output 6 decimal places for fractional seconds Co-authored-by: tg123 <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: tg123 <[email protected]> * Initial plan --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: tg123 <[email protected]>
Rebased onto latest master. The branch now includes the latest changes:
Also addressed code review suggestions:
All tests build successfully including for the new .NET 10 target framework. |
Implementation Plan for KubectlGet
Rebase Complete
Successfully rebased onto master commit 068251c. The branch now includes latest changes from master:
All code builds successfully with no errors, including for the new .NET 10 target framework.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.