Updates target framework to .NET 10 #84
Conversation
Upgrades the project to target .NET 10. This change includes updating the target framework in the project files, global.json, Dockerfile, and project templates. It also updates package versions to be compatible with .NET 10 and uses central package management. The update ensures the project uses the latest .NET runtime and features.
📝 WalkthroughWalkthroughAdds root-level central package management, upgrades .NET SDK/targets to .NET 10, converts Kubernetes client list calls to streaming watch semantics (ListAsync → WatchAsync) with EventWatcher and logging updates, modernizes test host and watch test helpers, and updates related builders and project metadata. Changes
Sequence Diagram(s)sequenceDiagram
actor Controller
participant EventWatcher
participant IKubernetesClient
participant KubernetesAPI
participant Logger
Controller ->> EventWatcher: StartWatch(labelSelector, ct)
EventWatcher ->> Logger: BeginWatch(ns, plural, labelSelector)
EventWatcher ->> IKubernetesClient: WatchAsync<T>(labelSelector, ct)
IKubernetesClient ->> KubernetesAPI: WatchListNamespaced/ClusterCustomObjectAsync(...)
KubernetesAPI -->> IKubernetesClient: stream events (IAsyncEnumerable)
IKubernetesClient -->> EventWatcher: yield (WatchEventType, object)
EventWatcher ->> EventWatcher: deserialize object → T (if JsonElement)
EventWatcher ->> Controller: OnEvent(T item, eventType)
EventWatcher ->> Logger: EndWatch(ns, plural, labelSelector)
alt error
KubernetesAPI -->> EventWatcher: throws (HttpOperationException/HttpRequestException/OperationCanceled)
EventWatcher ->> Logger: Log error and delay/retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @Directory.Packages.props:
- Around line 9-10: The listed packages are on the unsupported .NET Core 2.x
line and must be bumped to supported ASP.NET/Core 10.x releases: update
PackageVersion entries for Microsoft.AspNetCore,
Microsoft.AspNetCore.Server.IIS, Microsoft.AspNetCore.Http.Abstractions, and
Microsoft.AspNetCore.Hosting.Abstractions to the matching 10.x version used
elsewhere (e.g., 10.0.1 to align with Microsoft.AspNetCore.TestHost), update the
surrounding file comments to reflect the new versions, then run dotnet
restore/build/test to verify compatibility and resolve any API or transitive
dependency changes.
In @src/K8sOperator.NET/KubernetesClient.cs:
- Around line 12-13: The implementations of WatchAsync<T> are still calling
Logger.ListAsync() which is misleading; update the logging to reflect a watch
operation by either renaming the logger extension to Logger.WatchAsync(...) or
changing the message inside Logger.ListAsync(...) to indicate a watch, and then
replace calls to Logger.ListAsync() inside the WatchAsync<T> implementations
(the methods that call WatchListNamespacedCustomObjectAsync and
WatchListClusterCustomObjectAsync) to use the new Logger.WatchAsync(...) name
(or keep the same method but update its message), ensuring EventWatcher.cs
callers remain consistent with WatchAsync<T>() and log entries accurately
describe watching rather than listing.
In @test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj:
- Around line 3-6: Remove the unused PackageReference to
Microsoft.AspNetCore.Server.IIS from the test project file; open
K8sOperator.NET.Tests.csproj, locate the <PackageReference
Include="Microsoft.AspNetCore.Server.IIS" /> entry and delete that line so the
test project no longer references an unnecessary IIS server package.
In @test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs:
- Around line 20-43: The code calls builder.Build() twice (var app =
builder.Build() and later _server = builder.Build()), which freezes the service
collection and causes the InvalidOperationException; fix by calling
builder.UseTestServer() before building, remove the second Build() and assign
the already-built app to _server (i.e. set _server = app or equivalent), and
ensure you call app.Start() after building so GetTestServer() returns a valid
TestServer when you call _server.GetTestServer().
🧹 Nitpick comments (3)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (1)
52-57: Consider using async disposal pattern for cleaner shutdown.
StopAsync()returns aTaskthat is not awaited here. WhileWaitForShutdown()blocks, consider makingDisposeproperly synchronous or implementingIAsyncDisposable.♻️ Suggested improvement
public void Dispose() { - _server.StopAsync(); - _server.WaitForShutdown(); + _server.StopAsync().GetAwaiter().GetResult(); _server.Dispose(); }src/K8sOperator.NET/KubernetesClient.cs (1)
30-30: Logger method nameListAsyncis misleading for watch operations.The method name changed from
ListAsynctoWatchAsync, but the logger calls still referenceLogger.ListAsync(...). Consider renaming or adding aWatchAsynclogging extension for consistency.Also applies to: 99-99
src/K8sOperator.NET/EventWatcher.cs (1)
69-82: Task.Delay with cancellation token in catch blocks may throw during shutdown.When
OperationCanceledExceptionorHttpOperationExceptionis caught andTask.Delayis called with thecancellationToken, if cancellation has been requested,Task.Delaywill throw anotherOperationCanceledException. This is likely intentional to exit the loop, but theOperationCanceledExceptioncatch block (line 73-77) logs "restarting..." which is misleading when the delay itself is what triggers the next cancellation.Consider using
CancellationToken.Nonefor the delay, or checkcancellationToken.IsCancellationRequestedbefore delaying:Suggested improvement
catch (OperationCanceledException) { Logger.WatcherError("Operation was canceled restarting..."); - await Task.Delay(TimeSpan.FromSeconds(5), cancellationToken); + if (!cancellationToken.IsCancellationRequested) + await Task.Delay(TimeSpan.FromSeconds(5), cancellationToken); } catch (HttpOperationException ex) { Logger.WatcherError($"Http Error: {ex.Response.Content}, restarting..."); - await Task.Delay(TimeSpan.FromSeconds(5), cancellationToken); + if (!cancellationToken.IsCancellationRequested) + await Task.Delay(TimeSpan.FromSeconds(5), cancellationToken); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Directory.Packages.propsK8sOperator.slnexamples/SimpleOperator/SimpleOperator.csprojglobal.jsonsrc/Directory.Build.propssrc/Directory.Packages.propssrc/K8sOperator.NET.Generators/Builders/ClusterRoleBindingBuilderExtensions.cssrc/K8sOperator.NET.Generators/Builders/DeploymentBuilderExtensions.cssrc/K8sOperator.NET/EventWatcher.cssrc/K8sOperator.NET/K8sOperator.NET.csprojsrc/K8sOperator.NET/KubernetesClient.cstest/Directory.Build.propstest/Directory.Packages.propstest/K8sOperator.NET.Tests/ControllerDatasourceTests.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojtest/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs
💤 Files with no reviewable changes (3)
- src/K8sOperator.NET/K8sOperator.NET.csproj
- test/Directory.Packages.props
- src/Directory.Packages.props
🧰 Additional context used
🧬 Code graph analysis (3)
src/K8sOperator.NET.Generators/Builders/ClusterRoleBindingBuilderExtensions.cs (2)
src/K8sOperator.NET/ControllerDatasource.cs (1)
Add(83-91)src/K8sOperator.NET.Generators/Builders/KubernetesObjectBuilder.cs (2)
Add(13-13)Add(27-30)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (3)
test/K8sOperator.NET.Tests/Logging/TestOutputLogger.cs (1)
IDisposable(54-55)test/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cs (1)
AddTestOutput(23-38)test/K8sOperator.NET.Tests/Logging/TestOutputLoggerProvider.cs (1)
ILogger(59-59)
src/K8sOperator.NET/EventWatcher.cs (1)
src/K8sOperator.NET/Models/CustomResource.cs (3)
CustomResource(10-15)CustomResource(22-30)CustomResource(38-47)
🪛 GitHub Actions: Build
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs
[error] 39-39: System.InvalidOperationException: The service collection cannot be modified because it is read-only. at Microsoft.Extensions.DependencyInjection.ServiceCollection.ThrowReadOnlyException() ... at MockKubeApiServer..ctor(...) in MockKubeApiServer.cs:line 39
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze
🔇 Additional comments (16)
examples/SimpleOperator/SimpleOperator.csproj (1)
4-4: LGTM!The target framework update to
net10.0is consistent with the repository-wide upgrade reflected inglobal.jsonandsrc/Directory.Build.props.src/Directory.Build.props (1)
4-4: LGTM!Centralizing the
TargetFrameworkinDirectory.Build.propsis a good practice, ensuring all source projects consistently targetnet10.0.test/K8sOperator.NET.Tests/ControllerDatasourceTests.cs (2)
1-12: LGTM!The file-scoped namespace and reorganized using directives align with modern C# conventions. The switch to
AwesomeAssertionsis consistent with the test infrastructure changes mentioned in the PR.
74-78: LGTM!Minor formatting improvement adding explicit braces to the lambda expression, improving readability.
global.json (1)
3-3: .NET 10 SDK 10.0.100 is generally available and appropriate for use.The pinned SDK version is available and released as GA, so no changes are needed.
Directory.Packages.props (1)
1-28: Central package management setup is well-structured.The file correctly enables
ManagePackageVersionsCentrallyand organizes packages into logical groups (core, test, dev tools). This centralizes version management and simplifies updates.K8sOperator.sln (1)
3-4: Visual Studio solution format update looks correct.The update to Visual Studio Version 18 is consistent with the .NET 10 upgrade. The solution structure and project references remain intact.
src/K8sOperator.NET/KubernetesClient.cs (1)
26-44: Watch implementations look correct.Both
NamespacedKubernetesClientandClusterKubernetesClientproperly implement the watch pattern using the KubernetesClient library'sWatchList*CustomObjectAsyncmethods. The 60-minute timeout andallowWatchBookmarks: trueare appropriate for long-running watch operations.Also applies to: 94-112
test/Directory.Build.props (1)
4-4: net10.0 upgrade and AwesomeAssertions integration are correct.AwesomeAssertions is fully compatible with .NET 10 (targets net6.0+), and the package is properly configured with central package management. The upgrade and assertion library configuration are ready to merge.
src/K8sOperator.NET/EventWatcher.cs (3)
91-104: Fire-and-forget pattern for event processing is acceptable but worth noting.The
OnEventmethod uses a fire-and-forget pattern whereProccessEventAsyncruns without being awaited, and failures are only logged viaContinueWith. This is a reasonable approach for event processing where one failed event shouldn't block others, but be aware that:
- Exceptions won't propagate to the caller
- Events may be processed out of order if processing times vary
- No backpressure mechanism if events arrive faster than they can be processed
If ordered processing or backpressure is ever needed, consider using a
Channel<T>or similar.
56-87: Good restructuring of watch lifecycle logging.Moving
BeginWatchinside the loop andEndWatchto afinallyblock ensures each watch iteration is properly logged, even when exceptions occur. This improves observability and debugging.
62-66: Use defensive casting to handle potential deserialization issues.The cast
(T)itemat line 66 converts anobjectreturned from the Kubernetes client library. While the library should deserialize to the correct type based on the group/API version/plural name provided, anInvalidCastExceptionwould cause the watch loop to exit unexpectedly. Consider using theasoperator with a null check:var resource = item as T; if (resource != null) { OnEvent(type, resource); }This prevents unexpected loop termination if the underlying library returns an unexpected type.
src/K8sOperator.NET.Generators/Builders/ClusterRoleBindingBuilderExtensions.cs (2)
23-29: LGTM - Clean object initializer syntax.The switch to object initializer syntax for
V1RoleRefis cleaner and more idiomatic C#.
44-48: LGTM - Consistent style with WithRoleRef.The
Rbacv1Subjectinitialization matches the style used inWithRoleRef, maintaining consistency.src/K8sOperator.NET.Generators/Builders/DeploymentBuilderExtensions.cs (2)
1-1: LGTM - Using directive cleanup.Consolidating to
k8s.Modelsnamespace.
165-172: Verify Corev1ResourceClaim is correct for your Kubernetes client version.The type change from
V1ResourceClaimtoCorev1ResourceClaimis a valid API update. Both types exist ink8s.Models, whereCorev1ResourceClaimis the lightweight Pod-spec-level resource claim reference. This is the correct type for theclaimsparameter inWithResources. Since this is a public extension method, ensure this breaking change is documented in release notes and migration guidance for external consumers.
Directory.Packages.props
Outdated
| <PackageVersion Include="Microsoft.AspNetCore.Http.Abstractions" Version="2.3.9" /> | ||
| <PackageVersion Include="Microsoft.AspNetCore.Server.IIS" Version="2.2.6" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && pwdRepository: pmdevers/K8sOperator.NET
Length of output: 192
🏁 Script executed:
find . -name "Directory.Packages.props" -type fRepository: pmdevers/K8sOperator.NET
Length of output: 94
🏁 Script executed:
cat -n Directory.Packages.propsRepository: pmdevers/K8sOperator.NET
Length of output: 2047
🏁 Script executed:
# Check if there are any references to these old ASP.NET Core packages in project files
find . -name "*.csproj" -o -name "*.fsproj" | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 365
🌐 Web query:
NuGet security advisories Microsoft.AspNetCore.Http.Abstractions 2.3.9
💡 Result:
Short answer: I couldn't find a NuGet security advisory that specifically lists Microsoft.AspNetCore.Http.Abstractions v2.3.9 as vulnerable. The package page shows v2.3.9 published/updated on Jan 7, 2026. [1]
Notes and context:
- Microsoft/GitHub advisories have recent .NET/ASP.NET Core vulnerabilities (e.g., CVE-2024-21386, CVE-2024-21319) that affect various ASP.NET Core/runtime packages; check whether your app’s workload/runtime packages (Kestrel, Microsoft.AspNetCore.App, etc.) are impacted rather than this single Abstractions package. [2][3]
Recommendation: If you use v2.3.9, verify your full dependency tree for impacted runtime packages and apply the vendor-recommended patched versions or upgrade to a supported release. For authoritative details check the NuGet package page and the dotnet/aspnetcore security advisories. [1][2][3]
Sources:
[1] NuGet package — Microsoft.AspNetCore.Http.Abstractions 2.3.9 (NuGet). (packages.nuget.org)
[2] Microsoft Security Advisory / GitHub Advisory (CVE-2024-21386). (github.com)
[3] Microsoft Security Advisory (CVE-2024-21319) — dotnet/aspnetcore advisories. (github.com)
Would you like me to scan your project's dependency graph (project files) for known advisories and produce exact upgrade suggestions?
Citations:
- 1: https://packages.nuget.org/packages/Microsoft.AspNetCore.Http.Abstractions/2.3.9?utm_source=openai
- 2: https://github.com/advisories/GHSA-g74q-5xw3-j7q9?utm_source=openai
- 3: https://github.com/dotnet/aspnetcore/security/advisories/GHSA-59j7-ghrg-fj52?utm_source=openai
🌐 Web query:
NuGet security advisories Microsoft.AspNetCore.Server.IIS 2.2.6
💡 Result:
Short answer: There is no public security advisory that specifically lists Microsoft.AspNetCore.Server.IIS version 2.2.6 as vulnerable. The package is deprecated (end-of-life) and related ASP.NET Core 2.2-era packages have known advisories — you should upgrade to a supported, patched runtime. [1][2][3]
Details and sources
- NuGet package page: Microsoft.AspNetCore.Server.IIS is deprecated (2.2.6 listed). [1]
- Snyk vulnerability listing: shows an Open Redirect affecting the microsoft.aspnetcore.server.iis range up to but excluding 2.2.6 ([2.2.0,2.2.6) ); 2.2.6 itself is not listed as vulnerable there. [2]
- Broader ASP.NET Core security advisories (example: critical HTTP request-smuggling CVE/GHSA affecting Kestrel and other 2.x/8.x/9.x components) affect some 2.x components — check if your app uses affected packages (e.g., Microsoft.AspNetCore.Server.Kestrel.Core) and apply vendor patches. [3]
Recommended actions
- If possible, upgrade off the deprecated 2.2.x line to a supported .NET/ASP.NET Core release. (2.2 is end-of-life.) [1]
- Audit your project’s package graph for other 2.x packages (Kestrel, Microsoft.AspNetCore.All, etc.) and apply any published security patches (see GHSA/CVE advisories linked above). [3]
- If you want, tell me your project’s csproj/lockfile and I’ll check which packages/versions are known-vulnerable and produce exact upgrade recommendations.
Sources
- NuGet — Microsoft.AspNetCore.Server.IIS (package page). [https://www.nuget.org/packages/microsoft.aspnetcore.server.iis]
- Snyk — microsoft.aspnetcore.server.iis vulnerabilities. [https://security.snyk.io/package/nuget/microsoft.aspnetcore.server.iis]
- GHSA / Microsoft advisories and OSV summary (ASP.NET Core security advisories, e.g., GHSA-5rrx-jjjq-q2r5 / CVE-2025-55315) — see advisory listings. [https://osv.dev/vulnerability/GHSA-5rrx-jjjq-q2r5]
🌐 Web query:
NuGet security advisories Microsoft.AspNetCore 2.2.0
💡 Result:
Short answer — Microsoft.AspNetCore 2.2.0 is affected by multiple disclosed vulnerabilities. Key advisories and references:
- NuGet package page shows recorded advisories (GHSA-6px8-22w5-w334 and GHSA-prrf-397v-83xh) and marks 2.2.0 as having at least one high-severity vulnerability. (nuget.org)
- Microsoft.AspNetCore.All / App 2.2 packages list additional GHSA advisories (including GHSA-mv2r-q4g5-j8q5). (nuget.org)
- CVE-2018-8269 (OData denial-of-service) affects 2.2.0 through before 2.2.7; fixed in 2.2.7. (advisories.gitlab.com)
- CVE-2019-0815 / related DoS issues affecting ASP.NET Core 2.2 are documented by security scanners — upgrade to patched versions. (security.snyk.io)
Recommendation: upgrade off 2.2.0 to a patched/still-supported runtime (at minimum update to the patched 2.2.x releases like 2.2.7 for the CVE-2018-8269 fix) or migrate to a supported .NET / ASP.NET Core LTS/Current release and recompile/redeploy. (advisories.gitlab.com)
If you want, I can list each GHSA/CVE with dates, CVSS scores, and direct advisory links.
Citations:
- 1: https://www.nuget.org/packages/Microsoft.AspNetCore.App/2.2.0?utm_source=openai
- 2: https://www.nuget.org/packages/Microsoft.AspNetCore.All/2.2.0?utm_source=openai
- 3: https://advisories.gitlab.com/pkg/nuget/microsoft.aspnetcore.all/CVE-2018-8269/?utm_source=openai
- 4: https://security.snyk.io/vuln/SNYK-DOTNET-MICROSOFTASPNETCORE-174184?utm_source=openai
- 5: https://advisories.gitlab.com/pkg/nuget/microsoft.aspnetcore.all/CVE-2018-8269/?utm_source=openai
Critical: Microsoft.AspNetCore 2.2.0 contains confirmed security vulnerabilities. Upgrade immediately.
The following packages at lines 9-10 and 21-22 are on the .NET Core 2.2 EOL release line (deprecated since December 2019):
Microsoft.AspNetCore 2.2.0— has confirmed vulnerabilities including CVE-2018-8269 (OData denial-of-service) and CVE-2019-0815 (DoS issues)Microsoft.AspNetCore.Server.IIS 2.2.6— deprecated/EOLMicrosoft.AspNetCore.Http.Abstractions 2.3.9andMicrosoft.AspNetCore.Hosting.Abstractions 2.2.0— EOL versions
Additionally, the file comments contradict the versions: line 7 states "Updated to latest secure versions" and line 20 states "Update to .NET 10 compatible versions," but all four packages remain at 2.x. These must be updated to supported, patched versions to match the stated intent and eliminate security exposure. Consider aligning with the 10.x versions already present for other packages (Microsoft.AspNetCore.TestHost 10.0.1).
🤖 Prompt for AI Agents
In @Directory.Packages.props around lines 9 - 10, The listed packages are on the
unsupported .NET Core 2.x line and must be bumped to supported ASP.NET/Core 10.x
releases: update PackageVersion entries for Microsoft.AspNetCore,
Microsoft.AspNetCore.Server.IIS, Microsoft.AspNetCore.Http.Abstractions, and
Microsoft.AspNetCore.Hosting.Abstractions to the matching 10.x version used
elsewhere (e.g., 10.0.1 to align with Microsoft.AspNetCore.TestHost), update the
surrounding file comments to reflect the new versions, then run dotnet
restore/build/test to verify compatibility and resolve any API or transitive
dependency changes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/K8sOperator.NET/Extensions/LoggingExtensions.cs (2)
23-28: Fix malformed log template ({label)) to avoid generator/runtime failures.
Line 26 has an unbalanced brace in the template.Proposed fix
[LoggerMessage( EventId = 2, Level = LogLevel.Error, - Message = "Error watching {ns}/{resource} {label)")] + Message = "Error watching {ns}/{resource} {label}")] public static partial void LogWatchError(this ILogger logger, Exception? ex, string ns, string resource, string label);
190-196: Update message text to matchWatchAsync(currently says “ListAsync …”).
Otherwise dashboards/searches will be misleading.Proposed fix
[LoggerMessage( EventId = 29, Level = LogLevel.Information, - Message = "ListAsync {ns}/{plural} {labelselector}" + Message = "WatchAsync {ns}/{plural} {labelselector}" )] public static partial void WatchAsync(this ILogger logger, string ns, string plural, string labelselector);test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (1)
54-59: Dispose is likely to hang/flap: don’t callWaitForShutdown()here, and don’t ignoreStopAsync()’s task.Proposed fix
public void Dispose() { - _server.StopAsync(); - _server.WaitForShutdown(); - _server.Dispose(); + _server.StopAsync().GetAwaiter().GetResult(); + _server.Dispose(); }
🤖 Fix all issues with AI agents
In @test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs:
- Around line 16-43: Change the _server field from IHost to WebApplication and
call UseTestServer() on builder.WebHost before Build(); i.e., invoke
builder.WebHost.UseTestServer() then var app = builder.Build(), assign app to
the MockKubeApiServer._server field, and call app.Start(); also move the
routing/Map calls to operate on that WebApplication instance (e.g., call
app.UseRouting(), app.Map(...)) and ensure the catch-all route handler returns a
404 status (set context.Response.StatusCode = 404 or return Results.NotFound())
instead of relying on an implicit 200.
🧹 Nitpick comments (3)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj (1)
1-1: Consider removing the UTF-8 BOM character.The file now includes a UTF-8 BOM (
) at the start. While this doesn't break anything, BOM characters are typically unnecessary for XML/csproj files and can occasionally cause issues with Unix tooling or certain editors. Consider saving without BOM for consistency.src/K8sOperator.NET/KubernetesClient.cs (2)
26-44: Small cleanup: return the watch enumerable directly (and consider how you’ll handle watch timeouts).
The temp local adds little, and the 60-minute timeout implies downstream needs to re-establish the watch.Proposed refactor
public IAsyncEnumerable<(WatchEventType, object)> WatchAsync<T>(string labelSelector, CancellationToken cancellationToken) where T : CustomResource { var info = typeof(T).GetCustomAttribute<KubernetesEntityAttribute>()!; Logger.WatchAsync(Namespace, info.PluralName, labelSelector); - var response = Client.CustomObjects.WatchListNamespacedCustomObjectAsync( + return Client.CustomObjects.WatchListNamespacedCustomObjectAsync( info.Group, info.ApiVersion, Namespace, info.PluralName, allowWatchBookmarks: true, labelSelector: labelSelector, timeoutSeconds: (int)TimeSpan.FromMinutes(60).TotalSeconds, cancellationToken: cancellationToken ); - - return response; }
94-112: Same cleanup for cluster watch; log namespace label is fine, but keep it consistent.Proposed refactor
public IAsyncEnumerable<(WatchEventType, object)> WatchAsync<T>(string labelSelector, CancellationToken cancellationToken) where T : CustomResource { var info = typeof(T).GetCustomAttribute<KubernetesEntityAttribute>()!; Logger.WatchAsync("cluster-wide", info.PluralName, labelSelector); - var response = Client.CustomObjects.WatchListClusterCustomObjectAsync( + return Client.CustomObjects.WatchListClusterCustomObjectAsync( info.Group, info.ApiVersion, info.PluralName, allowWatchBookmarks: true, labelSelector: labelSelector, timeoutSeconds: (int)TimeSpan.FromMinutes(60).TotalSeconds, cancellationToken: cancellationToken ); - - return response; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Directory.Packages.propssrc/K8sOperator.NET/Extensions/LoggingExtensions.cssrc/K8sOperator.NET/KubernetesClient.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojtest/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Directory.Packages.props
🧰 Additional context used
🧬 Code graph analysis (1)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (3)
test/K8sOperator.NET.Tests/Logging/TestOutputLogger.cs (1)
IDisposable(54-55)test/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cs (1)
AddTestOutput(23-38)test/K8sOperator.NET.Tests/Logging/TestOutputLoggerProvider.cs (1)
ILogger(59-59)
🔇 Additional comments (2)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj (1)
2-16: LGTM — central package management pattern looks correct.The package references without explicit versions correctly rely on central package management via
Directory.Packages.props. TheUpdatepattern forcoverlet.collectorandxunit.runner.visualstudiois the standard approach for overriding asset inclusion settings with CPM.Based on the summary,
Microsoft.AspNetCoreandMicrosoft.AspNetCore.Hosting.Abstractionswere removed — this is appropriate for .NET 10 since these APIs are available directly from the shared framework when targetingnet10.0.src/K8sOperator.NET/KubernetesClient.cs (1)
10-14: The k8s watch API contract is correct. KubernetesClient 18.0.13'sWatchListNamespacedCustomObjectAsyncandWatchListClusterCustomObjectAsyncextension methods returnIAsyncEnumerable<(WatchEventType, object)>, which matches the interface signature. The implementations pass through the response directly without transformation, ensuring the dependency surface is properly exposed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/K8sOperator.NET/EventWatcher.cs:
- Around line 247-251: The code inconsistently uses the field _cancellationToken
when calling ReplaceAsync instead of the method parameter cancellationToken;
change the call ReplaceAsync(resource, _cancellationToken) to
ReplaceAsync(resource, cancellationToken) so both AddOrModifyAsync and
ReplaceAsync use the same cancellation token parameter (leave
_changeTracker.TrackResourceGenerationAsHandled(resource) unchanged).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/K8sOperator.NET/EventWatcher.cs
🔇 Additional comments (5)
src/K8sOperator.NET/EventWatcher.cs (5)
34-35: LGTM!The generic constraint formatting now follows C# style conventions with proper spacing around the colon.
92-105: Fire-and-forget event processing pattern.The
ContinueWitherror handling is appropriate for logging failures. Be aware this pattern processes events concurrently without ordering guarantees—acceptable if controller handlers are idempotent and don't require strict ordering.
107-134: Event routing logic is correct.The switch correctly handles all
WatchEventTypecases, and theDeletionTimestampcheck properly distinguishes between finalization and normal add/modify flows.
147-158: LGTM!Bookmark handling correctly tracks resource generation after processing.
60-88: Per-iteration watch lifecycle looks well-structured.The refactored watch loop correctly:
- Logs begin/end watch per iteration for restart visibility
- Uses async enumerable pattern with
ConfigureAwait(false)- Handles cancellation and HTTP errors appropriately
- Guards the restart delay with cancellation check
The cast
(T)itemon line 66 is necessary becauseIKubernetesClient.WatchAsync<T>returnsIAsyncEnumerable<(WatchEventType, object)>, so the cast is required to type the object toT.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (1)
59-64: Async methods not awaited in synchronousDispose().
StopAsync()andWaitForShutdown()returnTaskbut are called fire-and-forget, meaning the server may not fully shut down before_testServer.Dispose()runs. This could cause resource leaks or flaky tests.Proposed fix: block on async calls or implement IAsyncDisposable
Option 1: Block synchronously
public void Dispose() { - _server.StopAsync(); - _server.WaitForShutdown(); + _server.StopAsync().GetAwaiter().GetResult(); _testServer.Dispose(); }Option 2: Implement IAsyncDisposable (preferred for test cleanup)
-public sealed class MockKubeApiServer : IDisposable +public sealed class MockKubeApiServer : IAsyncDisposable, IDisposable { // ... existing code ... - public void Dispose() + public async ValueTask DisposeAsync() { - _server.StopAsync(); - _server.WaitForShutdown(); + await _server.StopAsync(); _testServer.Dispose(); } + + public void Dispose() + { + DisposeAsync().AsTask().GetAwaiter().GetResult(); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/K8sOperator.NET/K8sOperator.NET.csprojtest/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs
💤 Files with no reviewable changes (1)
- src/K8sOperator.NET/K8sOperator.NET.csproj
🧰 Additional context used
🧬 Code graph analysis (1)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (3)
test/K8sOperator.NET.Tests/Logging/TestOutputLogger.cs (1)
IDisposable(54-55)test/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cs (1)
AddTestOutput(23-38)test/K8sOperator.NET.Tests/Logging/TestOutputLoggerProvider.cs (1)
ILogger(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze
🔇 Additional comments (2)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (2)
19-48: Constructor refactored to WebApplication-based approach - looks good.The migration from
IWebHosttoWebApplication.CreateBuilder()withUseTestServer()is well-structured. The routing setup, test output logging integration, and catch-all route handler are properly configured.One minor observation: the
testOutputparameter is declared as non-nullable (ITestOutputHelper), yet line 26 performs a null check. Consider making the parameter nullable (ITestOutputHelper?) if null is a valid input, or remove the null check if callers are always expected to provide a value.
50-50: Clean simplification using TestServer's BaseAddress.Using
_testServer.BaseAddressis cleaner than the previous approach of extracting addresses from server features.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/K8sOperator.NET.Tests/EventWatcherTests.cs (2)
163-167: Placeholder test provides no value.This test only asserts
truewithout testing actual functionality. Either implement the test or remove it to avoid false confidence in test coverage.Would you like me to help implement this test case?
141-161: The mock endpoint for watch requests is not properly implementing the Kubernetes streaming protocol.The test fails because
MapListNamespacedCustomObjectWithHttpMessagesAsyncwrites a raw JSON response without the newline-delimited JSON format and body flush that the official Kubernetes client library expects for watch streams. Compare withMapWatchNamespacedCustomObjectAsync, which correctly implements streaming: writes JSON + newline, explicitly flushes the response body, and checks the?watch=truequery parameter.Additionally,
EventWatcher.OnEvent(line 108-121) uses fire-and-forget processing (ContinueWithwithout awaiting), which can allow the test to complete beforeFinalizeAsyncis invoked.Recommended fix: Either use
MapWatchNamespacedCustomObjectAsyncin the test instead ofMapListNamespacedCustomObjectWithHttpMessagesAsync, or update the endpoint to implement proper watch streaming with newline delimiters and body flushing. Also consider awaiting the processing task inOnEventor adding synchronization to ensure completion before test assertions.
🤖 Fix all issues with AI agents
In @test/K8sOperator.NET.Tests/EventWatcherTests.cs:
- Around line 16-26: The property Added is misnamed because it constructs a
WatchEventType.Modified event via CreateEvent; rename the property Added to
Modified (or another accurate name) and update all test references to use
Modified instead of Added so the symbol Modified => CreateEvent(...
WatchEventType.Modified ...) matches its behavior; ensure you update every usage
in EventWatcherTests to the new property name to keep tests consistent.
In @test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs:
- Line 29: The hardcoded Kestrel listen port in MockKubeApiServer
(config.UseKestrel(options => { options.Listen(IPAddress.Loopback, 8888); }))
can cause AddressInUseException in parallel tests; change the listen call to use
port 0 so the OS assigns an ephemeral port and expose that assigned port from
MockKubeApiServer (e.g., read IServerAddressesFeature or Server.Features after
Start and store an AssignedPort property) so callers/tests can retrieve the
actual bound port instead of relying on 8888.
🧹 Nitpick comments (2)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (1)
45-52: Redundant null check for non-nullable parameter.The
testOutputparameter is non-nullable (ITestOutputHelper testOutput), so the null check on line 48 is unnecessary. Consider removing it or changing the parameter to nullable if null is a valid input.test/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cs (1)
16-34: Consider setting Content-Type header for consistency.The endpoint returns JSON but doesn't set the
Content-Typeheader. While this may work for tests, setting it explicitly improves consistency with real Kubernetes API behavior.Proposed enhancement
builder.MapGet("/apis/{group}/{version}/namespaces/{namespace}/{plural}", async context => { var isWatch = context.Request.Query["watch"].ToString() == "true"; + context.Response.ContentType = "application/json"; if (!isWatch || watchEvent is null) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/K8sOperator.NET/EventWatcher.cssrc/K8sOperator.NET/Extensions/LoggingExtensions.cstest/K8sOperator.NET.Tests/EventWatcherTests.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cstest/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs
🧰 Additional context used
🧬 Code graph analysis (5)
src/K8sOperator.NET/EventWatcher.cs (1)
src/K8sOperator.NET/Models/CustomResource.cs (3)
CustomResource(10-15)CustomResource(22-30)CustomResource(38-47)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (4)
test/K8sOperator.NET.Tests/Logging/TestOutputLogger.cs (1)
IDisposable(54-55)test/K8sOperator.NET.Tests/Logging/TestOutputLoggerProvider.cs (1)
ILogger(59-59)src/K8sOperator.NET/Builder/OperatorHostBuilder.cs (1)
ConfigureLogging(133-145)test/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cs (1)
AddTestOutput(23-38)
test/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cs (2)
test/K8sOperator.NET.Tests/EventWatcherTests.cs (1)
Watcher(53-57)src/K8sOperator.NET/EventWatcher.cs (11)
Task(31-31)Task(51-105)Task(123-150)Task(152-161)Task(163-174)Task(176-197)Task(199-216)Task(218-222)Task(224-230)Task(233-241)Task(246-270)
test/K8sOperator.NET.Tests/EventWatcherTests.cs (4)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (2)
MockKubeApiServer(16-74)MockKubeApiServer(20-58)test/K8sOperator.NET.Tests/Mocks/Endpoints/ListNamespacedCustomObjectWithHttpMessagesAsync.cs (1)
MapListNamespacedCustomObjectWithHttpMessagesAsync(10-27)src/K8sOperator.NET/KubernetesClient.cs (1)
NamespacedKubernetesClient(20-66)src/K8sOperator.NET/EventWatcher.cs (1)
EventWatcher(34-271)
src/K8sOperator.NET/Extensions/LoggingExtensions.cs (1)
test/K8sOperator.NET.Tests/Logging/TestOutputLoggerProvider.cs (1)
ILogger(59-59)
🪛 GitHub Actions: Build
test/K8sOperator.NET.Tests/EventWatcherTests.cs
[error] 160-160: Test failed: HandleFinalize_async_Should_CallFinalizeAndRemoveFinalizer - NSubstitute.ReceivedCallsException: Expected to receive exactly 1 call matching: FinalizeAsync(TestResource, CancellationToken); Actually received no matching calls. See stack trace in logs. (dotnet test -c Release)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze
🔇 Additional comments (4)
src/K8sOperator.NET/Extensions/LoggingExtensions.cs (1)
190-195: LGTM!The method and message rename from
ListAsynctoWatchAsynccorrectly aligns with the broader API change from list-based to watch-based Kubernetes client operations.src/K8sOperator.NET/EventWatcher.cs (3)
62-70: LGTM on watch streaming implementation.The handling of potential
JsonElementitems via serialize-then-deserialize is a pragmatic approach to handle type mismatches from the Kubernetes client. The null check before callingOnEventis appropriate.
108-121: Fire-and-forget event processing may cause race conditions.
ProccessEventAsyncis invoked withoutawait, allowing concurrent processing of multiple events. This could lead to:
- Race conditions when multiple events for the same resource arrive in quick succession
- Out-of-order event processing
- Contention on
_changeTrackerIf concurrent processing is intentional for performance, consider documenting this behavior. Otherwise, consider awaiting or using a queue for ordered processing.
88-103: LGTM on reconnection handling.The finally block properly ensures
EndWatchis logged and implements a cancellation-safe delay before retry. The 5-second reconnection delay is reasonable for Kubernetes watch patterns.
| private static Watcher<TestResource>.WatchEvent Added => CreateEvent(WatchEventType.Modified, | ||
| new TestResource() | ||
| { | ||
| Metadata = new() | ||
| { | ||
| Name = "test", | ||
| NamespaceProperty = "default", | ||
| Finalizers = ["finalize"], | ||
| Uid = "1" | ||
| } | ||
| }); |
There was a problem hiding this comment.
Misleading variable name: Added creates a Modified event.
The Added property is named as if it creates an Added event, but it actually uses WatchEventType.Modified. This is confusing and could lead to test maintenance issues.
Proposed fix
- private static Watcher<TestResource>.WatchEvent Added => CreateEvent(WatchEventType.Modified,
+ private static Watcher<TestResource>.WatchEvent Modified => CreateEvent(WatchEventType.Modified,Then update all usages of Added to Modified in the test methods.
🤖 Prompt for AI Agents
In @test/K8sOperator.NET.Tests/EventWatcherTests.cs around lines 16 - 26, The
property Added is misnamed because it constructs a WatchEventType.Modified event
via CreateEvent; rename the property Added to Modified (or another accurate
name) and update all test references to use Modified instead of Added so the
symbol Modified => CreateEvent(... WatchEventType.Modified ...) matches its
behavior; ensure you update every usage in EventWatcherTests to the new property
name to keep tests consistent.
| app.UseEndpoints(endpoints => | ||
| services.AddRouting(); | ||
| }); | ||
| config.UseKestrel(options => { options.Listen(IPAddress.Loopback, 8888); }); |
There was a problem hiding this comment.
Hardcoded port may cause test failures during parallel execution.
Using a fixed port 8888 can lead to AddressInUseException when multiple tests run concurrently. Consider using port 0 to let Kestrel assign an ephemeral port:
Proposed fix
- config.UseKestrel(options => { options.Listen(IPAddress.Loopback, 8888); });
+ config.UseKestrel(options => { options.Listen(IPAddress.Loopback, 0); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config.UseKestrel(options => { options.Listen(IPAddress.Loopback, 8888); }); | |
| config.UseKestrel(options => { options.Listen(IPAddress.Loopback, 0); }); |
🤖 Prompt for AI Agents
In @test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs at line 29, The
hardcoded Kestrel listen port in MockKubeApiServer (config.UseKestrel(options =>
{ options.Listen(IPAddress.Loopback, 8888); })) can cause AddressInUseException
in parallel tests; change the listen call to use port 0 so the OS assigns an
ephemeral port and expose that assigned port from MockKubeApiServer (e.g., read
IServerAddressesFeature or Server.Features after Start and store an AssignedPort
property) so callers/tests can retrieve the actual bound port instead of relying
on 8888.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/K8sOperator.NET/EventWatcher.cs:
- Around line 63-79: The watch loop currently drops events when item is neither
JsonElement nor T or when JsonElement.Deserialize<T>() yields null; update the
loop that processes Client.WatchAsync<T>(LabelSelector, cancellationToken) to
log a warning (using the component's logger) whenever item is an unexpected type
or when deserialization of JsonElement returns null, include the event "type"
and item type/contents (or raw JsonElement) in the message so operators can
diagnose bad payloads, and only call OnEvent(type, ...) for the successful T
cases.
🧹 Nitpick comments (1)
src/K8sOperator.NET/EventWatcher.cs (1)
81-88: Cancellation exceptions logged as errors may be misleading.
TaskCanceledExceptionandOperationCanceledExceptionare expected during graceful shutdown. Logging them viaWatcherErrorcould create noise and confusion during normal shutdown scenarios.Consider using a different log level (e.g.,
LogDebugorLogInformation) or checkingcancellationToken.IsCancellationRequestedbefore logging.Suggested improvement
catch (TaskCanceledException ex) { - Logger.WatcherError($"Task was canceled: {ex.Message}"); + if (!cancellationToken.IsCancellationRequested) + { + Logger.WatcherError($"Task was canceled unexpectedly: {ex.Message}"); + } } catch (OperationCanceledException ex) { - Logger.WatcherError($"Operation was canceled: {ex.Message}"); + if (!cancellationToken.IsCancellationRequested) + { + Logger.WatcherError($"Operation was canceled unexpectedly: {ex.Message}"); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/K8sOperator.NET/EventWatcher.cs
🔇 Additional comments (6)
src/K8sOperator.NET/EventWatcher.cs (6)
1-8: LGTM!The addition of
System.Text.Jsonis appropriate for the newJsonElementdeserialization logic.
35-50: LGTM!Minor formatting adjustments to the generic constraint and spacing.
97-112: Good defensive handling of the finally block.The
EndWatchlogging in the finally block ensures cleanup happens regardless of how the watch exits. The delay with cancellation-safe handling before restart is a good pattern for backoff.
117-130: LGTM!The fire-and-forget pattern with proper exception handling in
ContinueWithis appropriate for event processing. The formatting change is minor.
132-159: LGTM!Minor formatting adjustments; event processing logic remains correct.
161-279: LGTM!Minor formatting and spacing adjustments; handler logic is unchanged.
| await foreach (var (type, item) in Client.WatchAsync<T>(LabelSelector, cancellationToken)) | ||
| { | ||
| OnEvent(type, item); | ||
| if (item is JsonElement je) | ||
| { | ||
| var i = je.Deserialize<T>(); | ||
| if (i is not null) | ||
| { | ||
| OnEvent(type, i); | ||
| continue; | ||
| } | ||
| } | ||
| else if (item is T resource) | ||
| { | ||
| OnEvent(type, resource); | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Events silently dropped when item type is unexpected or deserialization fails.
If item is neither JsonElement nor T, or if JsonElement deserialization returns null, the event is silently dropped with no logging. This could mask data issues or unexpected API responses.
Consider adding a log statement or warning for unhandled cases.
Suggested improvement
if (item is JsonElement je)
{
var i = je.Deserialize<T>();
if (i is not null)
{
OnEvent(type, i);
continue;
}
+ Logger.LogWarning("Failed to deserialize JsonElement to {Type}", typeof(T).Name);
}
else if (item is T resource)
{
OnEvent(type, resource);
continue;
}
+ else
+ {
+ Logger.LogWarning("Unexpected item type received: {Type}", item?.GetType().Name ?? "null");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await foreach (var (type, item) in Client.WatchAsync<T>(LabelSelector, cancellationToken)) | |
| { | |
| OnEvent(type, item); | |
| if (item is JsonElement je) | |
| { | |
| var i = je.Deserialize<T>(); | |
| if (i is not null) | |
| { | |
| OnEvent(type, i); | |
| continue; | |
| } | |
| } | |
| else if (item is T resource) | |
| { | |
| OnEvent(type, resource); | |
| continue; | |
| } | |
| } | |
| await foreach (var (type, item) in Client.WatchAsync<T>(LabelSelector, cancellationToken)) | |
| { | |
| if (item is JsonElement je) | |
| { | |
| var i = je.Deserialize<T>(); | |
| if (i is not null) | |
| { | |
| OnEvent(type, i); | |
| } | |
| else | |
| { | |
| Logger.LogWarning("Failed to deserialize JsonElement to {Type}", typeof(T).Name); | |
| } | |
| } | |
| else if (item is T resource) | |
| { | |
| OnEvent(type, resource); | |
| } | |
| else | |
| { | |
| Logger.LogWarning("Unexpected item type received: {Type}", item?.GetType().Name ?? "null"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/K8sOperator.NET/EventWatcher.cs around lines 63 - 79, The watch loop
currently drops events when item is neither JsonElement nor T or when
JsonElement.Deserialize<T>() yields null; update the loop that processes
Client.WatchAsync<T>(LabelSelector, cancellationToken) to log a warning (using
the component's logger) whenever item is an unexpected type or when
deserialization of JsonElement returns null, include the event "type" and item
type/contents (or raw JsonElement) in the message so operators can diagnose bad
payloads, and only call OnEvent(type, ...) for the successful T cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/K8sOperator.NET.Tests/EventWatcherTests.cs (4)
101-130: Consider cancelling the token and awaiting the watch task for deterministic cleanup.The
watchTaskvariable is assigned but never used. After event processing, the cancellation token should be cancelled to cleanly stop the watcher before disposing the server. Also,Task.Run(async () => await ...)can be simplified.♻️ Suggested improvement
- var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + using var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(2)); var eventProcessed = new TaskCompletionSource<bool>(); // Setup the controller to signal when AddOrModifyAsync is called _controller.AddOrModifyAsync(Arg.Any<TestResource>(), Arg.Any<CancellationToken>()) .Returns(x => { eventProcessed.TrySetResult(true); return Task.CompletedTask; }); using (var server = new MockKubeApiServer(_testOutput, endpoints => { endpoints.MapWatchNamespacedCustomObjectAsync(Added); endpoints.MapReplaceNamespacedCustomObjectAsync(); })) { var client = new NamespacedKubernetesClient(server.GetMockedKubernetesClient(), _loggerFactory.CreateLogger<NamespacedKubernetesClient>()); var watcher = new EventWatcher<TestResource>(client, _controller, _metadata, _loggerFactory); - var watchTask = Task.Run(async () => await watcher.Start(cancellationToken.Token)); + var watchTask = Task.Run(() => watcher.Start(cancellationToken.Token)); // Wait for either the event to be processed or timeout var completedTask = await Task.WhenAny(eventProcessed.Task, Task.Delay(TimeSpan.FromSeconds(3))); if (completedTask != eventProcessed.Task) { throw new TimeoutException("AddOrModifyAsync was not called within the timeout period"); } + + // Cancel and await the watcher for clean shutdown + cancellationToken.Cancel(); + await watchTask; }
140-168: Same cleanup pattern recommended as the AddOrModify test.This test has the same pattern where
watchTaskis unused andcancellationTokenis not cancelled before server disposal.♻️ Suggested improvement
- var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + using var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(2)); // ... existing setup ... - var watchTask = Task.Run(async () => await watcher.Start(cancellationToken.Token)); + var watchTask = Task.Run(() => watcher.Start(cancellationToken.Token)); // Wait for either the event to be processed or timeout var completedTask = await Task.WhenAny(eventProcessed.Task, Task.Delay(TimeSpan.FromSeconds(3))); if (completedTask != eventProcessed.Task) { throw new TimeoutException("DeleteAsync was not called within the timeout period"); } + + cancellationToken.Cancel(); + await watchTask; }
176-208: Inconsistent CancellationTokenSource usage compared to other tests.This test uses the class-level
_tokenSourcewhile the other two event tests create their ownCancellationTokenSource. For consistency and to avoid test interdependencies, consider using the same pattern across all tests.♻️ Suggested change for consistency
[Fact] public async Task HandleFinalizeAsync_Should_CallFinalizeAndRemoveFinalizer() { - var cancellationToken = _tokenSource.Token; + using var cancellationToken = new CancellationTokenSource(TimeSpan.FromSeconds(2)); var eventProcessed = new TaskCompletionSource<bool>(); // ... rest of setup ... - var watchTask = Task.Run(async () => await watcher.Start(cancellationToken)); + var watchTask = Task.Run(() => watcher.Start(cancellationToken.Token)); // ... wait logic ... + + cancellationToken.Cancel(); + await watchTask; }
215-219: Placeholder test should be implemented or removed.This test contains only
Assert.True(true)which provides no value. Consider implementing the actual test for the finalizer-adding behavior or removing it to avoid false confidence in test coverage.Would you like me to help draft the implementation for this test based on the existing test patterns?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/K8sOperator.NET.Tests/EventWatcherTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
test/K8sOperator.NET.Tests/EventWatcherTests.cs (2)
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (2)
MockKubeApiServer(16-74)MockKubeApiServer(20-58)test/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cs (1)
MapWatchNamespacedCustomObjectAsync(12-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze
🔇 Additional comments (3)
test/K8sOperator.NET.Tests/EventWatcherTests.cs (3)
1-8: LGTM!The using directives are properly organized with the new imports for
Microsoft.Extensions.LoggingandXunit.Abstractions.
16-51: LGTM!The event helper reformatting to multi-line object initializers improves readability without changing behavior.
79-96: LGTM!The test correctly switches to the Watch-based API pattern using
MapWatchNamespacedCustomObjectAsync, aligning with the EventWatcher changes that now useWatchAsync<T>instead of list-based polling.
Upgrades the project to target .NET 10.
This change includes updating the target framework in the project files, global.json, Dockerfile, and project templates. It also updates package versions to be compatible with .NET 10 and uses central package management. The update ensures the project uses the latest .NET runtime and features.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.