Skip to content

Conversation

lqlive
Copy link
Contributor

@lqlive lqlive commented Sep 15, 2025

Base on #1662
@tg123 Hi ,This is my initial implementation. Could you help me see how much it differs from your ideas?

During my implementation, I thought of two issues:

1.Can the ListNamespacedPodWithHttpMessagesAsync method be set as protected or private to prevent external access?

2.Replace the tuple (WatchEventType, V1Pod) in IAsyncEnumerable<(WatchEventType, V1Pod)> with a new type, such as:

public readonly struct ResourceEvent<TResource>(EventType eventType, TResource value, TResource oldValue = default!)
{
    public EventType EventType { get; } = eventType;
    public TResource OldValue { get; } = oldValue;
    public TResource Value { get; } = value;
}

public enum EventType
{
    Unknown = 0,
    Created = 1,
    Updated = 2,
    Deleted = 3
}

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 15, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 15, 2025
@tg123 tg123 requested a review from Copilot September 15, 2025 17:04
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 enhances the Kubernetes C# client with watch functionality by adding code generation support for watch operations. The implementation provides both callback-based and async enumerable patterns for watching Kubernetes resources.

  • Adds watch operation code generation to client templates
  • Introduces parameter filtering to exclude the "watch" parameter from generated methods
  • Updates examples to demonstrate the new watch functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
OperationsExtensions.cs.template Adds watch operation generation with callback and async enumerable patterns
Client.cs.template Generates watch methods in client classes with filtered parameters
TypeHelper.cs Adds support for extracting single item types from list types
ParamHelper.cs Implements parameter filtering and watch-specific parameter handling
KubernetesClient.Aot.csproj Enables Watcher classes in AOT build
Program.cs (examples) Updates watch and clientset examples to use new API

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tg123 tg123 self-assigned this Sep 16, 2025
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.

could you please also change e2e test and watcher related test?

and lets mark direct call to watch() deprecated, will be removed next version

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2025
…ling methods and add async enumerable support
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lqlive
Once this PR has been reviewed and has the lgtm label, please ask for approval from tg123. 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:

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 size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2025
@lqlive
Copy link
Contributor Author

lqlive commented Sep 17, 2025

could you please also change e2e test and watcher related test?

I'm adding some unit tests for WatchAsync. Can you help me check if there are any missing points?

and lets mark direct call to watch() deprecated, will be removed next version

XXXXXWithHttpMessagesAsync Added Obsolete attribute

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2025
@lqlive
Copy link
Contributor Author

lqlive commented Sep 18, 2025

@tg123 Please re-run CI. Yesterday I pulled the latest code

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.

overall lgtm

lets close on weather hide bool watch or watcherext, then we can merge

/// <returns>IAsyncEnumerable of watch events</returns>
[Obsolete("This method will be deprecated in future versions.")]
public static IAsyncEnumerable<(WatchEventType, T)> WatchAsync<T, L>(
this Task<HttpOperationResponse<L>> responseTask,
Copy link
Member

Choose a reason for hiding this comment

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

i think we should not filter bool watch in list at the moment
otherwise, this WatchExt cant work anymore for old code

so what do you think, make those internal or keep compatible until next version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's best to just change it to internal so we don't forget to remove it in a future version.

Copy link
Member

Choose a reason for hiding this comment

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

cc @brendanburns for ideas
internal it will create a big breaking change and users have to to migrate their code to new WatchXXX
do you think if it is nessary to wait one more big version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @brendanburns for ideas internal it will create a big breaking change and users have to to migrate their code to new WatchXXX do you think if it is nessary to wait one more big version?

It seems that @ is wrong. It should be @brendandburns

@tg123 tg123 requested a review from Copilot September 29, 2025 08:10
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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +299 to +300
var listSchema = response?.Schema?.Reference;
if (listSchema?.Properties?.TryGetValue("items", out var itemsProperty) != true)
Copy link
Preview

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

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

The code attempts to access Properties on Reference which is incorrect. The Reference property is a string reference to a schema definition, not the actual schema object. This should be accessing the resolved schema object instead of the reference.

Suggested change
var listSchema = response?.Schema?.Reference;
if (listSchema?.Properties?.TryGetValue("items", out var itemsProperty) != true)
// Resolve the schema reference to the actual schema object
var referencedSchema = response?.Schema?.ActualSchema;
if (referencedSchema?.Properties?.TryGetValue("items", out var itemsProperty) != true)

Copilot uses AI. Check for mistakes.

…ributes; improve example method signature in Program.cs
@tg123 tg123 requested a review from Copilot September 29, 2025 18:12
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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

await client.CoreV1.ListNamespacedPodWithHttpMessagesAsync("default", watch: true).ConfigureAwait(true);
using var watcher = client.CoreV1.WatchListNamespacedPod("default", onEvent: (type, item) => { });
Assert.Equal(HttpVersion.Version20, handler.Version);
await Task.CompletedTask.ConfigureAwait(true);
Copy link
Preview

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

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

The statement await Task.CompletedTask.ConfigureAwait(true) is unnecessary since Task.CompletedTask is already completed synchronously. This line can be removed.

Suggested change
await Task.CompletedTask.ConfigureAwait(true);

Copilot uses AI. Check for mistakes.

Comment on lines +298 to +302
var listSchema = response?.Schema?.Reference;
if (listSchema?.Properties?.TryGetValue("items", out var itemsProperty) != true)
{
return null;
}
Copy link
Preview

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

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

The method attempts to access Properties on a JsonReference object, but JsonReference doesn't have a Properties property. This will always return null. You should resolve the reference first to get the actual schema.

Suggested change
var listSchema = response?.Schema?.Reference;
if (listSchema?.Properties?.TryGetValue("items", out var itemsProperty) != true)
{
return null;
}
// Resolve the reference to the actual schema before accessing Properties
var referencedSchema = response?.Schema?.Reference as JsonSchema;
if (referencedSchema == null)
{
return null;
}
if (!referencedSchema.Properties.TryGetValue("items", out var itemsProperty))
{
return null;
}

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants