Rewritten #85
Rewritten #85
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRemoved legacy solution and central package props; introduced DI-first operator entrypoints (AddOperator, OperatorService), event-watcher and command datasources, refactored controller/metadata/generation surfaces, added commands to generate launchsettings/dockerfile, moved to .NET 10 packaging, and updated tests and example operator. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as IHost
participant DI as IServiceProvider
participant OperatorSvc as OperatorService
participant EWD as EventWatcherDatasource
participant Builder as ControllerBuilder
participant Watcher as EventWatcher<T>
participant K8s as IKubernetes
participant Controller as IOperatorController
Host->>DI: AddOperator() registers services (CommandDatasource, EventWatcherDatasource, IKubernetes, OperatorService)
Host->>OperatorSvc: Host starts -> ExecuteAsync(stoppingToken)
OperatorSvc->>EWD: GetWatchers()
EWD->>Builder: Create ControllerBuilder per registered controller
Builder->>Controller: Build() -> IOperatorController
EWD-->>OperatorSvc: Return IEventWatcher instances
OperatorSvc->>Watcher: Start(cancellationToken)
Watcher->>K8s: GetWatchStream()
K8s-->>Watcher: Stream watch events
Watcher->>Controller: Invoke AddOrModify/Delete/Finalize/Error handlers
Controller-->>Watcher: Handler outcomes (may call Replace via IKubernetes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
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/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
255-273: Bug: Boolean type incorrectly maps to"string"instead of"boolean".Line 259 sets
x.Type = "string"forSystem.Boolean. In OpenAPI/JSON Schema, booleans must be represented as type"boolean". The DateTime mapping to"string"on line 269 is correct (ISO 8601 serialization), but the boolean mapping appears to be a copy-paste error.🐛 Proposed fix
if (type.FullName == "System.Boolean") { builder.Add(x => { - x.Type = "string"; + x.Type = "boolean"; x.Nullable = nullable; }); return builder; }src/K8sOperator.NET/Operator.targets (1)
14-24: Critical: Namespace mismatch will cause build failures.The
AssemblyAttributeentries referenceFrontman.Operator.Metadata.*namespace, but the attribute classes in this PR are defined inK8sOperator.NET.Metadatanamespace:
DockerImageAttribute.cs→namespace K8sOperator.NET.Metadata;NamespaceAttribute.cs→namespace K8sOperator.NET.Metadata;OperatorNameAttribute.cs→namespace K8sOperator.NET.Metadata;This mismatch will cause compilation failures when MSBuild generates the assembly attributes.
🐛 Proposed fix - use correct namespace
<ItemGroup> - <AssemblyAttribute Include="Frontman.Operator.Metadata.OperatorNameAttribute"> + <AssemblyAttribute Include="K8sOperator.NET.Metadata.OperatorNameAttribute"> <_Parameter1>$(OperatorName)</_Parameter1> </AssemblyAttribute> - <AssemblyAttribute Include="Frontman.Operator.Metadata.NamespaceAttribute"> + <AssemblyAttribute Include="K8sOperator.NET.Metadata.NamespaceAttribute"> <_Parameter1>$(OperatorNamespace)</_Parameter1> </AssemblyAttribute> - <AssemblyAttribute Include="Frontman.Operator.Metadata.DockerImageAttribute"> + <AssemblyAttribute Include="K8sOperator.NET.Metadata.DockerImageAttribute"> <_Parameter1>$(ContainerRegistry)</_Parameter1> <_Parameter2>$(ContainerRepository)</_Parameter2> <_Parameter3>$(ContainerImageTag)$(ContainerFamily)</_Parameter3> </AssemblyAttribute> </ItemGroup>src/K8sOperator.NET/EventWatcher.cs (1)
103-103: Typo in method name:ProccessEventAsync.The method name has a typo - "Proccess" should be "Process".
🔧 Suggested fix
- private async Task ProccessEventAsync(WatchEventType eventType, T resource) + private async Task ProcessEventAsync(WatchEventType eventType, T resource)Also update the call site at line 92:
- ProccessEventAsync(eventType, customResource!) + ProcessEventAsync(eventType, customResource!)
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/Commands/HelpCommand.cs`:
- Around line 17-18: The code builds info via
commands.SelectMany(...OfType<OperatorArgumentAttribute>()) and then calls
info.Max(x => x.Argument.Length) which will throw if info is empty; update the
HelpCommand logic to guard against an empty sequence by using a fallback (e.g.,
if (!info.Any()) set maxSize = 0 or compute max via info.Select(x =>
x.Argument.Length).DefaultIfEmpty(0).Max()), ensuring references to
OperatorArgumentAttribute, the info variable and maxSize are preserved.
- Around line 1-4: Remove the unused Frontman.Operator using directive at the
top of HelpCommand.cs; the HelpCommand class only relies on
K8sOperator.NET.Builder types (IOperatorCommand and OperatorArgumentAttribute),
so delete the Frontman.Operator import to clean up unused references and ensure
no other code in HelpCommand.cs depends on that namespace.
In `@src/K8sOperator.NET/Commands/VersionCommand.cs`:
- Around line 15-19: The current VersionCommand prints empty values when
metadata is missing; update the retrieval and output to handle null/empty
results from watcher.Metadata.TryGetValue for OperatorNameAttribute and
DockerImageAttribute by providing explicit fallbacks (e.g., "unknown" or
"metadata not available") or printing a clear message like "Operator metadata
not available"; modify the variables where you call watcher.Metadata.TryGetValue
and the Console.WriteLine output so it checks for null/empty and substitutes the
fallback, ensuring EventWatcherDatasource and the TryGetValue calls are the
locations you change.
In `@src/K8sOperator.NET/EventWatcher.cs`:
- Line 320: Replace the use of .First() on metadata when assigning the private
field _crd to avoid InvalidOperationException: use
metadata.OfType<KubernetesEntityAttribute>().FirstOrDefault() and add a null
check after retrieval (in the EventWatcher constructor or initializer) that
throws a clear, descriptive exception (or handles the missing attribute)
referencing KubernetesEntityAttribute and _crd so callers see why the attribute
is required.
In `@src/K8sOperator.NET/EventWatcherDatasource.cs`:
- Around line 34-51: GetWatchers() is an iterator that creates new IEventWatcher
instances on each enumeration causing duplicate watcher objects when
InstallCommand.RunAsync() enumerates it twice (in CreateClusterRole() and the
subsequent foreach); fix by materializing the sequence once (e.g. call .ToList()
on the enumerable returned by GetWatchers() inside InstallCommand.RunAsync() and
use that list for both CreateClusterRole() and the foreach) or change
GetWatchers() to return a cached IReadOnlyList<IEventWatcher> so the same
instances are reused; update references to the watchers variable accordingly.
In `@src/K8sOperator.NET/Generation/ClusterRoleBuilder.cs`:
- Around line 1-2: Remove the unused import by deleting the using directive
"using Frontman.Operator;" from ClusterRoleBuilder.cs; the Initialize() method
referenced in this file comes from the k8s namespace extensions (k8s) so keep
"using k8s;" and remove only the unused Frontman.Operator using to clean up the
file.
In `@src/K8sOperator.NET/LoggerExtentions.cs`:
- Line 7: The public class name LoggerExtentions is misspelled and must be
renamed to LoggerExtensions to avoid shipping a broken public API; rename the
class declaration in LoggerExtentions (and the file to LoggerExtensions.cs),
update all references/usages (including using/imports, partial class
declarations, and any tests or callers) to the new name, and preserve
accessibility and members so there is no behavioral change — ensure the type
identity is updated consistently across the project (e.g., any partial
declarations, extension method declarations on the LoggerExtensions class) so
the build and public surface remain correct.
- Line 5: The file LoggerExtentions.cs currently declares the wrong namespace
(Frontman.Operator); update the namespace declaration to "K8sOperator.NET" so it
matches the rest of the assembly and project files; locate the namespace
declaration line in LoggerExtentions.cs and replace it with the correct
namespace while leaving the class/methods (e.g., any LoggerExtensions class or
extension methods) and using statements untouched, then rebuild to ensure no
remaining namespace mismatches.
In `@src/K8sOperator.NET/Metadata/DockerImageAttribute.cs`:
- Around line 6-13: The public API has a typo: the constructor parameter and
property are named "registery"/"Registery" instead of "registry"/"Registry";
update the primary constructor parameter name from registery to registry and
rename the property Registery to Registry (and update its initializer and any
usages) in the DockerImageAttribute class so the Default static factory and
GetImage() use the correctly spelled Registry identifier; keep the rest of the
API and behavior unchanged.
In `@src/K8sOperator.NET/Metadata/LabelSelectorAttribute.cs`:
- Around line 5-10: Make the attribute value immutable and guard against null by
validating the constructor parameter `labelSelector` in `LabelSelectorAttribute`
(throw ArgumentNullException if null) and expose the value as a read-only
property (no public setter) so `LabelSelector` cannot be mutated after
construction; keep the existing `ToString()` implementation that references
`LabelSelector`.
In `@src/K8sOperator.NET/OperatorExtensions.cs`:
- Around line 89-93: The Filter method in OperatorExtensions.cs uses
command.Metadata.OfType<OperatorArgumentAttribute>().First() which can throw if
the attribute is missing; change to use FirstOrDefault(), store the result in a
local (e.g., var attr = ...FirstOrDefault()), null-check it and return false if
attr is null, then use attr.Argument for the comparison against
args.FirstOrDefault(); this keeps the behavior but avoids a possible
InvalidOperationException when CommandInfo.Metadata lacks
OperatorArgumentAttribute.
In `@src/K8sOperator.NET/OperatorService.cs`:
- Around line 17-28: EventWatcherDatasource.GetWatchers() is being enumerated
twice (first by Any(), then by foreach) which can recreate watchers and cause
duplicates; fix by materializing the sequence once into a collection (e.g., call
ToList() on the result of EventWatcherDatasource.GetWatchers()) and then use
that variable for the Any() check and the foreach; update the local variable
watcher collection in OperatorService (the watchers variable) so the sequence is
enumerated only once.
In
`@test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLogger.cs`:
- Around line 5-17: Update the XML documentation for TestContextLogger to
reflect that it writes to the current TestContext rather than "Xunit test
output": change the class summary and constructor param docs to mention
TestContext-based logging and keep references to ILogger and the TestContext
"context" parameter (e.g., the <see cref="ILogger"/> reference and the
constructor param named "context") so the docs accurately describe the logging
target.
In
`@test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs`:
- Around line 5-17: The XML docs still say “Xunit test output” but the class is
a TestContext-based logger provider; update the XML summary on
TestContextLoggerProvider to describe it as a TestContext-based logger provider
that writes log output to the provided test context, and update the <param
name="testContext"> documentation to describe that it is the TestContext (test
output sink) passed into the constructor; ensure <see
cref="TestContextLoggerProvider"/> and <see cref="ILoggerProvider"/> cref tags
remain correct.
In
`@test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cs`:
- Around line 5-23: Update the XML docs on TestContextLoggingExtensions so they
reference TestContext logging rather than Xunit: change the class summary from
"Extension methods for logging to Xunit text output." to something like
"Extension methods for logging to TestContext output.", and update the method
summary and param descriptions on the extension(ILoggingBuilder logging) method
to mention "TestContext" (and that testContext is the current test's TestContext
output) and adjust any remaining "Xunit" references accordingly.
- Around line 8-40: The class TestContextLoggingExtensions uses the invalid
preview syntax extension(ILoggingBuilder logging); replace it with a standard
static extension method pattern: make the class static, declare a public static
void AddTestLogging(this ILoggingBuilder logging, TestContext testContext,
LogLevel minLogLevel = LogLevel.Information) and keep the existing null checks
and logging.AddProvider(new TestContextLoggerProvider(testContext,
minLogLevel)); also update the class summary XML comment to reference
TestContext instead of Xunit so documentation matches the behavior; ensure the
AddTestLogging method signature includes the this modifier to compile as an
extension method.
In
`@test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/OperatorExtensions_Tests.cs`:
- Around line 87-101: The test builds an unrelated IHost; instead build the host
from the same IServiceCollection configured by AddOperator so DI matches. After
calling services.AddOperator(), create the host with new
HostBuilder().ConfigureServices(s => { foreach (var sd in services) s.Add(sd);
}).Build(); then use host.Services.GetRequiredService<CommandDatasource>() (or
build the provider from the same services) and call
commandDatasource.GetCommands(host) to ensure commands resolve host-scoped
services.
In `@test/K8sOperator.NET.Tests/WebApp/WebApp.http`:
- Around line 3-4: The HTTP test file references a non-existent endpoint
(/weatherforecast/) while the app only exposes /ping in Program.cs; either
update test/K8sOperator.NET.Tests/WebApp/WebApp.http to call GET
{{WebApp_HostAddress}}/ping (and set Accept: application/json) or add a matching
route in Program.cs (e.g., mapGet("/weatherforecast", ...)) to restore the
endpoint; locate Program.cs and the WebApp.http file and make the change so the
test and the exposed route names match.
🧹 Nitpick comments (19)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (1)
19-50: AlignTestContextnullability with the null-guard.Line 19:
TestContextis non-nullable, yet guarded for null. Either make it nullable or drop the branch to reflect intent clearly.♻️ Optional refactor (if null is intended)
-public MockKubeApiServer(TestContext testOutput, Action<IEndpointRouteBuilder>? builder = null) +public MockKubeApiServer(TestContext? testOutput, Action<IEndpointRouteBuilder>? builder = null)Please confirm whether
TestContextcan be null in your TUnit usage.src/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetaData.cs (1)
10-17: Minor naming inconsistency (pre-existing).The interface uses
Metadata(line 10) while the class usesMetaData(line 16). Consider aligning these names in a future cleanup for consistency.src/K8sOperator.NET/Helpers/ConsoleHelpers.cs (1)
3-3: Public class but internal members may block external use.
If the goal is to expose these helpers, the fields should be public (or add public accessors). Otherwise consider keeping the class internal.♻️ Proposed adjustment (if external access is intended)
- internal static readonly string SPACE = string.Empty.PadRight(3); - internal static readonly string NL = Environment.NewLine; // shortcut - internal static readonly string NORMAL = Console.IsOutputRedirected ? "" : "\x1b[39m"; - internal static readonly string RED = Console.IsOutputRedirected ? "" : "\x1b[91m"; - internal static readonly string GREEN = Console.IsOutputRedirected ? "" : "\x1b[92m"; - internal static readonly string YELLOW = Console.IsOutputRedirected ? "" : "\x1b[93m"; - internal static readonly string BLUE = Console.IsOutputRedirected ? "" : "\x1b[94m"; - internal static readonly string MAGENTA = Console.IsOutputRedirected ? "" : "\x1b[95m"; - internal static readonly string CYAN = Console.IsOutputRedirected ? "" : "\x1b[96m"; - internal static readonly string GREY = Console.IsOutputRedirected ? "" : "\x1b[97m"; - internal static readonly string BOLD = Console.IsOutputRedirected ? "" : "\x1b[1m"; - internal static readonly string NOBOLD = Console.IsOutputRedirected ? "" : "\x1b[22m"; - internal static readonly string UNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[4m"; - internal static readonly string NOUNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[24m"; - internal static readonly string REVERSE = Console.IsOutputRedirected ? "" : "\x1b[7m"; - internal static readonly string NOREVERSE = Console.IsOutputRedirected ? "" : "\x1b[27m"; + public static readonly string SPACE = string.Empty.PadRight(3); + public static readonly string NL = Environment.NewLine; // shortcut + public static readonly string NORMAL = Console.IsOutputRedirected ? "" : "\x1b[39m"; + public static readonly string RED = Console.IsOutputRedirected ? "" : "\x1b[91m"; + public static readonly string GREEN = Console.IsOutputRedirected ? "" : "\x1b[92m"; + public static readonly string YELLOW = Console.IsOutputRedirected ? "" : "\x1b[93m"; + public static readonly string BLUE = Console.IsOutputRedirected ? "" : "\x1b[94m"; + public static readonly string MAGENTA = Console.IsOutputRedirected ? "" : "\x1b[95m"; + public static readonly string CYAN = Console.IsOutputRedirected ? "" : "\x1b[96m"; + public static readonly string GREY = Console.IsOutputRedirected ? "" : "\x1b[97m"; + public static readonly string BOLD = Console.IsOutputRedirected ? "" : "\x1b[1m"; + public static readonly string NOBOLD = Console.IsOutputRedirected ? "" : "\x1b[22m"; + public static readonly string UNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[4m"; + public static readonly string NOUNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[24m"; + public static readonly string REVERSE = Console.IsOutputRedirected ? "" : "\x1b[7m"; + public static readonly string NOREVERSE = Console.IsOutputRedirected ? "" : "\x1b[27m";src/K8sOperator.NET/K8sOperator.NET.csproj (2)
21-23: Consider adding package tags.
PackageTagsis empty. Adding relevant tags (e.g.,kubernetes,operator,crd,k8s,cloud-native) would improve discoverability on NuGet.
45-70: Duplicate packaging ofOperator.targets.
Operator.targetsis packed twice to thebuildfolder:
- As
Content(line 45-49) →\build\$(ProjectName).targets- As
None(line 66-69) →buildThis may cause redundant files or conflicts in the NuGet package. Consider keeping only one declaration.
♻️ Proposed fix - remove duplicate
- <!-- Auto-import for project references --> - <ItemGroup> - <None Include="Operator.targets"> - <Pack>true</Pack> - <PackagePath>build</PackagePath> - </None> - </ItemGroup>src/K8sOperator.NET/Metadata/FinalizerAttribute.cs (1)
6-10: Validate finalizer input to prevent invalid metadata.A null/whitespace finalizer would silently propagate; consider guarding at construction time.
🛠️ Proposed fix
public class FinalizerAttribute(string finalizer) : Attribute { public const string Default = "default"; - public string Finalizer { get; } = finalizer; + public string Finalizer { get; } = + string.IsNullOrWhiteSpace(finalizer) + ? throw new ArgumentException("Finalizer cannot be null or whitespace.", nameof(finalizer)) + : finalizer; public override string ToString() => DebuggerHelpers.GetDebugText(nameof(Finalizer), Finalizer); }src/K8sOperator.NET/Builder/ConventionBuilder.cs (1)
3-8: Guard against null conventions and null delegates.This avoids late NREs if the builder is constructed or used incorrectly.
🛠️ Proposed fix
-public class ConventionBuilder<T>(ICollection<Action<T>> conventions) +public class ConventionBuilder<T>(ICollection<Action<T>> conventions) { + private readonly ICollection<Action<T>> _conventions = + conventions ?? throw new ArgumentNullException(nameof(conventions)); + public void Add(Action<T> convention) { - conventions.Add(convention); + _conventions.Add(convention ?? throw new ArgumentNullException(nameof(convention))); } }src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (1)
5-8: Consider sealing this attribute class for consistency.
NamespaceAttributeis declared assealed, butDockerImageAttributeis not. Sealed attributes are a best practice in .NET for performance and design clarity.♻️ Suggested change
[AttributeUsage(AttributeTargets.Assembly)] -public class DockerImageAttribute(string registery, string repository, string tag) : Attribute +public sealed class DockerImageAttribute(string registry, string repository, string tag) : Attributesrc/K8sOperator.NET/Commands/InstallCommand.cs (1)
23-34: Potential multiple enumeration ofIEnumerable<IEventWatcher>.
watchersis obtained fromdataSource.GetWatchers()(which yields lazily per the relevant snippet) and is then iterated multiple times: once inCreateClusterRole(line 25), once in theforeachloop (line 29), and again inCreateClusterRoleinternally. This could cause the watchers to be built multiple times ifGetWatchers()performs non-trivial work.♻️ Suggested fix - materialize the enumerable
var dataSource = app.Services.GetRequiredService<EventWatcherDatasource>(); - var watchers = dataSource.GetWatchers(); + var watchers = dataSource.GetWatchers().ToList(); var clusterrole = CreateClusterRole(dataSource.Metadata, watchers);src/K8sOperator.NET/Metadata/OperatorNameAttribute.cs (1)
5-14: Minor inconsistencies with sibling attribute classes.
- This class is not
sealed, butNamespaceAttributeis sealed.OperatorNameis a get-only property (=>), whileDockerImageAttributeandNamespaceAttributeuse{ get; set; }.Consider aligning for consistency across the metadata attributes.
♻️ Suggested alignment
[AttributeUsage(AttributeTargets.Assembly)] -public class OperatorNameAttribute(string name) : Attribute +public sealed class OperatorNameAttribute(string name) : Attribute { public static OperatorNameAttribute Default => new("operator"); - public string OperatorName => name; + public string OperatorName { get; set; } = name; public override string ToString() => DebuggerHelpers.GetDebugText(nameof(OperatorName), OperatorName); }src/K8sOperator.NET/EventWatcherDatasource.cs (1)
61-69: Consider separating core interfaces into dedicated files.
IOperatorControlleris a fundamental interface used across the codebase. Having it inEventWatcherDatasource.csmay reduce discoverability. Consider moving it to its own file (e.g.,IOperatorController.cs).src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
34-42: Incomplete XML documentation.
<inheritdoc />on Lines 34 and 37 won't inherit anything meaningful since this extendsAttributedirectly. The empty<summary>on Lines 39-41 should be completed or removed.Proposed fix
- /// <inheritdoc /> + /// <summary> + /// Gets or sets the command argument name. + /// </summary> public string Argument { get; set; } = argument; - /// <inheritdoc /> + /// <summary> + /// Gets or sets the command description. + /// </summary> public string Description { get; set; } = string.Empty; - /// <summary> - /// - /// </summary> + /// <summary> + /// Gets or sets the display order for the command. + /// </summary> public int Order { get; set; } = 1;src/K8sOperator.NET/OperatorExtensions.cs (1)
105-108:OperatorBuilderis empty.The class is instantiated and passed to the
configurecallback but has no members. If this is a placeholder for future extensibility, consider adding a brief comment or TODO indicating planned functionality.src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
29-29: Redundant property initializer.The
= []initializer is redundant since the constructor always assigns themetadataparameter toMetadata. Field initializers execute before the constructor body, so this empty list is immediately overwritten.🔧 Suggested fix
- public List<object> Metadata { get; } = []; + public List<object> Metadata { get; }src/K8sOperator.NET/Generation/MetadataExtensions.cs (2)
3-3: Redundant using directive.The
using K8sOperator.NET.Generation;is unnecessary since this file is already in theK8sOperator.NET.Generationnamespace.🔧 Suggested fix
using k8s.Models; using K8sOperator.NET.Builder; -using K8sOperator.NET.Generation; using K8sOperator.NET.Metadata;
97-149: Incomplete XML documentation.The new extension methods
RemoveMetadata,WithMetadata, andWithSinglehave empty XML documentation comments. Consider adding descriptions for parameters and return values to maintain API documentation consistency with existing methods.src/K8sOperator.NET/EventWatcher.cs (2)
88-101: Fire-and-forget async may silently lose exceptions.The
OnEventmethod usesContinueWithfor fire-and-forget processing. While it logs faulted tasks, the continuation itself runs on a thread pool thread and any exception in the logging code would be unobserved. Consider using a safer pattern or ensure the continuation is fully resilient.Additionally,
ContinueWithdoesn't preserve the synchronization context by default. For consistency, you might want to specifyTaskScheduler.Defaultexplicitly.
252-276: Inconsistent use ofmetadatavsMetadataproperty.Lines 254 and 280 use the constructor parameter
metadatadirectly, while line 321 (Finalizerproperty) usesMetadata. Both refer to the same list, but using the property consistently would be clearer and more maintainable.src/K8sOperator.NET/CommandDatasource.cs (1)
24-29: Order is hardcoded to 1 for all commands.The
Orderproperty is always set to1, which means command ordering via_commands.OrderBy(x => x.Order)at line 35 has no effect. If ordering was meaningful before, this change removes that functionality.Consider either:
- Removing the
Orderproperty andOrderBycall if ordering is no longer needed- Adding an optional
orderparameter toAdd<TCommand>()if ordering should be configurable
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (101)
Directory.Packages.propsK8sOperator.NET.slnxK8sOperator.slnsrc/Directory.Build.propssrc/K8sOperator.NET.Generators/K8sOperator.NET.Generators.csprojsrc/K8sOperator.NET.Generators/OperatorApplicationExtensions.cssrc/K8sOperator.NET.Generators/Utilities.cssrc/K8sOperator.NET/Builder/CommandBuilder.cssrc/K8sOperator.NET/Builder/ControllerBuilder.cssrc/K8sOperator.NET/Builder/ControllerConventionBuilder.cssrc/K8sOperator.NET/Builder/ConventionBuilder.cssrc/K8sOperator.NET/Builder/EventWatcherBuilder.cssrc/K8sOperator.NET/Builder/KubernetesBuilder.cssrc/K8sOperator.NET/Builder/LoggingBuilder.cssrc/K8sOperator.NET/Builder/OperatorCommandBuilder.cssrc/K8sOperator.NET/Builder/OperatorCommandConventionBuilder.cssrc/K8sOperator.NET/Builder/OperatorHostBuilder.cssrc/K8sOperator.NET/ChangeTracker.cssrc/K8sOperator.NET/CommandDatasource.cssrc/K8sOperator.NET/Commands/HelpCommand.cssrc/K8sOperator.NET/Commands/InstallCommand.cssrc/K8sOperator.NET/Commands/OperatorCommand.cssrc/K8sOperator.NET/Commands/VersionCommand.cssrc/K8sOperator.NET/ControllerDatasource.cssrc/K8sOperator.NET/CustomResource.cssrc/K8sOperator.NET/EventWatcher.cssrc/K8sOperator.NET/EventWatcherDatasource.cssrc/K8sOperator.NET/Extensions/ControllerConventionBuilderExtensions.cssrc/K8sOperator.NET/Extensions/KubernetesBuilderExtensions.cssrc/K8sOperator.NET/Extensions/OperatorHostBuilderExtensions.cssrc/K8sOperator.NET/Generation/ClusterRoleBindingBuilder.cssrc/K8sOperator.NET/Generation/ClusterRoleBindingBuilderExtensions.cssrc/K8sOperator.NET/Generation/ClusterRoleBuilder.cssrc/K8sOperator.NET/Generation/ClusterRoleBuilderExtensions.cssrc/K8sOperator.NET/Generation/ContainerBuilder.cssrc/K8sOperator.NET/Generation/CustomResourceDefinitionBuilder.cssrc/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cssrc/K8sOperator.NET/Generation/DeploymentBuilder.cssrc/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cssrc/K8sOperator.NET/Generation/EntityScope.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilder.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilderExtentions.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetaData.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetadataExtentions.cssrc/K8sOperator.NET/Generation/MetadataExtensions.cssrc/K8sOperator.NET/Generation/PolicyRuleBuilder.cssrc/K8sOperator.NET/Generation/PolicyRuleBuilderExtensions.cssrc/K8sOperator.NET/Helpers/ConsoleHelpers.cssrc/K8sOperator.NET/Helpers/DebuggerHelpers.cssrc/K8sOperator.NET/Helpers/Utilities.cssrc/K8sOperator.NET/K8sOperator.NET.csprojsrc/K8sOperator.NET/KubernetesClient.cssrc/K8sOperator.NET/LoggerExtentions.cssrc/K8sOperator.NET/Metadata/DescriptionMetadata.cssrc/K8sOperator.NET/Metadata/DockerImageAttribute.cssrc/K8sOperator.NET/Metadata/FinalizerAttribute.cssrc/K8sOperator.NET/Metadata/FinalizerMetadata.cssrc/K8sOperator.NET/Metadata/ImageRepositoryMetadata.cssrc/K8sOperator.NET/Metadata/LabelSelectorAttribute.cssrc/K8sOperator.NET/Metadata/LabelSelectorMetadata.cssrc/K8sOperator.NET/Metadata/NamespaceAttribute.cssrc/K8sOperator.NET/Metadata/NamespaceMetadata.cssrc/K8sOperator.NET/Metadata/OperatorArgumentMetadata.cssrc/K8sOperator.NET/Metadata/OperatorNameAttribute.cssrc/K8sOperator.NET/Metadata/OperatorNameMetadata.cssrc/K8sOperator.NET/Operator.cssrc/K8sOperator.NET/Operator.targetssrc/K8sOperator.NET/OperatorController.cssrc/K8sOperator.NET/OperatorException.cssrc/K8sOperator.NET/OperatorExtensions.cssrc/K8sOperator.NET/OperatorHost.cssrc/K8sOperator.NET/OperatorService.cstest/Directory.Build.propstest/K8sOperator.NET.Generators.Tests/K8sOperator.NET.Generators.Tests.csprojtest/K8sOperator.NET.Generators.Tests/UnitTest1.cstest/K8sOperator.NET.Tests/ControllerDatasourceTests.cstest/K8sOperator.NET.Tests/ControllerTests.cstest/K8sOperator.NET.Tests/EntityScope.cstest/K8sOperator.NET.Tests/EventWatcherTests.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojtest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/ChangeTracking_Tests.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Fixtures/TestResource.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/GlobalSetup.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojtest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLogger.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests/OperatorExtensions_Tests.cstest/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/ListNamespacedCustomObjectWithHttpMessagesAsync.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/ReplaceNamespacedCustomObjectAsync.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cstest/K8sOperator.NET.Tests/MyController.cstest/K8sOperator.NET.Tests/OperatorTests.cstest/K8sOperator.NET.Tests/WebApp/Program.cstest/K8sOperator.NET.Tests/WebApp/Properties/launchSettings.jsontest/K8sOperator.NET.Tests/WebApp/WebApp.csprojtest/K8sOperator.NET.Tests/WebApp/WebApp.httptest/K8sOperator.NET.Tests/WebApp/appsettings.Development.jsontest/K8sOperator.NET.Tests/WebApp/appsettings.json
💤 Files with no reviewable changes (41)
- test/K8sOperator.NET.Tests/MyController.cs
- test/K8sOperator.NET.Generators.Tests/K8sOperator.NET.Generators.Tests.csproj
- test/K8sOperator.NET.Tests/ControllerTests.cs
- src/K8sOperator.NET.Generators/OperatorApplicationExtensions.cs
- Directory.Packages.props
- src/K8sOperator.NET/Extensions/KubernetesBuilderExtensions.cs
- src/K8sOperator.NET.Generators/Utilities.cs
- test/K8sOperator.NET.Tests/Mocks/Endpoints/ListNamespacedCustomObjectWithHttpMessagesAsync.cs
- src/K8sOperator.NET/Metadata/OperatorNameMetadata.cs
- test/K8sOperator.NET.Tests/ControllerDatasourceTests.cs
- test/K8sOperator.NET.Tests/Mocks/Endpoints/ReplaceNamespacedCustomObjectAsync.cs
- src/K8sOperator.NET/Metadata/OperatorArgumentMetadata.cs
- src/K8sOperator.NET/Metadata/DescriptionMetadata.cs
- src/K8sOperator.NET/OperatorHost.cs
- src/K8sOperator.NET/Metadata/LabelSelectorMetadata.cs
- src/K8sOperator.NET/Extensions/OperatorHostBuilderExtensions.cs
- src/K8sOperator.NET/Builder/OperatorCommandConventionBuilder.cs
- src/K8sOperator.NET/Builder/OperatorCommandBuilder.cs
- test/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cs
- src/K8sOperator.NET.Generators/K8sOperator.NET.Generators.csproj
- test/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cs
- src/K8sOperator.NET/Builder/OperatorHostBuilder.cs
- src/Directory.Build.props
- test/K8sOperator.NET.Tests/EventWatcherTests.cs
- src/K8sOperator.NET/ControllerDatasource.cs
- src/K8sOperator.NET/Helpers/Utilities.cs
- test/K8sOperator.NET.Generators.Tests/UnitTest1.cs
- src/K8sOperator.NET/Metadata/ImageRepositoryMetadata.cs
- test/K8sOperator.NET.Tests/OperatorTests.cs
- src/K8sOperator.NET/Operator.cs
- src/K8sOperator.NET/KubernetesClient.cs
- test/Directory.Build.props
- src/K8sOperator.NET/Builder/KubernetesBuilder.cs
- src/K8sOperator.NET/Builder/ControllerConventionBuilder.cs
- src/K8sOperator.NET/Metadata/FinalizerMetadata.cs
- src/K8sOperator.NET/Extensions/ControllerConventionBuilderExtensions.cs
- src/K8sOperator.NET/Builder/LoggingBuilder.cs
- src/K8sOperator.NET/Metadata/NamespaceMetadata.cs
- K8sOperator.sln
- test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj
- test/K8sOperator.NET.Tests/EntityScope.cs
🧰 Additional context used
🧬 Code graph analysis (20)
src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs (3)
src/K8sOperator.NET.Generators/Builders/DeploymentBuilderExtensions.cs (6)
DeploymentBuilderExtensions(8-442)x(436-439)x(361-361)x(185-185)x(317-317)x(414-422)src/K8sOperator.NET.Generators/Builders/KubernetesObjectBuilderExtentions.cs (1)
KubernetesObjectBuilderExtentions(9-63)src/K8sOperator.NET.Generators/Builders/CustomResourceDefinitionBuilderExtensions.cs (1)
CustomResourceDefinitionBuilderExtensions(14-400)
src/K8sOperator.NET/Metadata/NamespaceAttribute.cs (2)
examples/SimpleOperator/Projects/ProjectController.cs (1)
Namespace(6-36)src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLogger.cs (1)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs (1)
ILogger(58-58)
src/K8sOperator.NET/Commands/HelpCommand.cs (5)
src/K8sOperator.NET/Helpers/ConsoleHelpers.cs (1)
ConsoleHelpers(3-21)src/K8sOperator.NET/Commands/OperatorCommand.cs (2)
OperatorArgument(7-14)Task(10-13)src/K8sOperator.NET/Commands/VersionCommand.cs (2)
OperatorArgument(10-23)Task(13-22)src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
IOperatorCommand(15-23)src/K8sOperator.NET/CommandDatasource.cs (1)
CommandDatasource(7-49)
src/K8sOperator.NET/OperatorExtensions.cs (3)
src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (1)
IKubernetes(65-69)src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
IOperatorController(19-28)
src/K8sOperator.NET/EventWatcherDatasource.cs (2)
src/K8sOperator.NET/Builder/EventWatcherBuilder.cs (1)
IEventWatcher(23-28)src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
IOperatorController(19-28)
src/K8sOperator.NET/CommandDatasource.cs (2)
src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
CommandBuilder(9-24)IOperatorCommand(15-23)src/K8sOperator.NET/Builder/ConventionBuilder.cs (2)
ConventionBuilder(3-9)Add(5-8)
src/K8sOperator.NET/Metadata/LabelSelectorAttribute.cs (1)
src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
src/K8sOperator.NET/OperatorController.cs (1)
src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
IOperatorController(19-28)
src/K8sOperator.NET/EventWatcher.cs (3)
src/K8sOperator.NET/OperatorController.cs (1)
OperatorController(7-69)src/K8sOperator.NET/Builder/EventWatcherBuilder.cs (1)
IEventWatcher(23-28)src/K8sOperator.NET/ChangeTracker.cs (2)
TrackResourceGenerationAsHandled(16-22)ChangeTracker(3-28)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/OperatorExtensions_Tests.cs (2)
src/K8sOperator.NET/CommandDatasource.cs (1)
CommandDatasource(7-49)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)
src/K8sOperator.NET/Builder/EventWatcherBuilder.cs (2)
src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
IOperatorController(19-28)src/K8sOperator.NET/EventWatcher.cs (1)
EventWatcher(12-324)
src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (1)
src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/ChangeTracking_Tests.cs (1)
src/K8sOperator.NET/ChangeTracker.cs (1)
ChangeTracker(3-28)
src/K8sOperator.NET/Builder/ConventionBuilder.cs (1)
src/K8sOperator.NET/CommandDatasource.cs (1)
ConventionBuilder(20-31)
src/K8sOperator.NET/Commands/InstallCommand.cs (4)
src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
IOperatorCommand(15-23)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilder.cs (1)
CustomResourceDefinitionBuilder(6-14)src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (1)
GetImage(13-13)
src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
src/K8sOperator.NET/Commands/OperatorCommand.cs (3)
src/K8sOperator.NET/Commands/HelpCommand.cs (2)
OperatorArgument(9-34)Task(12-33)src/K8sOperator.NET/Commands/VersionCommand.cs (2)
OperatorArgument(10-23)Task(13-22)src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
IOperatorCommand(15-23)
src/K8sOperator.NET/Metadata/OperatorNameAttribute.cs (1)
src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
src/K8sOperator.NET/OperatorService.cs (3)
src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)src/K8sOperator.NET/EventWatcher.cs (11)
Task(24-86)Task(103-130)Task(132-141)Task(143-154)Task(156-177)Task(179-196)Task(198-202)Task(204-210)Task(213-221)Task(226-250)Task(252-276)src/K8sOperator.NET/OperatorException.cs (1)
OperatorException(3-6)
⏰ 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 (59)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/GlobalSetup.cs (1)
1-4: Looks good for test-assembly coverage exclusion.Assembly-level exclusion is appropriate for test projects and keeps coverage reporting focused on production code.
test/K8sOperator.NET.Tests/WebApp/Properties/launchSettings.json (1)
1-41: LGTM — standard launch settings configuration.
No concerns with the profiles or IIS settings as defined.src/K8sOperator.NET/LoggerExtentions.cs (1)
2-2: LGTM!The import is required for the
CustomResourcetype used in the logging methods.src/K8sOperator.NET/Generation/PolicyRuleBuilderExtensions.cs (1)
3-3: No breaking changes detected — namespace refactoring is complete.The old namespace
K8sOperator.NET.Generators.Buildershas no references in the codebase and appears to be an incorrect assumption about the previous namespace. All internal code already uses the newK8sOperator.NET.Generationnamespace consistently. No compatibility shims or type-forwarding are needed.test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (2)
1-1: No review needed for this cosmetic change.
59-62: Property formatting looks good.test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/ChangeTracking_Tests.cs (2)
10-121: The code is correct. The project uses TUnit (version 1.11.28), which fully supports async assertions with theawait Assert.That(...).IsFalse()/IsTrue()pattern. No changes needed.Likely an incorrect or invalid review comment.
6-9: This review comment is incorrect. TUnit creates a new instance of the test class for each test by default, so the_changeTrackerfield is not shared between tests. Each test receives its own fresh instance, eliminating any state leakage or test order dependency. No changes are needed.Likely an incorrect or invalid review comment.
src/K8sOperator.NET/ChangeTracker.cs (1)
3-26: This concern is not applicable to the current code.
ChangeTrackeris instantiated as a private field within eachEventWatcher<T>instance, so there is no shared state across watchers. Events are processed sequentially within each watcher's event loop (await foreachin thewhile (_isRunning && !_cancellationToken.IsCancellationRequested)block), meaning concurrent access to a singleChangeTrackerinstance does not occur. The isolation-by-design pattern makes a thread-safe collection unnecessary here.Likely an incorrect or invalid review comment.
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLogger.cs (1)
53-60: NullScope + TestContext output wiring looks good.No concerns with the no-op scope implementation or the switch to
Context.Output.WriteLine(...).Also applies to: 79-88
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs (1)
21-29: Constructor validation + CreateLogger wiring looks solid.The provider cleanly validates
testContextand createsTestContextLoggerwith the expected inputs.Also applies to: 58-58
test/K8sOperator.NET.Tests/WebApp/appsettings.Development.json (1)
1-7: LGTM: sensible dev logging defaults.Clear, minimal logging config for the test WebApp.
src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilder.cs (1)
4-4: LGTM: clean namespace move.No functional impact in this file.
src/K8sOperator.NET/Generation/KubernetesObjectBuilderExtentions.cs (1)
1-5: LGTM: namespace alignment looks good.The refactor is consistent with the new Generation layout.
test/K8sOperator.NET.Tests/WebApp/appsettings.json (1)
1-9: LGTM: standard test appsettings.Logging and
AllowedHostslook appropriate for the test WebApp.test/K8sOperator.NET.Tests/WebApp/WebApp.csproj (1)
4-10: No action required. The configuration is correct:global.jsonpins .NET 10 SDK (10.0.100), andMicrosoft.AspNetCore.OpenApi10.0.2 is compatible withnet10.0. The new test project will build successfully.test/K8sOperator.NET.Tests/WebApp/Program.cs (1)
1-13: LGTM!Clean minimal API setup for test scaffolding. The
public partial class Programdeclaration correctly enablesWebApplicationFactory<Program>usage in integration tests.src/K8sOperator.NET/Generation/PolicyRuleBuilder.cs (1)
3-8: LGTM!Namespace migration to
K8sOperator.NET.Generationis consistent with the broader refactoring effort.src/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetaData.cs (1)
4-23: LGTM!Namespace migration is consistent with the refactoring effort. The interface/implementation pattern with public interface and internal class provides good encapsulation.
K8sOperator.NET.slnx (1)
6-8: The test project path is correct; verify whether WebApp should be included in the solution.The test project path
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojis the actual directory structure and is correctly referenced in the solution file.However, the WebApp project exists at
test/K8sOperator.NET.Tests/WebApp/WebApp.csprojbut is not included in the solution. Verify if this should be added.src/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetadataExtentions.cs (1)
4-4: Namespace update looks consistent.
No behavior change detected.src/K8sOperator.NET/CustomResource.cs (1)
5-5: Namespace realignment is straightforward.
No behavioral changes observed.src/K8sOperator.NET/Generation/EntityScope.cs (1)
1-1: Namespace update looks good.src/K8sOperator.NET/OperatorException.cs (1)
3-3: No action required. The project targetsnet10.0, which defaults to C# 13, fully supporting primary constructor syntax. The code will compile without issues.test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Fixtures/TestResource.cs (1)
1-17: LGTM!Well-structured test fixture with proper
KubernetesEntityattribute configuration. The nestedTestSpecandTestStatusclasses follow the expected CRD pattern.src/K8sOperator.NET/OperatorController.cs (2)
1-9: LGTM!The rename from
Controller<T>toOperatorController<T>improves clarity and theIOperatorControllerinterface enables polymorphic handling inControllerBuilder.Build(). Good API design.
54-57: No substantive change.The
BookmarkAsyncmethod signature and implementation remain unchanged; only annotation metadata differs.src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (2)
1-6: LGTM!Namespace relocation to
K8sOperator.NET.Generationand the new imports align with the broader architectural refactoring.
405-419: LGTM!The
IsNullableextension methods properly useNullabilityInfoContextfor accurate nullability detection of reference types with C# nullable annotations, with a fallback forNullable<T>value types.src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (1)
1-7: Formatting-only change.No functional changes; just a blank line added after the namespace declaration.
src/K8sOperator.NET/K8sOperator.NET.csproj (1)
4-11: Ensure your CI/CD pipeline and consumers support .NET 10.The project targets
net10.0, which is .NET 10 (released November 2025, LTS through November 2028). Verify that your build environment has the .NET 10 SDK installed and that Visual Studio 2026 or compatible tooling is in place. Confirm that any downstream consumers or deployment targets support this framework version.src/K8sOperator.NET/Generation/ClusterRoleBuilderExtensions.cs (1)
3-3: Namespace migration looks good.The namespace change from
K8sOperator.NET.Generators.BuilderstoK8sOperator.NET.Generationis consistent with the broader PR refactoring.src/K8sOperator.NET/Generation/ClusterRoleBuilder.cs (1)
5-14: Namespace migration and Build() implementation look correct.The namespace change aligns with the broader refactoring, and the
Build()override correctly initializes theV1ClusterRolebefore returning.src/K8sOperator.NET/Generation/ClusterRoleBindingBuilder.cs (1)
4-14: LGTM!The namespace migration is consistent with the broader refactoring. The
Build()implementation correctly follows the same pattern asClusterRoleBuilder.src/K8sOperator.NET/Generation/ContainerBuilder.cs (1)
3-7: LGTM!The namespace migration is consistent. The empty class body is acceptable as the base
KubernetesObjectBuilder<V1Container>provides the necessary builder functionality.src/K8sOperator.NET/Generation/ClusterRoleBindingBuilderExtensions.cs (2)
3-3: Namespace migration is consistent with the broader refactoring.
20-51: Extension methods are well-implemented.Both
WithRoleRefandWithSubjectmethods correctly implement the fluent builder pattern with proper generic constraints. The null-coalescing assignment forSubjectscollection initialization is a good defensive pattern.src/K8sOperator.NET/Generation/KubernetesObjectBuilder.cs (1)
1-1: Namespace update is consistent.Line 1 aligns the builder with the new
K8sOperator.NET.Generationnamespace. No functional concerns here.src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs (1)
3-3: Namespace update is consistent.Line 3 aligns
DeploymentBuilderExtensionswith the Generation namespace refactor.src/K8sOperator.NET/Generation/DeploymentBuilder.cs (1)
4-4: No action needed—DeploymentBuilder is correctly in theK8sOperator.NET.Generationnamespace and all references are properly updated. The namespaceK8sOperator.NET.Generators.Buildersdoes not exist in the codebase, so there is no breaking change concern or migration path needed.Likely an incorrect or invalid review comment.
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj (1)
7-12: SDK and package compatibility are configured correctly.The repository correctly pins .NET 10.0.100 in
global.json, andMicrosoft.AspNetCore.Mvc.Testing10.0.2 explicitly targetsnet10.0. Recent versions ofTUnitsupportnet10.0; if tests encounter failures related to "Testing with VSTest target is no longer supported on .NET 10 SDK", add theTestingPlatformDotnetTestSupportproperty to the project file:<PropertyGroup> <TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport> </PropertyGroup>src/K8sOperator.NET/Commands/OperatorCommand.cs (1)
8-12: LGTM — clean delegation to the host run loop.src/K8sOperator.NET/Metadata/NamespaceAttribute.cs (1)
5-14: LGTM!The attribute is well-structured with proper
sealedmodifier, correctAttributeUsagetargeting both Assembly and Class, and consistent use ofDebuggerHelpersfor theToString()override.src/K8sOperator.NET/Commands/InstallCommand.cs (2)
49-76: LGTM!The
CreateCustomResourceDefinitionmethod correctly uses the newNamespaceAttributetype and properly appliesToLower()to resource names for Kubernetes naming convention compliance.
78-137: LGTM!The metadata retrieval correctly uses the new attribute types (
OperatorNameAttribute,DockerImageAttribute,NamespaceAttribute) with theTryGetValueextension method pattern.src/K8sOperator.NET/Operator.targets (1)
5-6: LGTM on the newOperatorNamespaceproperty.The addition of a separate
OperatorNamespaceproperty with a sensible default provides better configurability and separates the operator name from its deployment namespace.src/K8sOperator.NET/EventWatcherDatasource.cs (1)
5-11: LGTM - Clean interface definition.The
IEventWatcherinterface is well-designed with clear responsibilities: lifecycle management viaStart, metadata exposure, and controller access.src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
1-1: SameFrontman.Operatorimport concern.As noted in
HelpCommand.cs, this import should be verified. IfIOperatorCommandis defined locally in this file (Line 26), this import may be unnecessary or conflicting.
15-23: LGTM - Build logic is sound.The fallback derivation from
CommandType.Name(removing "Command" suffix and lowercasing) provides sensible defaults. Note thatBuildhas a side effect of mutatingMetadata, which is expected based on the usage pattern inCommandDatasource.src/K8sOperator.NET/Builder/EventWatcherBuilder.cs (2)
5-21: LGTM - Clean factory pattern implementation.The private constructor with public static
Createfactory ensures controlled instantiation. The design aligns well with the builder pattern used throughout the codebase.
23-28: Verify type compatibility:IOperatorControllercannot be directly passed whereOperatorController<T>is expected.The
Build()method passesController(declared asIOperatorController) toActivatorUtilities.CreateInstance, butEventWatcher<T>expectsOperatorController<T>in its constructor (line 14). WhileOperatorController<T>implementsIOperatorController, the parameter type mismatch may causeActivatorUtilitiesto fail to find a matching constructor or cause a runtime type error. CastControllerto the concrete type or modifyEventWatcher<T>to acceptIOperatorControllerif design permits.src/K8sOperator.NET/OperatorExtensions.cs (2)
22-66: LGTM - Service registration is well-structured.The singleton registrations for
CommandDatasource,EventWatcherDatasource,IKubernetes, andOperatorServiceare appropriate. The fallback to default attributes whenGetEntryAssembly()returns null is a good defensive pattern.
15-17: > Likely an incorrect or invalid review comment.src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
5-28: LGTM!The refactored
ControllerBuilderwith a private constructor and static factory method is a clean design. TheBuild()method correctly usesActivatorUtilities.CreateInstanceand aggregates metadata from both the controller type and its resource type via reflection.test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/OperatorExtensions_Tests.cs (1)
10-73: Good test coverage for the extension method.The tests comprehensively cover null handling, service registrations (OperatorService, CommandDatasource, EventWatcherDatasource, IKubernetes), and the configure action callback.
src/K8sOperator.NET/Generation/MetadataExtensions.cs (1)
90-95: Verify:WithFinalizeruses additive semantics unlike other methods.
WithFinalizercallsWithMetadata(additive), whileWithGroup,ForNamespace, andWithLabeluseWithSingle(replace). This means callingWithFinalizermultiple times will accumulate finalizers, whereas the others replace existing attributes.Is this intentional to support multiple finalizers on a single controller?
src/K8sOperator.NET/CommandDatasource.cs (2)
1-1: Verify:Frontman.Operatornamespace import.Same as in
EventWatcher.cs- this import appears to reference an external namespace. Verify this is the correct import.
33-48: LGTM!The
GetCommandsmethod cleanly implements the builder pattern, applying conventions and yieldingCommandInforecords. The iteration and building logic is correct.src/K8sOperator.NET/EventWatcher.cs (1)
1-1: Theusing Frontman.Operator;import is correct.Frontman.Operatoris an internal namespace defined within this project (in LoggerExtentions.cs), not an external or unrelated import. This usage is consistent across multiple files in the codebase.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| using Frontman.Operator; | ||
| using K8sOperator.NET.Builder; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.Extensions.Hosting; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Frontman namespace definitions in the codebase
echo "=== Searching for Frontman.Operator namespace definition ==="
rg -n "namespace Frontman" --type cs
echo -e "\n=== Searching for Frontman in .csproj files ==="
rg -n "Frontman" -g "*.csproj"
echo -e "\n=== Searching for Frontman references in .cs files ==="
rg -n "Frontman" --type cs | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Check CommandBuilder.cs for IOperatorCommand definition
echo "=== Searching for IOperatorCommand definition ==="
rg -n "interface IOperatorCommand\|class.*IOperatorCommand" --type cs -A 2
echo -e "\n=== Checking CommandBuilder.cs file ==="
fd -n "CommandBuilder.cs" -x cat -n {}Repository: pmdevers/K8sOperator.NET
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Read HelpCommand.cs to see what it actually uses
echo "=== Reading HelpCommand.cs content ==="
cat -n src/K8sOperator.NET/Commands/HelpCommand.csRepository: pmdevers/K8sOperator.NET
Length of output: 1698
🏁 Script executed:
#!/bin/bash
# Search for IOperatorCommand interface definition
echo "=== Searching for IOperatorCommand interface ==="
rg -n "interface IOperatorCommand" --type cs -B 2 -A 5
echo -e "\n=== Checking what's in Frontman.Operator namespace ==="
cat -n src/K8sOperator.NET/LoggerExtentions.cs
echo -e "\n=== Searching for OperatorArgumentAttribute ==="
rg -n "class OperatorArgumentAttribute\|record OperatorArgumentAttribute" --type cs -A 3Repository: pmdevers/K8sOperator.NET
Length of output: 8943
🏁 Script executed:
#!/bin/bash
# Search for OperatorArgumentAttribute definition
echo "=== Searching for OperatorArgumentAttribute ==="
rg -n "OperatorArgumentAttribute" --type cs -B 2 -A 10 | head -40
echo -e "\n=== Check CommandBuilder.cs entirely ==="
cat -n src/K8sOperator.NET/Builder/CommandBuilder.csRepository: pmdevers/K8sOperator.NET
Length of output: 5093
🏁 Script executed:
#!/bin/bash
# Check CommandDatasource definition to see if it's in Frontman.Operator namespace
echo "=== Searching for CommandDatasource definition ==="
rg -n "class CommandDatasource\|record CommandDatasource" --type cs -B 3 -A 10
echo -e "\n=== Check all usages of Frontman.Operator imports in files ==="
rg -l "using Frontman.Operator" --type cs | while read file; do
echo "=== File: $file ==="
head -15 "$file"
echo ""
done | head -100Repository: pmdevers/K8sOperator.NET
Length of output: 2967
🏁 Script executed:
#!/bin/bash
# Verify if Frontman.Operator's LoggerExtensions are actually used in HelpCommand.cs
echo "=== Checking if LoggerExtensions methods are used in HelpCommand.cs ==="
cat src/K8sOperator.NET/Commands/HelpCommand.cs
echo -e "\n=== Checking if LoggerExtensions methods are used in CommandBuilder.cs ==="
cat src/K8sOperator.NET/Builder/CommandBuilder.cs
echo -e "\n=== Search for actual usage of Frontman.Operator in HelpCommand context ==="
rg -n "StartOperator|NoWatchers|LogWatchError|StopOperator|BeginWatch|EndWatch|EventReceived|ProcessEventError|HandleDelete|BeginDelete|SkipDelete|EndDelete|ReplaceResource|HandleAddOrModify|AddFinalizer|SkipAddOrModify|EndAddOrModify|HandleFinalize|SkipFinalize|BeginFinalize|EndFinalize|RemoveFinalizer|HandleBookmark|BeginBookmark|EndBookmark|HandleError|BeginError|EndError|WatcherError|WatchAsync" src/K8sOperator.NET/Commands/HelpCommand.cs
echo -e "\n=== Search for any logger reference in HelpCommand.cs ==="
rg -n "ILogger|Logger|logger" src/K8sOperator.NET/Commands/HelpCommand.csRepository: pmdevers/K8sOperator.NET
Length of output: 3061
Remove the unused Frontman.Operator import.
The Frontman.Operator namespace (which contains logger extensions) is not used in this file. The class only depends on IOperatorCommand and OperatorArgumentAttribute, both from K8sOperator.NET.Builder. Remove the unused import from line 1.
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/HelpCommand.cs` around lines 1 - 4, Remove the
unused Frontman.Operator using directive at the top of HelpCommand.cs; the
HelpCommand class only relies on K8sOperator.NET.Builder types (IOperatorCommand
and OperatorArgumentAttribute), so delete the Frontman.Operator import to clean
up unused references and ensure no other code in HelpCommand.cs depends on that
namespace.
| public static class TestContextLoggingExtensions | ||
| { | ||
| extension(ILoggingBuilder logging) | ||
| { | ||
| /// <summary> | ||
| /// Log to test output. | ||
| /// </summary> | ||
| /// <param name="logging"> | ||
| /// The global logging configuration. | ||
| /// </param> | ||
| /// <param name="testContext"> | ||
| /// Output for the current test. | ||
| /// </param> | ||
| /// <param name="minLogLevel"> | ||
| /// The minimum level to log at. | ||
| /// </param> | ||
| public void AddTestLogging(TestContext testContext, | ||
| LogLevel minLogLevel = LogLevel.Information) | ||
| { | ||
| if (logging == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(logging)); | ||
| } | ||
|
|
||
| if (testContext == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(testContext)); | ||
| } | ||
|
|
||
| logging.AddProvider( | ||
| new TestContextLoggerProvider(testContext, minLogLevel)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.csproj" -o -name "Directory.Build.props" -o -name "Directory.Build.targets" \) | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 290
🏁 Script executed:
rg -n "LangVersion|LanguageVersion" -g "*.csproj" -g "*.props" -g "*.targets" -B 1 -A 1Repository: pmdevers/K8sOperator.NET
Length of output: 50
🏁 Script executed:
cat test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj 2>/dev/null || find . -name "*Tests.csproj" -type f | head -1 | xargs catRepository: pmdevers/K8sOperator.NET
Length of output: 598
🏁 Script executed:
cat ./Directory.Build.props 2>/dev/null || echo "No Directory.Build.props at root"Repository: pmdevers/K8sOperator.NET
Length of output: 100
🏁 Script executed:
cat -n test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cs 2>/dev/null | head -50Repository: pmdevers/K8sOperator.NET
Length of output: 1532
Remove extension block syntax—this code will not compile.
The extension(ILoggingBuilder logging) syntax at line 10 is a C# 13 preview feature. The project configuration has no EnablePreviewFeatures property set, so this code cannot compile. Replace with a standard extension method:
Corrected implementation
public static class TestContextLoggingExtensions
{
- extension(ILoggingBuilder logging)
- {
- public void AddTestLogging(TestContext testContext,
- LogLevel minLogLevel = LogLevel.Information)
- {
- if (logging == null)
- {
- throw new ArgumentNullException(nameof(logging));
- }
-
- if (testContext == null)
- {
- throw new ArgumentNullException(nameof(testContext));
- }
-
- logging.AddProvider(
- new TestContextLoggerProvider(testContext, minLogLevel));
- }
- }
+ public static ILoggingBuilder AddTestLogging(
+ this ILoggingBuilder logging,
+ TestContext testContext,
+ LogLevel minLogLevel = LogLevel.Information)
+ {
+ if (logging == null)
+ {
+ throw new ArgumentNullException(nameof(logging));
+ }
+
+ if (testContext == null)
+ {
+ throw new ArgumentNullException(nameof(testContext));
+ }
+
+ logging.AddProvider(
+ new TestContextLoggerProvider(testContext, minLogLevel));
+ return logging;
+ }
}Also update the class summary at line 6 to reference TestContext instead of Xunit.
📝 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.
| public static class TestContextLoggingExtensions | |
| { | |
| extension(ILoggingBuilder logging) | |
| { | |
| /// <summary> | |
| /// Log to test output. | |
| /// </summary> | |
| /// <param name="logging"> | |
| /// The global logging configuration. | |
| /// </param> | |
| /// <param name="testContext"> | |
| /// Output for the current test. | |
| /// </param> | |
| /// <param name="minLogLevel"> | |
| /// The minimum level to log at. | |
| /// </param> | |
| public void AddTestLogging(TestContext testContext, | |
| LogLevel minLogLevel = LogLevel.Information) | |
| { | |
| if (logging == null) | |
| { | |
| throw new ArgumentNullException(nameof(logging)); | |
| } | |
| if (testContext == null) | |
| { | |
| throw new ArgumentNullException(nameof(testContext)); | |
| } | |
| logging.AddProvider( | |
| new TestContextLoggerProvider(testContext, minLogLevel)); | |
| } | |
| } | |
| public static class TestContextLoggingExtensions | |
| { | |
| /// <summary> | |
| /// Log to test output. | |
| /// </summary> | |
| /// <param name="logging"> | |
| /// The global logging configuration. | |
| /// </param> | |
| /// <param name="testContext"> | |
| /// Output for the current test. | |
| /// </param> | |
| /// <param name="minLogLevel"> | |
| /// The minimum level to log at. | |
| /// </param> | |
| public static ILoggingBuilder AddTestLogging( | |
| this ILoggingBuilder logging, | |
| TestContext testContext, | |
| LogLevel minLogLevel = LogLevel.Information) | |
| { | |
| if (logging == null) | |
| { | |
| throw new ArgumentNullException(nameof(logging)); | |
| } | |
| if (testContext == null) | |
| { | |
| throw new ArgumentNullException(nameof(testContext)); | |
| } | |
| logging.AddProvider( | |
| new TestContextLoggerProvider(testContext, minLogLevel)); | |
| return logging; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@test/K8sOperator.NET.Tests/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cs`
around lines 8 - 40, The class TestContextLoggingExtensions uses the invalid
preview syntax extension(ILoggingBuilder logging); replace it with a standard
static extension method pattern: make the class static, declare a public static
void AddTestLogging(this ILoggingBuilder logging, TestContext testContext,
LogLevel minLogLevel = LogLevel.Information) and keep the existing null checks
and logging.AddProvider(new TestContextLoggerProvider(testContext,
minLogLevel)); also update the class summary XML comment to reference
TestContext instead of Xunit so documentation matches the behavior; ensure the
AddTestLogging method signature includes the this modifier to compile as an
extension method.
| GET {{WebApp_HostAddress}}/weatherforecast/ | ||
| Accept: application/json |
There was a problem hiding this comment.
Endpoint mismatch: /weatherforecast/ does not exist.
The .http file references /weatherforecast/ but Program.cs only defines a /ping endpoint. This appears to be leftover from a template.
🔧 Suggested fix
`@WebApp_HostAddress` = http://localhost:5298
-GET {{WebApp_HostAddress}}/weatherforecast/
+GET {{WebApp_HostAddress}}/ping
Accept: application/json
###📝 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.
| GET {{WebApp_HostAddress}}/weatherforecast/ | |
| Accept: application/json | |
| `@WebApp_HostAddress` = http://localhost:5298 | |
| GET {{WebApp_HostAddress}}/ping | |
| Accept: application/json | |
| ### |
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/WebApp/WebApp.http` around lines 3 - 4, The HTTP
test file references a non-existent endpoint (/weatherforecast/) while the app
only exposes /ping in Program.cs; either update
test/K8sOperator.NET.Tests/WebApp/WebApp.http to call GET
{{WebApp_HostAddress}}/ping (and set Accept: application/json) or add a matching
route in Program.cs (e.g., mapGet("/weatherforecast", ...)) to restore the
endpoint; locate Program.cs and the WebApp.http file and make the change so the
test and the exposed route names match.
pmdevers
force-pushed
the
entire-rewrite
branch
from
0db75f8 to
a9987e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
254-262: Boolean type incorrectly maps to "string" instead of "boolean".The JSON Schema type for
System.Booleanshould be"boolean", not"string". This will cause Kubernetes CRD schema validation to expect string values for boolean fields instead oftrue/false.🐛 Proposed fix
if (type.FullName == "System.Boolean") { builder.Add(x => { - x.Type = "string"; + x.Type = "boolean"; x.Nullable = nullable; }); return builder; }src/K8sOperator.NET/ChangeTracker.cs (1)
3-27: Add thread-safe synchronization forChangeTrackerDictionary access.
Dictionary<,>is not safe for concurrent writes. TheOnEvent()method fires async tasks viaContinueWith()without awaiting, allowing multiple event handlers to execute concurrently and access_lastResourceGenerationProcessedwithout synchronization. This can corrupt state or throw at runtime. UseConcurrentDictionaryor implement locking to protect concurrent access within a singleEventWatcherinstance.src/K8sOperator.NET/Generation/EntityScope.cs (1)
4-6: Add missing XML documentation summary.The
<summary>tag for theEntityScopeenum is empty. Consider adding a brief description for API documentation completeness.📝 Suggested documentation
/// <summary> -/// +/// Specifies the scope of a Kubernetes entity. /// </summary> public enum EntityScopesrc/K8sOperator.NET/Commands/InstallCommand.cs (1)
139-149: Null check needed fornameinCreateClusterRoleBinding.If
OperatorNameAttributeis missing,namewill benull, resulting in role binding names like"-role-binding". This would create invalid or unexpected Kubernetes resources.Proposed fix
private static V1ClusterRoleBinding CreateClusterRoleBinding(IReadOnlyList<object> metadata) { - var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName); + var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName) + ?? throw new InvalidOperationException("OperatorNameAttribute is required."); var clusterrolebinding = new ClusterRoleBindingBuilder()
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/Builder/ControllerBuilder.cs`:
- Around line 7-23: The public factory and Build path lack guards for null or
invalid inputs: validate parameters in Create and/or the private constructor and
make ControllerType setter immutable; specifically, ensure Create and the
ControllerBuilder constructor/null-check that serviceProvider, controllerType,
and metadata are not null and that controllerType implements IOperatorController
(or is assignable to it) before returning the builder, and change the
ControllerType property to have a private setter to prevent external mutation so
Build (which calls ActivatorUtilities.CreateInstance and casts to
IOperatorController and
Metadata.AddRange(ControllerType.GetCustomAttributes(true))) cannot throw
InvalidCastException/NullReferenceException at runtime.
In `@src/K8sOperator.NET/CommandDatasource.cs`:
- Around line 19-30: Add<TCommand> currently hardcodes CommandEntry.Order = 1
which breaks ordering used by GetCommands; change Add<TCommand> to assign Order
based on existing entries (e.g., Order = _commands.Count + 1) or otherwise
preserve append order so newly added commands get increasing Order values.
Update the constructor for the new CommandEntry in Add<TCommand> (the
CommandEntry creation block) to set Order dynamically instead of 1 so sorting by
Order in GetCommands reflects insertion order.
In `@src/K8sOperator.NET/Commands/InstallCommand.cs`:
- Around line 151-156: CreateClusterRole can receive a null name from
metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName), so add
a null/empty check after that call and fail fast with a clear exception (e.g.,
throw new InvalidOperationException or ArgumentException) referencing the
missing OperatorNameAttribute; update the code around CreateClusterRole and the
ClusterRoleBuilder().WithName($"{name}-role") to only build when name is
non-null (or use the exception) so you never pass a null into WithName.
- Around line 80-82: The assignments to name and image in InstallCommand (using
metadata.TryGetValue for OperatorNameAttribute and DockerImageAttribute) use the
null-forgiving operator and can NRE when attributes are missing; update
InstallCommand.cs to validate the results of metadata.TryGetValue(x =>
x.OperatorName) and metadata.TryGetValue(x => x.GetImage()) (and optionally the
NamespaceAttribute result stored in ns), and either throw a clear
ArgumentException/InvalidOperationException identifying the missing attribute(s)
or provide sensible defaults via null-coalescing before proceeding so callers of
InstallCommand have deterministic behavior.
In `@src/K8sOperator.NET/Generation/MetadataExtensions.cs`:
- Around line 90-95: WithFinalizer currently calls WithMetadata which appends
multiple FinalizerAttribute instances, making repeated calls order-dependent;
change WithFinalizer (the extension method on TBuilder where TBuilder :
ConventionBuilder<ControllerBuilder>) to call WithSingle(new
FinalizerAttribute(finalizer)) instead of WithMetadata so the metadata is
single-valued (replace the WithMetadata invocation on the builder with
WithSingle to ensure only one FinalizerAttribute is stored).
In `@src/K8sOperator.NET/LoggerExtensions.cs`:
- Around line 4-8: The example import and a minor formatting issue need fixing:
update the using directive in the example file that references the old namespace
to the new public namespace (change the using in
examples/SimpleOperator/Program.cs from K8sOperator.NET.Extensions to
K8sOperator.NET so it matches the public static partial class LoggerExtensions),
and remove the extra blank line after the opening brace in LoggerExtensions.cs
to tidy formatting.
In `@src/K8sOperator.NET/Operator.targets`:
- Line 6: The default for OperatorNamespace currently falls back to the project
name; change it so when OperatorNamespace is empty it uses OperatorName +
"-system" to preserve backward compatibility for consumers who override
OperatorName, and if OperatorName itself is empty then still fall back to the
existing MSBuildProjectName.Replace(" ", "_").ToLower() behavior; update the
OperatorNamespace element (the one with the Condition="'$(OperatorNamespace)' ==
''") to compute "$(OperatorName)-system" when OperatorName is set and only use
the project-name replacement when OperatorName is not set.
In `@src/K8sOperator.NET/OperatorService.cs`:
- Around line 29-46: The code only catches OperatorException from watcher.Start
and rethrows, allowing any other exception to bubble up through Task.WhenAll and
crash the host; update the Task.Run block that calls watcher.Start to catch
Exception as well, log unexpected errors (use the service logger, e.g.,
_logger), and call cts.CancelAsync() to cancel sibling watchers instead of
letting the exception propagate—keep rethrowing only for OperatorException if
you need existing behavior, otherwise swallow non-OperatorException after
logging and cancelling so Task.WhenAll doesn't terminate the background service
unexpectedly; ensure you reference watcher.Start, OperatorException,
cts.CancelAsync, _runningTasks and Task.WhenAll when making the changes.
♻️ Duplicate comments (2)
src/K8sOperator.NET/Metadata/LabelSelectorAttribute.cs (1)
5-10: Previous feedback partially addressed.The property is now immutable (
get;only), which addresses part of the prior feedback. The null guard on the constructor parameter was not added as previously suggested.src/K8sOperator.NET/OperatorService.cs (1)
17-24: UseCount == 0instead ofAny()for the materialized list.Since
watchersis now materialized as aList<>, preferwatchers.Count == 0over!watchers.Any()for O(1) property access instead of enumerator overhead.Suggested change
- if (!watchers.Any()) + if (watchers.Count == 0)
🧹 Nitpick comments (19)
src/K8sOperator.NET/Helpers/ConsoleHelpers.cs (1)
3-20: Public class exposes no usable API.
Since all members remaininternal, external consumers can’t use this type. If the goal is to expose console helpers, make the fields (or wrapper methods) public; otherwise keep the classinternalto avoid a misleading public surface.♻️ Example fix: expose the constants
- internal static readonly string SPACE = string.Empty.PadRight(3); - internal static readonly string NL = Environment.NewLine; // shortcut - internal static readonly string NORMAL = Console.IsOutputRedirected ? "" : "\x1b[39m"; - internal static readonly string RED = Console.IsOutputRedirected ? "" : "\x1b[91m"; - internal static readonly string GREEN = Console.IsOutputRedirected ? "" : "\x1b[92m"; - internal static readonly string YELLOW = Console.IsOutputRedirected ? "" : "\x1b[93m"; - internal static readonly string BLUE = Console.IsOutputRedirected ? "" : "\x1b[94m"; - internal static readonly string MAGENTA = Console.IsOutputRedirected ? "" : "\x1b[95m"; - internal static readonly string CYAN = Console.IsOutputRedirected ? "" : "\x1b[96m"; - internal static readonly string GREY = Console.IsOutputRedirected ? "" : "\x1b[97m"; - internal static readonly string BOLD = Console.IsOutputRedirected ? "" : "\x1b[1m"; - internal static readonly string NOBOLD = Console.IsOutputRedirected ? "" : "\x1b[22m"; - internal static readonly string UNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[4m"; - internal static readonly string NOUNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[24m"; - internal static readonly string REVERSE = Console.IsOutputRedirected ? "" : "\x1b[7m"; - internal static readonly string NOREVERSE = Console.IsOutputRedirected ? "" : "\x1b[27m"; + public static readonly string SPACE = string.Empty.PadRight(3); + public static readonly string NL = Environment.NewLine; // shortcut + public static readonly string NORMAL = Console.IsOutputRedirected ? "" : "\x1b[39m"; + public static readonly string RED = Console.IsOutputRedirected ? "" : "\x1b[91m"; + public static readonly string GREEN = Console.IsOutputRedirected ? "" : "\x1b[92m"; + public static readonly string YELLOW = Console.IsOutputRedirected ? "" : "\x1b[93m"; + public static readonly string BLUE = Console.IsOutputRedirected ? "" : "\x1b[94m"; + public static readonly string MAGENTA = Console.IsOutputRedirected ? "" : "\x1b[95m"; + public static readonly string CYAN = Console.IsOutputRedirected ? "" : "\x1b[96m"; + public static readonly string GREY = Console.IsOutputRedirected ? "" : "\x1b[97m"; + public static readonly string BOLD = Console.IsOutputRedirected ? "" : "\x1b[1m"; + public static readonly string NOBOLD = Console.IsOutputRedirected ? "" : "\x1b[22m"; + public static readonly string UNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[4m"; + public static readonly string NOUNDERLINE = Console.IsOutputRedirected ? "" : "\x1b[24m"; + public static readonly string REVERSE = Console.IsOutputRedirected ? "" : "\x1b[7m"; + public static readonly string NOREVERSE = Console.IsOutputRedirected ? "" : "\x1b[27m";src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (4)
264-272: DateTime mapping looks correct.Mapping
System.DateTimeto"string"is appropriate for Kubernetes CRDs. Optionally, consider addingformat: "date-time"to the schema for better OpenAPI documentation.✨ Optional enhancement for date-time format
if (type.FullName == "System.DateTime") { builder.Add(x => { x.Type = "string"; + x.Format = "date-time"; x.Nullable = nullable; }); return builder; }
404-410: String-based nullability check is fragile.Checking
type.FullName?.Contains("Nullable")could match unintended types (e.g., a customMyNullableWrapperclass). Consider using the framework'sNullable.GetUnderlyingType()for a more robust check.♻️ Proposed improvement
public static bool IsNullable(this Type type) - => type.FullName?.Contains("Nullable") == true; + => Nullable.GetUnderlyingType(type) != null;
411-418: AllocatingNullabilityInfoContextper call is inefficient.
NullabilityInfoContextis designed to be reused and caches reflection metadata internally. Creating a new instance for each property check causes unnecessary allocations. Consider caching it as a static field.♻️ Proposed improvement
+ private static readonly NullabilityInfoContext NullabilityContext = new(); + /// <summary> /// Check if a property is nullable. /// </summary> /// <param name="prop">The property.</param> - /// <returns>True if the type is nullable (i.e. contains "nullable" in its name).</returns> + /// <returns>True if the property is nullable.</returns> public static bool IsNullable(this PropertyInfo prop) - => new NullabilityInfoContext().Create(prop).ReadState == NullabilityState.Nullable || - prop.PropertyType.FullName?.Contains("Nullable") == true; + => NullabilityContext.Create(prop).ReadState == NullabilityState.Nullable || + Nullable.GetUnderlyingType(prop.PropertyType) != null;
159-166: Missing null checks for optional properties.Using the null-forgiving operator (
!) onGetPropertyresults assumes all resource types have bothStatusandSpecproperties. This will throwNullReferenceExceptionif a custom resource type doesn't define one of these properties.♻️ Defensive null handling
- var status = resourceType.GetProperty("Status")!; - var spec = resourceType.GetProperty("Spec")!; + var status = resourceType.GetProperty("Status"); + var spec = resourceType.GetProperty("Spec"); - s.WithProperty("status", sub => + if (status != null) { - sub.ObjectType(status.PropertyType); - }); - s.WithProperty("spec", sub => sub.ObjectType(spec.PropertyType)); + s.WithProperty("status", sub => sub.ObjectType(status.PropertyType)); + } + if (spec != null) + { + s.WithProperty("spec", sub => sub.ObjectType(spec.PropertyType)); + }src/K8sOperator.NET/ChangeTracker.cs (1)
3-27: Public API should validate inputs.Now that
ChangeTrackeris public, consider guarding against nullresource(and possiblyresource.Metadata/Uid) to make the API contract explicit.♻️ Example guard pattern
public bool IsResourceGenerationAlreadyHandled(CustomResource resource) { + ArgumentNullException.ThrowIfNull(resource); + ArgumentNullException.ThrowIfNull(resource.Metadata); + ArgumentNullException.ThrowIfNull(resource.Metadata.Uid); bool processedInPast = _lastResourceGenerationProcessed.TryGetValue(resource.Metadata.Uid, out long resourceGeneration);test/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cs (1)
8-40: Replace preview extension block with a standard extension method.The
extension(ILoggingBuilder logging)block is a C# 14 preview feature. If you prefer to avoid preview features or need compatibility with stable language features, convert to the standard extension-method pattern instead.Proposed fix
public static class TestContextLoggingExtensions { - extension(ILoggingBuilder logging) - { - /// <summary> - /// Log to test output. - /// </summary> - /// <param name="logging"> - /// The global logging configuration. - /// </param> - /// <param name="testContext"> - /// Output for the current test. - /// </param> - /// <param name="minLogLevel"> - /// The minimum level to log at. - /// </param> - public void AddTestLogging(TestContext testContext, - LogLevel minLogLevel = LogLevel.Information) - { - if (logging == null) - { - throw new ArgumentNullException(nameof(logging)); - } - - if (testContext == null) - { - throw new ArgumentNullException(nameof(testContext)); - } - - logging.AddProvider( - new TestContextLoggerProvider(testContext, minLogLevel)); - } - } + /// <summary> + /// Log to test output. + /// </summary> + /// <param name="logging"> + /// The global logging configuration. + /// </param> + /// <param name="testContext"> + /// Output for the current test. + /// </param> + /// <param name="minLogLevel"> + /// The minimum level to log at. + /// </param> + public static ILoggingBuilder AddTestLogging( + this ILoggingBuilder logging, + TestContext testContext, + LogLevel minLogLevel = LogLevel.Information) + { + if (logging == null) + { + throw new ArgumentNullException(nameof(logging)); + } + + if (testContext == null) + { + throw new ArgumentNullException(nameof(testContext)); + } + + logging.AddProvider( + new TestContextLoggerProvider(testContext, minLogLevel)); + return logging; + } }src/K8sOperator.NET/Generation/PolicyRuleBuilderExtensions.cs (1)
3-3: Confirm migration path for the namespace move.
This public extension class moved namespaces, so existingusing K8sOperator.NET.Generators.Builderscode will break. If compatibility matters, add an obsolete shim wrapper or include clear migration notes.test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj (1)
12-13: Consider updating TUnit to a newer version.Microsoft.AspNetCore.Mvc.Testing 10.0.2 is the latest version and compatible with .NET 10. However, TUnit 1.11.28 is outdated; newer versions (1.11.45, 1.11.51) are available and compatible with .NET 10.
src/K8sOperator.NET/K8sOperator.NET.csproj (1)
23-25: EmptyPackageTagselement.The
PackageTagselement is empty, which may be an oversight. Consider adding relevant tags (e.g.,kubernetes,operator,k8s,dotnet) to improve discoverability on NuGet.src/K8sOperator.NET/Commands/VersionCommand.cs (1)
1-6: Consider removing unusedusing K8sOperator.NET;.The root namespace
K8sOperator.NETappears unused in this file since the code is already within theK8sOperator.NET.Commandsnamespace and explicitly uses types from other namespaces.src/K8sOperator.NET/OperatorService.cs (1)
13-13: Consider making_runningTasksa local variable.The
_runningTasksfield is only used withinExecuteAsyncand isn't accessed elsewhere. A local variable would better express this scope and avoid holding references after execution completes.Suggested change
- private readonly List<Task> _runningTasks = [];Then in
ExecuteAsync:+ var runningTasks = new List<Task>(); foreach (var watcher in watchers) { // ... - _runningTasks.Add(task); + runningTasks.Add(task); } - await Task.WhenAll(_runningTasks); + await Task.WhenAll(runningTasks);src/K8sOperator.NET/Metadata/NamespaceAttribute.cs (1)
10-10: Consider makingNamespaceimmutable (remove setter).Other similar attributes like
FinalizerAttributeandLabelSelectorAttributeexpose their main property as read-only. Having a setter here introduces inconsistency and allows mutation after construction, which is atypical for attributes.Suggested change
- public string Namespace { get; set; } = ns; + public string Namespace { get; } = ns;src/K8sOperator.NET/Commands/InstallCommand.cs (1)
17-20: Incomplete XML documentation.The XML doc comment for
RunAsyncis empty. Either remove it or add meaningful documentation describing the method's behavior.src/K8sOperator.NET/Builder/EventWatcherBuilder.cs (1)
5-9: Consider encapsulation ofServiceProvider.Making
ServiceProviderpublicly accessible allows external code to resolve arbitrary services from the DI container. If this builder is intended for internal use only, consider keeping these propertiesinternalor using a more restrictive access pattern.src/K8sOperator.NET/OperatorExtensions.cs (2)
48-61: Consider error handling for Kubernetes client configuration.If neither in-cluster config nor a valid kubeconfig file exists,
KubernetesClientConfiguration.BuildConfigFromConfigFile()will throw. This fail-fast behavior may be intentional, but consider wrapping with a more informative exception message for better debugging experience.Proposed improvement
services.AddSingleton<IKubernetes>(x => { KubernetesClientConfiguration config; - if (KubernetesClientConfiguration.IsInCluster()) - { - config = KubernetesClientConfiguration.InClusterConfig(); - } - else + try { - config = KubernetesClientConfiguration.BuildConfigFromConfigFile(); + if (KubernetesClientConfiguration.IsInCluster()) + { + config = KubernetesClientConfiguration.InClusterConfig(); + } + else + { + config = KubernetesClientConfiguration.BuildConfigFromConfigFile(); + } + } + catch (Exception ex) + { + throw new InvalidOperationException( + "Failed to configure Kubernetes client. Ensure running in-cluster or with valid kubeconfig.", ex); } return new Kubernetes(config); });
107-110: EmptyOperatorBuilderclass.The
OperatorBuilderclass is empty. If this is a placeholder for future configuration options, consider adding a TODO comment. Otherwise, if no configuration is needed, theAction<OperatorBuilder>?parameter inAddOperatorcould be removed.src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
33-37: Remove misleading<inheritdoc />tags.
OperatorArgumentAttributedoesn't inherit from a type with documented properties, so<inheritdoc />onArgumentandDescriptionwon't generate any documentation. Either add actual documentation or remove the tags.Proposed fix
- /// <inheritdoc /> + /// <summary> + /// Gets or sets the command argument name. + /// </summary> public string Argument { get; set; } = argument; - /// <inheritdoc /> + /// <summary> + /// Gets or sets the command description for help text. + /// </summary> public string Description { get; set; } = string.Empty;
38-41: Incomplete XML documentation forOrderproperty.The
<summary>tag is empty. Add a description explaining the purpose of theOrderproperty (e.g., command display/execution order).Proposed fix
- /// <summary> - /// - /// </summary> + /// <summary> + /// Gets or sets the display order for commands in help output. Lower values appear first. + /// </summary> public int Order { get; set; } = 1;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (94)
Directory.Packages.propsK8sOperator.NET.slnxK8sOperator.slnsrc/Directory.Build.propssrc/K8sOperator.NET.Generators/K8sOperator.NET.Generators.csprojsrc/K8sOperator.NET.Generators/OperatorApplicationExtensions.cssrc/K8sOperator.NET.Generators/Utilities.cssrc/K8sOperator.NET/Builder/CommandBuilder.cssrc/K8sOperator.NET/Builder/ControllerBuilder.cssrc/K8sOperator.NET/Builder/ControllerConventionBuilder.cssrc/K8sOperator.NET/Builder/ConventionBuilder.cssrc/K8sOperator.NET/Builder/EventWatcherBuilder.cssrc/K8sOperator.NET/Builder/KubernetesBuilder.cssrc/K8sOperator.NET/Builder/LoggingBuilder.cssrc/K8sOperator.NET/Builder/OperatorCommandBuilder.cssrc/K8sOperator.NET/Builder/OperatorCommandConventionBuilder.cssrc/K8sOperator.NET/Builder/OperatorHostBuilder.cssrc/K8sOperator.NET/ChangeTracker.cssrc/K8sOperator.NET/CommandDatasource.cssrc/K8sOperator.NET/Commands/HelpCommand.cssrc/K8sOperator.NET/Commands/InstallCommand.cssrc/K8sOperator.NET/Commands/OperatorCommand.cssrc/K8sOperator.NET/Commands/VersionCommand.cssrc/K8sOperator.NET/ControllerDatasource.cssrc/K8sOperator.NET/CustomResource.cssrc/K8sOperator.NET/EventWatcher.cssrc/K8sOperator.NET/EventWatcherDatasource.cssrc/K8sOperator.NET/Extensions/ControllerConventionBuilderExtensions.cssrc/K8sOperator.NET/Extensions/KubernetesBuilderExtensions.cssrc/K8sOperator.NET/Extensions/OperatorHostBuilderExtensions.cssrc/K8sOperator.NET/Generation/ClusterRoleBindingBuilder.cssrc/K8sOperator.NET/Generation/ClusterRoleBindingBuilderExtensions.cssrc/K8sOperator.NET/Generation/ClusterRoleBuilder.cssrc/K8sOperator.NET/Generation/ClusterRoleBuilderExtensions.cssrc/K8sOperator.NET/Generation/ContainerBuilder.cssrc/K8sOperator.NET/Generation/CustomResourceDefinitionBuilder.cssrc/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cssrc/K8sOperator.NET/Generation/DeploymentBuilder.cssrc/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cssrc/K8sOperator.NET/Generation/EntityScope.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilder.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilderExtentions.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetaData.cssrc/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetadataExtentions.cssrc/K8sOperator.NET/Generation/MetadataExtensions.cssrc/K8sOperator.NET/Generation/PolicyRuleBuilder.cssrc/K8sOperator.NET/Generation/PolicyRuleBuilderExtensions.cssrc/K8sOperator.NET/Helpers/ConsoleHelpers.cssrc/K8sOperator.NET/Helpers/DebuggerHelpers.cssrc/K8sOperator.NET/Helpers/Utilities.cssrc/K8sOperator.NET/K8sOperator.NET.csprojsrc/K8sOperator.NET/KubernetesClient.cssrc/K8sOperator.NET/LoggerExtensions.cssrc/K8sOperator.NET/Metadata/DescriptionMetadata.cssrc/K8sOperator.NET/Metadata/DockerImageAttribute.cssrc/K8sOperator.NET/Metadata/FinalizerAttribute.cssrc/K8sOperator.NET/Metadata/FinalizerMetadata.cssrc/K8sOperator.NET/Metadata/ImageRepositoryMetadata.cssrc/K8sOperator.NET/Metadata/LabelSelectorAttribute.cssrc/K8sOperator.NET/Metadata/LabelSelectorMetadata.cssrc/K8sOperator.NET/Metadata/NamespaceAttribute.cssrc/K8sOperator.NET/Metadata/NamespaceMetadata.cssrc/K8sOperator.NET/Metadata/OperatorArgumentMetadata.cssrc/K8sOperator.NET/Metadata/OperatorNameAttribute.cssrc/K8sOperator.NET/Metadata/OperatorNameMetadata.cssrc/K8sOperator.NET/Operator.cssrc/K8sOperator.NET/Operator.targetssrc/K8sOperator.NET/OperatorController.cssrc/K8sOperator.NET/OperatorException.cssrc/K8sOperator.NET/OperatorExtensions.cssrc/K8sOperator.NET/OperatorHost.cssrc/K8sOperator.NET/OperatorService.cstest/Directory.Build.propstest/K8sOperator.NET.Generators.Tests/K8sOperator.NET.Generators.Tests.csprojtest/K8sOperator.NET.Generators.Tests/UnitTest1.cstest/K8sOperator.NET.Tests/ChangeTracking_Tests.cstest/K8sOperator.NET.Tests/ControllerDatasourceTests.cstest/K8sOperator.NET.Tests/ControllerTests.cstest/K8sOperator.NET.Tests/EntityScope.cstest/K8sOperator.NET.Tests/EventWatcherTests.cstest/K8sOperator.NET.Tests/Fixtures/TestResource.cstest/K8sOperator.NET.Tests/GlobalSetup.cstest/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojtest/K8sOperator.NET.Tests/Logging/TestContextLogger.cstest/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cstest/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cstest/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/ListNamespacedCustomObjectWithHttpMessagesAsync.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/ReplaceNamespacedCustomObjectAsync.cstest/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cstest/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cstest/K8sOperator.NET.Tests/MyController.cstest/K8sOperator.NET.Tests/OperatorExtensions_Tests.cstest/K8sOperator.NET.Tests/OperatorTests.cs
💤 Files with no reviewable changes (40)
- src/K8sOperator.NET/Metadata/LabelSelectorMetadata.cs
- src/K8sOperator.NET/Metadata/OperatorNameMetadata.cs
- test/K8sOperator.NET.Tests/EntityScope.cs
- test/K8sOperator.NET.Tests/Mocks/Endpoints/ReplaceNamespacedCustomObjectAsync.cs
- src/K8sOperator.NET.Generators/Utilities.cs
- test/K8sOperator.NET.Tests/ControllerTests.cs
- src/K8sOperator.NET/Builder/OperatorHostBuilder.cs
- src/K8sOperator.NET/Metadata/DescriptionMetadata.cs
- src/K8sOperator.NET/Builder/ControllerConventionBuilder.cs
- test/K8sOperator.NET.Generators.Tests/UnitTest1.cs
- test/K8sOperator.NET.Tests/Mocks/Endpoints/ListNamespacedCustomObjectWithHttpMessagesAsync.cs
- src/K8sOperator.NET/Metadata/OperatorArgumentMetadata.cs
- test/K8sOperator.NET.Tests/ControllerDatasourceTests.cs
- src/K8sOperator.NET/ControllerDatasource.cs
- src/Directory.Build.props
- src/K8sOperator.NET/OperatorHost.cs
- src/K8sOperator.NET/Metadata/FinalizerMetadata.cs
- src/K8sOperator.NET/Metadata/ImageRepositoryMetadata.cs
- test/K8sOperator.NET.Tests/Logging/TestOutputLoggingExtensions.cs
- test/K8sOperator.NET.Generators.Tests/K8sOperator.NET.Generators.Tests.csproj
- test/K8sOperator.NET.Tests/Mocks/Endpoints/WatchNamespaced.cs
- src/K8sOperator.NET/Metadata/NamespaceMetadata.cs
- test/K8sOperator.NET.Tests/EventWatcherTests.cs
- src/K8sOperator.NET.Generators/K8sOperator.NET.Generators.csproj
- src/K8sOperator.NET/Builder/LoggingBuilder.cs
- src/K8sOperator.NET.Generators/OperatorApplicationExtensions.cs
- src/K8sOperator.NET/KubernetesClient.cs
- src/K8sOperator.NET/Extensions/KubernetesBuilderExtensions.cs
- src/K8sOperator.NET/Builder/OperatorCommandBuilder.cs
- Directory.Packages.props
- src/K8sOperator.NET/Extensions/OperatorHostBuilderExtensions.cs
- src/K8sOperator.NET/Helpers/Utilities.cs
- src/K8sOperator.NET/Builder/KubernetesBuilder.cs
- test/K8sOperator.NET.Tests/MyController.cs
- src/K8sOperator.NET/Operator.cs
- test/K8sOperator.NET.Tests/OperatorTests.cs
- src/K8sOperator.NET/Extensions/ControllerConventionBuilderExtensions.cs
- src/K8sOperator.NET/Builder/OperatorCommandConventionBuilder.cs
- K8sOperator.sln
- test/Directory.Build.props
✅ Files skipped from review due to trivial changes (1)
- K8sOperator.NET.slnx
🚧 Files skipped from review as they are similar to previous changes (11)
- src/K8sOperator.NET/Generation/ClusterRoleBuilder.cs
- src/K8sOperator.NET/Generation/ContainerBuilder.cs
- src/K8sOperator.NET/CustomResource.cs
- src/K8sOperator.NET/Generation/PolicyRuleBuilder.cs
- src/K8sOperator.NET/Generation/ClusterRoleBuilderExtensions.cs
- src/K8sOperator.NET/Generation/ClusterRoleBindingBuilderExtensions.cs
- src/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetadataExtentions.cs
- src/K8sOperator.NET/Generation/DeploymentBuilder.cs
- src/K8sOperator.NET/EventWatcherDatasource.cs
- src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs
- src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilder.cs
🧰 Additional context used
🧬 Code graph analysis (22)
test/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs (1)
test/K8sOperator.NET.Tests/Logging/TestContextLogger.cs (2)
TestContextLogger(8-103)TestContextLogger(24-36)
src/K8sOperator.NET/Commands/InstallCommand.cs (7)
src/K8sOperator.NET/Commands/HelpCommand.cs (1)
OperatorArgument(8-33)src/K8sOperator.NET/Commands/OperatorCommand.cs (1)
OperatorArgument(6-13)src/K8sOperator.NET/Commands/VersionCommand.cs (1)
OperatorArgument(10-29)src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
IOperatorCommand(14-22)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilder.cs (1)
CustomResourceDefinitionBuilder(6-14)src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (1)
GetImage(13-13)
src/K8sOperator.NET/Builder/ConventionBuilder.cs (2)
src/K8sOperator.NET/CommandDatasource.cs (1)
ConventionBuilder(19-30)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
ConventionBuilder(21-32)
src/K8sOperator.NET/Commands/VersionCommand.cs (3)
src/K8sOperator.NET/Commands/InstallCommand.cs (3)
OperatorArgument(12-188)Task(21-41)Task(43-47)src/K8sOperator.NET/Commands/OperatorCommand.cs (2)
OperatorArgument(6-13)Task(9-12)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)
src/K8sOperator.NET/OperatorController.cs (1)
src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
IOperatorController(19-28)
src/K8sOperator.NET/Commands/OperatorCommand.cs (3)
src/K8sOperator.NET/Commands/HelpCommand.cs (2)
OperatorArgument(8-33)Task(11-32)src/K8sOperator.NET/Commands/VersionCommand.cs (2)
OperatorArgument(10-29)Task(13-28)src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
IOperatorCommand(14-22)Task(27-27)
src/K8sOperator.NET/Commands/HelpCommand.cs (5)
src/K8sOperator.NET/Helpers/ConsoleHelpers.cs (1)
ConsoleHelpers(3-21)src/K8sOperator.NET/Commands/OperatorCommand.cs (2)
OperatorArgument(6-13)Task(9-12)src/K8sOperator.NET/Commands/VersionCommand.cs (2)
OperatorArgument(10-29)Task(13-28)src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
IOperatorCommand(14-22)Task(27-27)src/K8sOperator.NET/CommandDatasource.cs (1)
CommandDatasource(6-48)
src/K8sOperator.NET/Generation/KubernetesObjectBuilderExtentions.cs (2)
src/K8sOperator.NET/Generation/KubernetesObjectBuilder.cs (4)
T(19-19)T(32-42)Add(13-13)Add(27-30)src/K8sOperator.NET/Builder/ConventionBuilder.cs (1)
Add(5-8)
src/K8sOperator.NET/OperatorService.cs (3)
src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)src/K8sOperator.NET/EventWatcher.cs (11)
Task(23-85)Task(102-129)Task(131-140)Task(142-153)Task(155-176)Task(178-195)Task(197-201)Task(203-209)Task(212-220)Task(225-249)Task(251-275)src/K8sOperator.NET/OperatorException.cs (1)
OperatorException(3-6)
src/K8sOperator.NET/EventWatcher.cs (3)
src/K8sOperator.NET/OperatorController.cs (6)
OperatorController(7-69)Task(21-24)Task(32-35)Task(43-46)Task(54-57)Task(65-68)src/K8sOperator.NET/CustomResource.cs (3)
CustomResource(10-15)CustomResource(22-30)CustomResource(38-47)src/K8sOperator.NET/ChangeTracker.cs (2)
TrackResourceGenerationAsHandled(16-22)ChangeTracker(3-28)
test/K8sOperator.NET.Tests/OperatorExtensions_Tests.cs (2)
src/K8sOperator.NET/CommandDatasource.cs (1)
CommandDatasource(6-48)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
EventWatcherDatasource(14-59)
src/K8sOperator.NET/OperatorExtensions.cs (5)
src/K8sOperator.NET/CommandDatasource.cs (2)
CommandDatasource(6-48)ConventionBuilder(19-30)src/K8sOperator.NET/Builder/ConventionBuilder.cs (2)
Add(5-8)ConventionBuilder(3-9)src/K8sOperator.NET/EventWatcherDatasource.cs (2)
EventWatcherDatasource(14-59)ConventionBuilder(21-32)src/K8sOperator.NET/Builder/ControllerBuilder.cs (4)
ControllerBuilder(5-30)ControllerBuilder(7-12)ControllerBuilder(16-17)IOperatorController(19-28)src/K8sOperator.NET/Commands/OperatorCommand.cs (1)
Task(9-12)
test/K8sOperator.NET.Tests/ChangeTracking_Tests.cs (2)
src/K8sOperator.NET/ChangeTracker.cs (1)
ChangeTracker(3-28)src/K8sOperator.NET/EventWatcher.cs (10)
Task(23-85)Task(102-129)Task(131-140)Task(142-153)Task(155-176)Task(178-195)Task(197-201)Task(203-209)Task(212-220)Task(225-249)
src/K8sOperator.NET/Generation/MetadataExtensions.cs (4)
src/K8sOperator.NET/Generation/ClusterRoleBindingBuilderExtensions.cs (2)
TBuilder(20-29)TBuilder(41-51)src/K8sOperator.NET/Builder/ConventionBuilder.cs (2)
ConventionBuilder(3-9)Add(5-8)src/K8sOperator.NET/CommandDatasource.cs (1)
ConventionBuilder(19-30)src/K8sOperator.NET/EventWatcherDatasource.cs (1)
ConventionBuilder(21-32)
src/K8sOperator.NET/Metadata/OperatorNameAttribute.cs (6)
src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
AttributeUsage(30-45)ToString(43-44)src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (2)
AttributeUsage(5-17)ToString(15-16)src/K8sOperator.NET/Metadata/FinalizerAttribute.cs (2)
AttributeUsage(5-12)ToString(10-11)src/K8sOperator.NET/Metadata/LabelSelectorAttribute.cs (2)
AttributeUsage(5-11)ToString(9-10)src/K8sOperator.NET/Metadata/NamespaceAttribute.cs (2)
AttributeUsage(5-14)ToString(12-13)src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
test/K8sOperator.NET.Tests/Fixtures/TestResource.cs (1)
src/K8sOperator.NET/CustomResource.cs (3)
CustomResource(10-15)CustomResource(22-30)CustomResource(38-47)
src/K8sOperator.NET/Builder/EventWatcherBuilder.cs (2)
src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
IOperatorController(19-28)src/K8sOperator.NET/EventWatcher.cs (1)
EventWatcher(11-324)
src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
test/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cs (1)
test/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs (2)
TestContextLoggerProvider(8-59)TestContextLoggerProvider(21-30)
src/K8sOperator.NET/Metadata/FinalizerAttribute.cs (6)
src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
AttributeUsage(30-45)ToString(43-44)src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (2)
AttributeUsage(5-17)ToString(15-16)src/K8sOperator.NET/Metadata/LabelSelectorAttribute.cs (2)
AttributeUsage(5-11)ToString(9-10)src/K8sOperator.NET/Metadata/NamespaceAttribute.cs (2)
AttributeUsage(5-14)ToString(12-13)src/K8sOperator.NET/Metadata/OperatorNameAttribute.cs (2)
AttributeUsage(5-14)ToString(12-13)src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (2)
DebuggerHelpers(3-8)GetDebugText(5-6)
src/K8sOperator.NET/CommandDatasource.cs (2)
src/K8sOperator.NET/Builder/CommandBuilder.cs (2)
CommandBuilder(8-23)IOperatorCommand(14-22)src/K8sOperator.NET/Builder/ConventionBuilder.cs (2)
ConventionBuilder(3-9)Add(5-8)
src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (2)
src/K8sOperator.NET/Builder/ConventionBuilder.cs (1)
Add(5-8)src/K8sOperator.NET/Generation/KubernetesObjectBuilder.cs (2)
Add(13-13)Add(27-30)
⏰ 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 (47)
test/K8sOperator.NET.Tests/GlobalSetup.cs (1)
1-4: LGTM — assembly-level exclusion is appropriate for tests.test/K8sOperator.NET.Tests/Fixtures/TestResource.cs (1)
5-16: LGTM — test fixture aligns with CustomResource<TSpec, TStatus> pattern.src/K8sOperator.NET/LoggerExtensions.cs (1)
9-195: LGTM!The
LoggerMessageattribute definitions are well-structured with uniqueEventIdvalues (0-29), appropriate log levels, and consistent message formatting. The use of source-generated logging is a good practice for high-performance logging.src/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetaData.cs (1)
4-4: This file is newly created—no namespace migration concern.The git history shows this is a new file in the latest commit, not a namespace move from an existing API surface. There is no breaking change and no need for backward compatibility shims.
Likely an incorrect or invalid review comment.
src/K8sOperator.NET/Operator.targets (1)
18-18: Good wiring ofNamespaceAttributetoOperatorNamespace.This cleanly decouples namespace from operator name when explicitly set.
src/K8sOperator.NET/ChangeTracker.cs (1)
5-5: No action needed. The project targets .NET 10.0, which supports C# 14 by default and fully includes C# 12 features like collection expressions. The[]syntax on line 5 is completely compatible with the project's configuration and will compile without issues.Likely an incorrect or invalid review comment.
test/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs (1)
5-58: LGTM — provider wiring is clean and consistent.Null checks and logger creation align with the new TestContext-based logger.
test/K8sOperator.NET.Tests/Logging/TestContextLogger.cs (1)
5-101: LGTM — logger behavior and null-safety look good.test/K8sOperator.NET.Tests/ChangeTracking_Tests.cs (2)
6-8: Initialization cleanup looks good.Inline
ChangeTrackerinitialization keeps the fixture simple and clear.
10-11: Remove this comment—the tests are using TUnit, not NUnit.The test project uses TUnit 1.11.28, not NUnit. In TUnit, assertions are awaitable and must be awaited:
await Assert.That(result).IsTrue()andawait Assert.That(result).IsFalse()are the correct syntax, and tests should beasync Task. The current implementation is correct and requires no changes.Likely an incorrect or invalid review comment.
test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (3)
1-1: Non-functional encoding change is fine.
No behavioral impact observed.
59-61: LGTM — formatting-only change.
No functional impact.
19-50: No action needed. TheAddTestLoggingimplementation is thread-safe for background-thread logging. The code correctly captures the passedTestContextinstance at construction time (not relying on the thread-affineTestContext.CurrentContext), and includes defensive error handling that catches "There is no currently active test" exceptions when logging occurs on thread-pool threads.src/K8sOperator.NET/Helpers/DebuggerHelpers.cs (1)
1-2: Whitespace-only change is fine.src/K8sOperator.NET/Generation/KubernetesObjectBuilderExtentions.cs (2)
1-3: Namespace refactor looks consistent.
34-39: Formatting-only adjustment is fine.src/K8sOperator.NET/OperatorController.cs (1)
54-56: Default BookmarkAsync implementation looks fine.src/K8sOperator.NET/OperatorException.cs (1)
1-4: Primary constructor usage is appropriate for this project.The project targets .NET 10.0, which supports C# 12+ primary constructors by default (C# 14 is the default for .NET 10.0). The primary constructor syntax used here is a modern and standard approach; no compatibility concerns exist.
Likely an incorrect or invalid review comment.
src/K8sOperator.NET/Generation/KubernetesObjectBuilder.cs (1)
1-42: LGTM!The namespace migration to
K8sOperator.NET.Generationis clean. The interface and implementation are well-structured with proper XML documentation.src/K8sOperator.NET/Generation/ClusterRoleBindingBuilder.cs (1)
1-13: LGTM!The namespace migration is consistent with the broader refactor. The
Build()pattern correctly callsbase.Build()followed byInitialize()to set the Kubernetes object's API version and kind.test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj (1)
7-8: .NET 10 is now generally available and LTS—no action needed for framework status..NET 10 is generally available (GA) as of November 11, 2025, and is designated as an LTS release with support through November 2028. The
EnablePreviewFeaturesflag can remain if specific C# preview language features are in use, but the framework itself is stable and suitable for production.src/K8sOperator.NET/Builder/ConventionBuilder.cs (1)
3-9: LGTM! Clean simplification of the convention builder.The simplified
ConventionBuilder<T>provides a fluent API for adding conventions without the complexity of the removedFinallypattern. The primary constructor cleanly captures the backing collection, and usage inCommandDatasourceandEventWatcherDatasourceconfirms correct integration.src/K8sOperator.NET/K8sOperator.NET.csproj (2)
4-4: Verify .NET 10 preview framework target is intentional.
net10.0withEnablePreviewFeaturestargets the preview release. Ensure this is intended for the package, as consumers will also need preview SDK support.
46-79:Operator.targetsis included in the NuGet package with two different configurations.The file is referenced as:
Content(lines 47-51) withPackagePath=\build\$(ProjectName).targets— packages asbuild/K8sOperator.NET.targetsNone(lines 68-71) withPackagePath=build— packages asbuild/Operator.targets- Via
IncludeOperatorTargetstarget (lines 75-78) — build-time only (noPack=True, doesn't affect packaging)Both pack items will be included in the NuGet package with different filenames. If this is intentional for auto-import support, clarify in the comment. Otherwise, consolidate to a single packaging entry.
test/K8sOperator.NET.Tests/OperatorExtensions_Tests.cs (4)
87-99: LGTM! Test now correctly builds host with the same DI configuration.The test properly configures the host with
AddOperator()viaConfigureServices, ensuringCommandDatasourceresolves commands with the correct host context.Minor nit: Line 90 comment says
// Assertbut the setup code follows—consider updating to// Arrangefor consistency with other tests.
10-19: LGTM! Proper null-guard validation.
21-73: LGTM! Service registration tests are well-structured.The tests verify that
AddOperator()registers the expected services:IHostedService,CommandDatasource,EventWatcherDatasource, andIKubernetes.
75-85: LGTM! Configure callback verification.src/K8sOperator.NET/Commands/OperatorCommand.cs (1)
7-12: LGTM! Clean migration toIHost-based command execution.The command now delegates directly to
app.RunAsync(), which is the idiomatic way to run a hosted application. This aligns with the broader DI/hosting refactor.Note: The
argsparameter inRunAsync(string[] args)is unused here, which appears intentional since the operator command simply starts the host.src/K8sOperator.NET/Commands/VersionCommand.cs (1)
11-28: LGTM! Metadata retrieval with proper null handling.The implementation correctly:
- Resolves
EventWatcherDatasourcevia DI- Extracts operator name and version from metadata attributes
- Handles missing metadata gracefully with an informative message (addressing prior feedback)
src/K8sOperator.NET/Metadata/FinalizerAttribute.cs (1)
5-12: LGTM!The attribute follows the established pattern for metadata attributes in this codebase. The use of
const string Defaultvs. a static instance property (like inNamespaceAttribute) is a minor inconsistency, but acceptable since a simple string constant suffices here.src/K8sOperator.NET/Metadata/DockerImageAttribute.cs (1)
5-17: LGTM - typo has been corrected.The previous review comment about the "Registery" typo has been addressed. The attribute now correctly uses
Registrythroughout.Note: The properties are mutable (have setters), which differs from attributes like
FinalizerAttributeandLabelSelectorAttribute. If consistency is desired, consider making them read-only as well.src/K8sOperator.NET/Metadata/OperatorNameAttribute.cs (1)
5-14: LGTM!The attribute is well-structured and follows the established patterns. Using
=> nameforOperatorNamekeeps it effectively immutable, which is appropriate for an attribute.src/K8sOperator.NET/Commands/InstallCommand.cs (1)
146-146: Verify the hardcoded namespace"system"for the ServiceAccount subject.The ClusterRoleBinding uses
ns: "system"for the ServiceAccount, but Kubernetes typically uses"default"or a custom namespace. Verify this is intentional and not a typo for"kube-system"or the operator's configured namespace.src/K8sOperator.NET/Commands/HelpCommand.cs (1)
8-32: LGTM!The implementation correctly addresses previous review feedback:
- The
Frontman.Operatorimport has been removed- The
Max()call is now guarded withinfo.Count > 0check on line 17The DI-based command retrieval and help text rendering are clean and well-structured.
src/K8sOperator.NET/OperatorExtensions.cs (2)
89-95: Previous review concern addressed.The
Filterfunction now correctly usesFirstOrDefault()with a null check, preventingInvalidOperationExceptionwhen metadata lacksOperatorArgumentAttribute.
15-67: No action required. The project targets .NET 10.0, which fully supports C# 13 extension member syntax. All consumers will automatically be compatible with this feature.src/K8sOperator.NET/Builder/CommandBuilder.cs (1)
8-23: LGTM!The
CommandBuilderimplementation is clean:
- Uses primary constructor syntax effectively
- Provides a sensible fallback when
OperatorArgumentAttributeis missing (deriving from class name)- Correctly uses
ActivatorUtilities.CreateInstancefor DI-aware instantiationsrc/K8sOperator.NET/Builder/EventWatcherBuilder.cs (1)
23-28: All dependencies required byEventWatcher<T>are properly registered and available:
IKubernetes: Registered atOperatorExtensions.cs:48ILoggerFactory: Automatically registered byWebApplication.CreateBuilder()(ASP.NET Core default)- Type compatibility:
Controlleris stored asIOperatorControllerbut instantiated as a concreteOperatorController<T>subclass at runtime, which matches the constructor requirementNo issues with the DI resolution in this code.
Likely an incorrect or invalid review comment.
src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
21-25: Inherited attribute capture is a solid improvement.Using
GetCustomAttributes(true)ensures base-class metadata is included for both controller and resource types.src/K8sOperator.NET/Generation/MetadataExtensions.cs (2)
37-81: Consistent use ofWithSinglefor group/namespace/label is good.This keeps those metadata entries unique and aligns with the new
ConventionBuilder<ControllerBuilder>constraint.
104-149: New metadata helpers are a clean addition.
RemoveMetadata/WithMetadata/WithSingleprovide a clear, composable API for metadata mutations.src/K8sOperator.NET/EventWatcher.cs (4)
11-22: Constructor DI wiring and public surface look good.
23-84: Watch loop retry + exception handling is solid.
131-249: Controller callback routing is consistent across event types.
251-324: No issues found. The initialization order is correct and metadata is fully populated beforeEventWatcherconstruction. TheControllerBuilder.Build()method (lines 22-25) appends both controller and resource type attributes to the metadata list beforeEventWatcherBuildercreates the watcher instance (lines 47-48 of EventWatcherDatasource). The_crdand_labelSelectorfields are eagerly initialized upon object construction and safely access the complete metadata at that point.src/K8sOperator.NET/CommandDatasource.cs (1)
32-53: Command build pipeline and metadata passthrough look clean.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private ControllerBuilder(IServiceProvider serviceProvider, Type controllerType, List<object> metadata) | ||
| { | ||
| ServiceProvider = serviceProvider; | ||
| ControllerType = controllerType; | ||
| Metadata = metadata; | ||
| } | ||
| public IServiceProvider ServiceProvider { get; } | ||
| public Type ControllerType { get; set; } | ||
|
|
||
| public List<object> Metadata { get; } = []; | ||
| public static ControllerBuilder Create(IServiceProvider serviceProvider, Type controllerType, List<object> metadata) | ||
| => new(serviceProvider, controllerType, metadata); | ||
|
|
||
| public IController Build() | ||
| public IOperatorController Build() | ||
| { | ||
| var controller = (IController)ActivatorUtilities.CreateInstance(_serviceProvider, _controllerType); | ||
| Metadata.AddRange(_controllerType.GetCustomAttributes()); | ||
| var controller = (IOperatorController)ActivatorUtilities.CreateInstance(ServiceProvider, ControllerType); | ||
| Metadata.AddRange(ControllerType.GetCustomAttributes(true)); | ||
|
|
There was a problem hiding this comment.
Guard ControllerType/metadata invariants in the public factory.
Create/Build can throw InvalidCastException/NullReferenceException if controllerType is null, incompatible, or metadata is null—especially because ControllerType is publicly settable. Add explicit validation (or make the setter private) for clearer, fail-fast behavior.
🛠️ Proposed guardrails
- public static ControllerBuilder Create(IServiceProvider serviceProvider, Type controllerType, List<object> metadata)
- => new(serviceProvider, controllerType, metadata);
+ public static ControllerBuilder Create(IServiceProvider serviceProvider, Type controllerType, List<object> metadata)
+ {
+ ArgumentNullException.ThrowIfNull(serviceProvider);
+ ArgumentNullException.ThrowIfNull(controllerType);
+ ArgumentNullException.ThrowIfNull(metadata);
+ if (!typeof(IOperatorController).IsAssignableFrom(controllerType))
+ {
+ throw new ArgumentException(
+ $"Type must implement {nameof(IOperatorController)}.",
+ nameof(controllerType));
+ }
+ return new(serviceProvider, controllerType, metadata);
+ }
public IOperatorController Build()
{
+ if (!typeof(IOperatorController).IsAssignableFrom(ControllerType))
+ {
+ throw new InvalidOperationException(
+ $"ControllerType must implement {nameof(IOperatorController)}.");
+ }
var controller = (IOperatorController)ActivatorUtilities.CreateInstance(ServiceProvider, ControllerType);📝 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.
| private ControllerBuilder(IServiceProvider serviceProvider, Type controllerType, List<object> metadata) | |
| { | |
| ServiceProvider = serviceProvider; | |
| ControllerType = controllerType; | |
| Metadata = metadata; | |
| } | |
| public IServiceProvider ServiceProvider { get; } | |
| public Type ControllerType { get; set; } | |
| public List<object> Metadata { get; } = []; | |
| public static ControllerBuilder Create(IServiceProvider serviceProvider, Type controllerType, List<object> metadata) | |
| => new(serviceProvider, controllerType, metadata); | |
| public IController Build() | |
| public IOperatorController Build() | |
| { | |
| var controller = (IController)ActivatorUtilities.CreateInstance(_serviceProvider, _controllerType); | |
| Metadata.AddRange(_controllerType.GetCustomAttributes()); | |
| var controller = (IOperatorController)ActivatorUtilities.CreateInstance(ServiceProvider, ControllerType); | |
| Metadata.AddRange(ControllerType.GetCustomAttributes(true)); | |
| private ControllerBuilder(IServiceProvider serviceProvider, Type controllerType, List<object> metadata) | |
| { | |
| ServiceProvider = serviceProvider; | |
| ControllerType = controllerType; | |
| Metadata = metadata; | |
| } | |
| public IServiceProvider ServiceProvider { get; } | |
| public Type ControllerType { get; set; } | |
| public static ControllerBuilder Create(IServiceProvider serviceProvider, Type controllerType, List<object> metadata) | |
| { | |
| ArgumentNullException.ThrowIfNull(serviceProvider); | |
| ArgumentNullException.ThrowIfNull(controllerType); | |
| ArgumentNullException.ThrowIfNull(metadata); | |
| if (!typeof(IOperatorController).IsAssignableFrom(controllerType)) | |
| { | |
| throw new ArgumentException( | |
| $"Type must implement {nameof(IOperatorController)}.", | |
| nameof(controllerType)); | |
| } | |
| return new(serviceProvider, controllerType, metadata); | |
| } | |
| public IOperatorController Build() | |
| { | |
| if (!typeof(IOperatorController).IsAssignableFrom(ControllerType)) | |
| { | |
| throw new InvalidOperationException( | |
| $"ControllerType must implement {nameof(IOperatorController)}."); | |
| } | |
| var controller = (IOperatorController)ActivatorUtilities.CreateInstance(ServiceProvider, ControllerType); | |
| Metadata.AddRange(ControllerType.GetCustomAttributes(true)); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Builder/ControllerBuilder.cs` around lines 7 - 23, The
public factory and Build path lack guards for null or invalid inputs: validate
parameters in Create and/or the private constructor and make ControllerType
setter immutable; specifically, ensure Create and the ControllerBuilder
constructor/null-check that serviceProvider, controllerType, and metadata are
not null and that controllerType implements IOperatorController (or is
assignable to it) before returning the builder, and change the ControllerType
property to have a private setter to prevent external mutation so Build (which
calls ActivatorUtilities.CreateInstance and casts to IOperatorController and
Metadata.AddRange(ControllerType.GetCustomAttributes(true))) cannot throw
InvalidCastException/NullReferenceException at runtime.
| public ConventionBuilder<CommandBuilder> Add<TCommand>() | ||
| where TCommand : IOperatorCommand | ||
| { | ||
| var conventions = new List<Action<IOperatorCommandBuilder>>(); | ||
| var finallyConventions = new List<Action<IOperatorCommandBuilder>>(); | ||
|
|
||
| _commands.Add(new() | ||
| var conventions = new List<Action<CommandBuilder>>(); | ||
| _commands.Add(new CommandEntry | ||
| { | ||
| CommandType = operatorCommandType, | ||
| CommandType = typeof(TCommand), | ||
| Conventions = conventions, | ||
| FinallyConventions = finallyConventions, | ||
| Order = order ?? (_commands.Count + 1) | ||
| Order = 1 | ||
| }); | ||
|
|
||
| return new OperatorCommandConventionBuilder(conventions, finallyConventions); | ||
| return new ConventionBuilder<CommandBuilder>(conventions); | ||
| } |
There was a problem hiding this comment.
Command ordering is effectively fixed to 1.
Add<TCommand> always sets Order = 1 while GetCommands sorts by Order, so consumers can no longer influence command ordering. If ordering matters (as it did before), this is a regression.
🛠️ Suggested fix
- public ConventionBuilder<CommandBuilder> Add<TCommand>()
+ public ConventionBuilder<CommandBuilder> Add<TCommand>(int order = 1)
where TCommand : IOperatorCommand
{
var conventions = new List<Action<CommandBuilder>>();
_commands.Add(new CommandEntry
{
CommandType = typeof(TCommand),
Conventions = conventions,
- Order = 1
+ Order = order
});
return new ConventionBuilder<CommandBuilder>(conventions);
}📝 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.
| public ConventionBuilder<CommandBuilder> Add<TCommand>() | |
| where TCommand : IOperatorCommand | |
| { | |
| var conventions = new List<Action<IOperatorCommandBuilder>>(); | |
| var finallyConventions = new List<Action<IOperatorCommandBuilder>>(); | |
| _commands.Add(new() | |
| var conventions = new List<Action<CommandBuilder>>(); | |
| _commands.Add(new CommandEntry | |
| { | |
| CommandType = operatorCommandType, | |
| CommandType = typeof(TCommand), | |
| Conventions = conventions, | |
| FinallyConventions = finallyConventions, | |
| Order = order ?? (_commands.Count + 1) | |
| Order = 1 | |
| }); | |
| return new OperatorCommandConventionBuilder(conventions, finallyConventions); | |
| return new ConventionBuilder<CommandBuilder>(conventions); | |
| } | |
| public ConventionBuilder<CommandBuilder> Add<TCommand>(int order = 1) | |
| where TCommand : IOperatorCommand | |
| { | |
| var conventions = new List<Action<CommandBuilder>>(); | |
| _commands.Add(new CommandEntry | |
| { | |
| CommandType = typeof(TCommand), | |
| Conventions = conventions, | |
| Order = order | |
| }); | |
| return new ConventionBuilder<CommandBuilder>(conventions); | |
| } |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/CommandDatasource.cs` around lines 19 - 30, Add<TCommand>
currently hardcodes CommandEntry.Order = 1 which breaks ordering used by
GetCommands; change Add<TCommand> to assign Order based on existing entries
(e.g., Order = _commands.Count + 1) or otherwise preserve append order so newly
added commands get increasing Order values. Update the constructor for the new
CommandEntry in Add<TCommand> (the CommandEntry creation block) to set Order
dynamically instead of 1 so sorting by Order in GetCommands reflects insertion
order.
| var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName)!; | ||
| var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())!; | ||
| var ns = metadata.TryGetValue<NamespaceAttribute, string>(x => x.Namespace); |
There was a problem hiding this comment.
Potential NullReferenceException if metadata attributes are missing.
TryGetValue returns null if the attribute is not found. Using the null-forgiving operator (!) on lines 80-81 will cause a NullReferenceException if OperatorNameAttribute or DockerImageAttribute is missing from metadata.
Consider adding validation or using null-coalescing with sensible defaults:
Proposed defensive fix
- var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName)!;
- var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())!;
- var ns = metadata.TryGetValue<NamespaceAttribute, string>(x => x.Namespace);
+ var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName)
+ ?? throw new InvalidOperationException("OperatorNameAttribute is required for deployment generation.");
+ var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())
+ ?? throw new InvalidOperationException("DockerImageAttribute is required for deployment generation.");
+ var ns = metadata.TryGetValue<NamespaceAttribute, string>(x => x.Namespace);📝 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.
| var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName)!; | |
| var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())!; | |
| var ns = metadata.TryGetValue<NamespaceAttribute, string>(x => x.Namespace); | |
| var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName) | |
| ?? throw new InvalidOperationException("OperatorNameAttribute is required for deployment generation."); | |
| var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage()) | |
| ?? throw new InvalidOperationException("DockerImageAttribute is required for deployment generation."); | |
| var ns = metadata.TryGetValue<NamespaceAttribute, string>(x => x.Namespace); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/InstallCommand.cs` around lines 80 - 82, The
assignments to name and image in InstallCommand (using metadata.TryGetValue for
OperatorNameAttribute and DockerImageAttribute) use the null-forgiving operator
and can NRE when attributes are missing; update InstallCommand.cs to validate
the results of metadata.TryGetValue(x => x.OperatorName) and
metadata.TryGetValue(x => x.GetImage()) (and optionally the NamespaceAttribute
result stored in ns), and either throw a clear
ArgumentException/InvalidOperationException identifying the missing attribute(s)
or provide sensible defaults via null-coalescing before proceeding so callers of
InstallCommand have deterministic behavior.
| private static V1ClusterRole CreateClusterRole(IReadOnlyList<object> metadata, IEnumerable<IEventWatcher> watchers) | ||
| { | ||
| var name = metadata.TryGetValue<IOperatorNameMetadata, string>(x => x.OperatorName); | ||
| var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName); | ||
|
|
||
| var clusterrole = new ClusterRoleBuilder() | ||
| .WithName($"{name}-role"); |
There was a problem hiding this comment.
Null check needed for name in CreateClusterRole.
Same issue as CreateClusterRoleBinding - name can be null if OperatorNameAttribute is missing.
Proposed fix
private static V1ClusterRole CreateClusterRole(IReadOnlyList<object> metadata, IEnumerable<IEventWatcher> watchers)
{
- var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName);
+ var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName)
+ ?? throw new InvalidOperationException("OperatorNameAttribute is required.");
var clusterrole = new ClusterRoleBuilder()
.WithName($"{name}-role");📝 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.
| private static V1ClusterRole CreateClusterRole(IReadOnlyList<object> metadata, IEnumerable<IEventWatcher> watchers) | |
| { | |
| var name = metadata.TryGetValue<IOperatorNameMetadata, string>(x => x.OperatorName); | |
| var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName); | |
| var clusterrole = new ClusterRoleBuilder() | |
| .WithName($"{name}-role"); | |
| private static V1ClusterRole CreateClusterRole(IReadOnlyList<object> metadata, IEnumerable<IEventWatcher> watchers) | |
| { | |
| var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName) | |
| ?? throw new InvalidOperationException("OperatorNameAttribute is required."); | |
| var clusterrole = new ClusterRoleBuilder() | |
| .WithName($"{name}-role"); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/InstallCommand.cs` around lines 151 - 156,
CreateClusterRole can receive a null name from
metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName), so add
a null/empty check after that call and fail fast with a clear exception (e.g.,
throw new InvalidOperationException or ArgumentException) referencing the
missing OperatorNameAttribute; update the code around CreateClusterRole and the
ClusterRoleBuilder().WithName($"{name}-role") to only build when name is
non-null (or use the exception) so you never pass a null into WithName.
| public static TBuilder WithFinalizer<TBuilder>(this TBuilder builder, string finalizer) | ||
| where TBuilder : IControllerConventionBuilder | ||
| where TBuilder : ConventionBuilder<ControllerBuilder> | ||
| { | ||
| builder.WithMetadata(new FinalizerAttribute(finalizer)); | ||
| return builder; | ||
| } |
There was a problem hiding this comment.
Finalizer metadata should be single-valued.
WithFinalizer currently uses WithMetadata, so repeated calls will add multiple FinalizerAttribute entries. The watcher reads the first attribute, making the result order-dependent. Prefer WithSingle for predictable behavior.
🛠️ Suggested fix
- builder.WithMetadata(new FinalizerAttribute(finalizer));
+ builder.WithSingle(new FinalizerAttribute(finalizer));📝 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.
| public static TBuilder WithFinalizer<TBuilder>(this TBuilder builder, string finalizer) | |
| where TBuilder : IControllerConventionBuilder | |
| where TBuilder : ConventionBuilder<ControllerBuilder> | |
| { | |
| builder.WithMetadata(new FinalizerAttribute(finalizer)); | |
| return builder; | |
| } | |
| public static TBuilder WithFinalizer<TBuilder>(this TBuilder builder, string finalizer) | |
| where TBuilder : ConventionBuilder<ControllerBuilder> | |
| { | |
| builder.WithSingle(new FinalizerAttribute(finalizer)); | |
| return builder; | |
| } |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Generation/MetadataExtensions.cs` around lines 90 - 95,
WithFinalizer currently calls WithMetadata which appends multiple
FinalizerAttribute instances, making repeated calls order-dependent; change
WithFinalizer (the extension method on TBuilder where TBuilder :
ConventionBuilder<ControllerBuilder>) to call WithSingle(new
FinalizerAttribute(finalizer)) instead of WithMetadata so the metadata is
single-valued (replace the WithMetadata invocation on the builder with
WithSingle to ensure only one FinalizerAttribute is stored).
| namespace K8sOperator.NET; | ||
|
|
||
| internal static partial class LoggingExtensions | ||
| public static partial class LoggerExtensions | ||
| { | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the actual file to confirm the changes
echo "=== File content (lines 1-20) ==="
head -20 src/K8sOperator.NET/LoggerExtensions.cs
echo ""
echo "=== Searching for references to old namespace K8sOperator.NET.Extensions ==="
rg -n "K8sOperator\.NET\.Extensions" --type cs
echo ""
echo "=== Searching for references to old class name LoggingExtensions ==="
rg -n "LoggingExtensions" --type cs
echo ""
echo "=== Checking for any using statements that might import from Extensions ==="
rg -n "using.*Extensions" --type csRepository: pmdevers/K8sOperator.NET
Length of output: 3482
Breaking change: Namespace and visibility updated, but example file not updated.
The namespace moved from K8sOperator.NET.Extensions to K8sOperator.NET and visibility changed from internal to public. This breaks the example in examples/SimpleOperator/Program.cs:2, which still imports from the old namespace using K8sOperator.NET.Extensions;. This needs to be updated to using K8sOperator.NET;.
Minor nit: Line 8 has an extra blank line after the opening brace.
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/LoggerExtensions.cs` around lines 4 - 8, The example
import and a minor formatting issue need fixing: update the using directive in
the example file that references the old namespace to the new public namespace
(change the using in examples/SimpleOperator/Program.cs from
K8sOperator.NET.Extensions to K8sOperator.NET so it matches the public static
partial class LoggerExtensions), and remove the extra blank line after the
opening brace in LoggerExtensions.cs to tidy formatting.
| <PropertyGroup> | ||
| <PackageVersion Condition=" '$(PackageVersion)' == '' ">1.0.0</PackageVersion> | ||
| <OperatorName Condition=" '$(OperatorName)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())</OperatorName> | ||
| <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace> |
There was a problem hiding this comment.
Preserve backward-compatible default for OperatorNamespace.
If consumers override OperatorName but don’t set OperatorNamespace, the namespace metadata now falls back to the project name, which changes prior behavior and can be surprising. Consider defaulting OperatorNamespace to $(OperatorName)-system to keep compatibility while still allowing explicit override.
🛠️ Proposed fix
- <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace>
+ <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(OperatorName)-system</OperatorNamespace>📝 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.
| <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace> | |
| <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(OperatorName)-system</OperatorNamespace> |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Operator.targets` at line 6, The default for
OperatorNamespace currently falls back to the project name; change it so when
OperatorNamespace is empty it uses OperatorName + "-system" to preserve backward
compatibility for consumers who override OperatorName, and if OperatorName
itself is empty then still fall back to the existing
MSBuildProjectName.Replace(" ", "_").ToLower() behavior; update the
OperatorNamespace element (the one with the Condition="'$(OperatorNamespace)' ==
''") to compute "$(OperatorName)-system" when OperatorName is set and only use
the project-name replacement when OperatorName is not set.
| var task = Task.Run(async () => | ||
| { | ||
| try | ||
| { | ||
| await watcher.Start(cts.Token); | ||
| } | ||
| catch (OperatorException) | ||
| { | ||
| await cts.CancelAsync(); | ||
| throw; | ||
| } | ||
|
|
||
| } | ||
| , cts.Token); | ||
| _runningTasks.Add(task); | ||
| } | ||
|
|
||
| await Task.WhenAll(_runningTasks); |
There was a problem hiding this comment.
Non-OperatorException errors will crash the host.
Only OperatorException is caught and handled; any other exception thrown by watcher.Start() will propagate through Task.WhenAll and terminate the BackgroundService. If graceful degradation is desired, consider wrapping all exceptions or logging unexpected failures before deciding whether to cancel sibling watchers.
Potential approach
var task = Task.Run(async () =>
{
try
{
await watcher.Start(cts.Token);
}
catch (OperatorException)
{
await cts.CancelAsync();
throw;
}
+ catch (Exception ex) when (ex is not OperationCanceledException)
+ {
+ logger.LogError(ex, "Watcher failed unexpectedly");
+ await cts.CancelAsync();
+ throw;
+ }
}
, cts.Token);📝 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.
| var task = Task.Run(async () => | |
| { | |
| try | |
| { | |
| await watcher.Start(cts.Token); | |
| } | |
| catch (OperatorException) | |
| { | |
| await cts.CancelAsync(); | |
| throw; | |
| } | |
| } | |
| , cts.Token); | |
| _runningTasks.Add(task); | |
| } | |
| await Task.WhenAll(_runningTasks); | |
| var task = Task.Run(async () => | |
| { | |
| try | |
| { | |
| await watcher.Start(cts.Token); | |
| } | |
| catch (OperatorException) | |
| { | |
| await cts.CancelAsync(); | |
| throw; | |
| } | |
| catch (Exception ex) when (ex is not OperationCanceledException) | |
| { | |
| logger.LogError(ex, "Watcher failed unexpectedly"); | |
| await cts.CancelAsync(); | |
| throw; | |
| } | |
| } | |
| , cts.Token); | |
| _runningTasks.Add(task); | |
| } | |
| await Task.WhenAll(_runningTasks); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorService.cs` around lines 29 - 46, The code only
catches OperatorException from watcher.Start and rethrows, allowing any other
exception to bubble up through Task.WhenAll and crash the host; update the
Task.Run block that calls watcher.Start to catch Exception as well, log
unexpected errors (use the service logger, e.g., _logger), and call
cts.CancelAsync() to cancel sibling watchers instead of letting the exception
propagate—keep rethrowing only for OperatorException if you need existing
behavior, otherwise swallow non-OperatorException after logging and cancelling
so Task.WhenAll doesn't terminate the background service unexpectedly; ensure
you reference watcher.Start, OperatorException, cts.CancelAsync, _runningTasks
and Task.WhenAll when making the changes.
6c805b5 to
b7dc178
Compare
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)
.github/workflows/security-analysis.yml (1)
30-44: Add required permissions block for CodeQL v4 compatibility.CodeQL v4 requires explicit workflow/job permissions. The workflow is missing the mandatory
security-events: writepermission, which CodeQL v4 needs to upload results. Add the permissions block at the workflow or job level:permissions: security-events: write contents: readAlso note: CodeQL v4 removed the
add-snippetsinput from the analyze action (removed in v4.31.1). If your workflow uses it, remove that input.
🧹 Nitpick comments (2)
test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj (1)
1-8: Consider usingMicrosoft.NET.Sdk.Webfor ASP.NET Core MVC testing.The
Microsoft.AspNetCore.Mvc.Testingdocumentation recommends: "Specify the Web SDK in the test project file (<Project Sdk="Microsoft.NET.Sdk.Web">)." This ensures proper content root resolution and static file handling during integration tests.♻️ Suggested change
-<Project Sdk="Microsoft.NET.Sdk"> +<Project Sdk="Microsoft.NET.Sdk.Web">global.json (1)
6-7: Test runner configuration looks good.The
Microsoft.Testing.Platformrunner configuration is the recommended approach for the new .NET testing infrastructure.Minor nit: the indentation is inconsistent with the
sdksection (8 spaces vs 6 spaces for nested keys). Consider aligning for consistency.Suggested formatting alignment
"test": { - "runner": "Microsoft.Testing.Platform" + "runner": "Microsoft.Testing.Platform" }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/security-analysis.ymlglobal.jsontest/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csproj
⏰ 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/K8sOperator.NET.Tests.csproj (2)
15-17: LGTM!The project reference to the main library project is correctly configured with a relative path.
11-12: No action needed. TUnit version 1.11.28 exists on NuGet and is a valid package reference. The latest available version is 1.11.51, but 1.11.28 is a stable release that can be used as-is or updated if desired.Likely an incorrect or invalid review comment.
global.json (1)
2-4: .NET 10 SDK version10.0.100is appropriate.The .NET 10 SDK reached general availability on November 11, 2025, and is stable as of January 2026 with servicing updates available. This version is suitable for CI/CD environments.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pmdevers
force-pushed
the
entire-rewrite
branch
from
b7dc178 to
55977e7
Compare
pmdevers
force-pushed
the
entire-rewrite
branch
2 times, most recently
from
8a17f10 to
1ccb469
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
254-262: Bug: Boolean type incorrectly mapped to "string" instead of "boolean".In JSON Schema/OpenAPI, boolean values should use the
"boolean"type, not"string". This will cause CRD validation to fail for boolean fields or allow invalid string values.🐛 Proposed fix
if (type.FullName == "System.Boolean") { builder.Add(x => { - x.Type = "string"; + x.Type = "boolean"; x.Nullable = nullable; }); return builder; }
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/Commands/InstallCommand.cs`:
- Around line 51-52: Replace the unsafe use of .First() when retrieving the
KubernetesEntityAttribute in InstallCommand (the variable 'group') with a
.FirstOrDefault() and validate the result similar to EventWatcher.cs: check that
the retrieved KubernetesEntityAttribute is not null, and if it is null, throw or
return a clear descriptive error (or log and abort) indicating the missing
attribute so the failure is explicit; keep the existing .FirstOrDefault() usage
for 'scope' (NamespaceAttribute) and apply the same validation pattern for
consistency.
In `@src/K8sOperator.NET/EventWatcher.cs`:
- Around line 243-245: The call sequence in EventWatcher.cs uses the method
parameter cancellationToken for controller.AddOrModifyAsync but then calls
ReplaceAsync(resource, _cancellationToken) using the field _cancellationToken;
replace the field usage with the passed parameter to keep cancellation tokens
consistent — update the ReplaceAsync call to use cancellationToken (the method
parameter) instead of _cancellationToken, ensuring any callers of ReplaceAsync
within this method consistently propagate the same token.
In `@src/K8sOperator.NET/K8sOperator.NET.csproj`:
- Around line 15-26: Replace the undefined MSBuild property usage by changing
the PackageId and any other occurrences of $(ProjectName) to the built-in
$(MSBuildProjectName); specifically update the PackageId element and the package
URL properties that currently reference $(ProjectName) so they use
$(MSBuildProjectName) (search for the literal "$(ProjectName)" and replace with
"$(MSBuildProjectName)" in the csproj, e.g., the PackageId element and the two
package URL properties).
In `@src/K8sOperator.NET/OperatorExtensions.cs`:
- Around line 55-56: OperatorService is only registered via
services.TryAddSingleton<OperatorService>(), but since it derives from
BackgroundService (IHostedService) you must register it as a hosted service so
the host will run it; replace or augment the registration in OperatorExtensions
(where TryAddSingleton<OperatorService>() is called) with a hosted-service
registration such as services.AddHostedService<OperatorService>() (or
services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService,
OperatorService>())) to ensure OperatorService is started by the host.
- Around line 49-54: The current TryAddSingleton registration returns a concrete
Kubernetes instance, which prevents DI from resolving IKubernetes (used by
EventWatcher<T> and ActivatorUtilities.CreateInstance); change the registration
to register the IKubernetes service (e.g.,
services.TryAddSingleton<IKubernetes>(sp => { var config =
builder?.Configuration ?? KubernetesClientConfiguration.BuildDefaultConfig();
return new Kubernetes(config); })) so the container can resolve IKubernetes via
constructor injection while still returning the Kubernetes implementation.
In `@test/K8sOperator.NET.Tests/CommandDatasource_Tests.cs`:
- Around line 153-172: The test
GetCommands_Should_ApplyMultipleConventionsInOrder currently uses IsEquivalentTo
which ignores order; update the assertion on conventionOrder to assert exact
sequence by replacing IsEquivalentTo with Is.EqualTo (i.e., assert that
conventionOrder equals the expected ordered sequence) in the Assert.That call
inside that test so the applied convention order is enforced.
In `@test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.cs`:
- Around line 153-172: Update the assertion in the test
GetWatchers_Should_ApplyMultipleConventionsInOrder to validate order strictly:
replace the call to IsEquivalentTo on the conventionOrder assertion with
IsEqualTo and pass an ordered collection (e.g., new[] { 1, 2, 3 }) so the test
enforces the sequence produced by EventWatcherDatasource.GetWatchers().
In `@test/K8sOperator.NET.Tests/WatchEvents.cs`:
- Around line 8-18: The Added helper property incorrectly creates a Modified
event; update the Added property (the expression that calls CreateEvent) to use
WatchEventType.Added instead of WatchEventType.Modified so the Added watcher
helper returns the correct Kubernetes watch event type; ensure the change is
made where the Added property is defined and keeps the existing object payload
unchanged.
♻️ Duplicate comments (5)
src/K8sOperator.NET/LoggerExtensions.cs (1)
4-6: Confirm callers updated for namespace/type rename.This change is breaking for consumers still referencing
K8sOperator.NET.Extensions.LoggingExtensions. Please verify downstream references (examples/docs/tests) now useK8sOperator.NET.LoggerExtensions.#!/bin/bash # Find any lingering references to the old namespace or class name. rg -n "K8sOperator\.NET\.Extensions|LoggingExtensions" --type cstest/K8sOperator.NET.Tests/Logging/TestContextLoggingExtensions.cs (1)
8-31: Extension block syntax requires C# 13 preview features—code will not compile.The
extension(ILoggingBuilder logging)syntax at line 8 is a C# 13 preview feature. Unless the project explicitly enables preview features (<LangVersion>preview</LangVersion>or<EnablePreviewFeatures>true</EnablePreviewFeatures>), this code will fail to compile.🔧 Standard extension method pattern
public static class TestContextLoggingExtensions { - extension(ILoggingBuilder logging) + /// <summary> + /// Log to test output. + /// </summary> + /// <param name="logging"> + /// The global logging configuration. + /// </param> + /// <param name="testContext"> + /// Output for the current test. + /// </param> + /// <param name="minLogLevel"> + /// The minimum level to log at. + /// </param> + /// <returns> + /// The logging builder for chaining. + /// </returns> + public static ILoggingBuilder AddTestLogging( + this ILoggingBuilder logging, + TestContext testContext, + LogLevel minLogLevel = LogLevel.Information) { - /// <summary> - /// Log to test output. - /// </summary> - /// <param name="logging"> - /// The global logging configuration. - /// </param> - /// <param name="testContext"> - /// Output for the current test. - /// </param> - /// <param name="minLogLevel"> - /// The minimum level to log at. - /// </param> - public void AddTestLogging(TestContext testContext, - LogLevel minLogLevel = LogLevel.Information) - { - ArgumentNullException.ThrowIfNull(logging); - ArgumentNullException.ThrowIfNull(testContext); + ArgumentNullException.ThrowIfNull(logging); + ArgumentNullException.ThrowIfNull(testContext); - logging.AddProvider( - new TestContextLoggerProvider(testContext, minLogLevel)); - } + logging.AddProvider( + new TestContextLoggerProvider(testContext, minLogLevel)); + return logging; } }src/K8sOperator.NET/Commands/InstallCommand.cs (2)
80-82: Null-forgiving operators remain on potentially null values.The use of
!on lines 80-81 will causeNullReferenceExceptionifOperatorNameAttributeorDockerImageAttributeis missing. This was flagged in a previous review and remains unaddressed.
141-141: Null handling still missing forOperatorNameAttributelookup.Both
CreateClusterRoleBinding(line 141) andCreateClusterRole(line 153) retrievenameviaTryGetValuewithout null validation. If the attribute is missing, string interpolation will produce malformed names like"-role-binding". This was previously flagged.Also applies to: 153-153
src/K8sOperator.NET/OperatorService.cs (1)
29-43: Non-OperatorExceptionerrors will still crash the host.Only
OperatorExceptionis caught; any other exception (e.g., network failures, serialization errors) will propagate throughTask.WhenAlland terminate theBackgroundService. This was previously flagged and remains unaddressed.
🧹 Nitpick comments (9)
src/K8sOperator.NET/Generation/KubernetesObjectBuilderWithMetaData.cs (1)
16-23: Consider renaming the file to match the class name.The class has been renamed to
KubernetesObjectBuilderWithMetadatabut the filename is stillKubernetesObjectBuilderWithMetaData.cs. For consistency with C# conventions, the filename should match the class name.src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
404-418: Inconsistent nullability detection between the twoIsNullableoverloads.
IsNullable(PropertyInfo)properly usesNullabilityInfoContextfor accurate nullability detection, butIsNullable(Type)only checks if the type name contains "Nullable". This string-based check is fragile and can produce incorrect results (e.g., a class namedNullableContainerwould incorrectly returntrue).Consider aligning the approach or documenting the limitation.
♻️ Proposed fix for consistent behavior
/// <summary> /// Check if a type is nullable. /// </summary> /// <param name="type">The type.</param> - /// <returns>True if the type is nullable (i.e. contains "nullable" in its name).</returns> + /// <returns>True if the type is a Nullable<T> value type.</returns> public static bool IsNullable(this Type type) - => type.FullName?.Contains("Nullable") == true; + => Nullable.GetUnderlyingType(type) != null;test/K8sOperator.NET.Tests/Logging/TestContextLogger.cs (1)
103-118:AddProviderthrowingNotImplementedExceptionmay cause runtime failures.The
ILoggerFactorycontract expectsAddProviderto be callable. ThrowingNotImplementedExceptioncould break integrations where providers are added dynamically (e.g., viaILoggingBuilderextensions).Consider a no-op implementation or storing providers in a list (even if unused):
♻️ Suggested fix
public void AddProvider(ILoggerProvider provider) { - throw new NotImplementedException(); + // Intentionally no-op: this factory is for test-only logging + // and does not support additional providers. }Also note that
CreateLoggerhardcodesLogLevel.Debug—consider making this configurable if varying log levels are needed in tests.src/K8sOperator.NET/OperatorService.cs (1)
20-20: Minor: preferCount == 0over.Any()for materialized lists.Since
watchersis already aList<T>, usingwatchers.Count == 0is slightly more efficient than.Any()as it avoids enumerator allocation.Proposed fix
- if (!watchers.Any()) + if (watchers.Count == 0)test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (2)
20-26: Consider extracting attribute validation to reduce duplication.The attribute extraction and validation logic (lines 23-26 and 51-54) is duplicated. A private helper method would reduce repetition and ensure consistent error messaging.
Proposed helper extraction
private static (string group, string version, string plural) GetEntityInfo<T>() where T : CustomResource { var attr = typeof(T).GetCustomAttribute<KubernetesEntityAttribute>(); var group = attr?.Group ?? throw new InvalidOperationException($"KubernetesEntityAttribute.Group not defined on {typeof(T).FullName}"); var version = attr?.ApiVersion ?? throw new InvalidOperationException($"KubernetesEntityAttribute.Version not defined on {typeof(T).FullName}"); var plural = attr?.PluralName ?? throw new InvalidOperationException($"KubernetesEntityAttribute.Plural not defined on {typeof(T).FullName}"); return (group, version, plural); }Also applies to: 48-54
34-35: Inconsistent serialization:KubernetesJsonvsJsonSerializer.Line 34 uses
KubernetesJson.Serializewhile lines 59 and 63 useSystem.Text.Json.JsonSerializer. This inconsistency could cause subtle differences in serialization behavior (e.g., property naming conventions, null handling). Consider usingKubernetesJsonconsistently for Kubernetes objects.Proposed fix
- var requestBody = await JsonSerializer.DeserializeAsync<T>(context.Request.Body); + var requestBody = await KubernetesJson.DeserializeAsync<T>(context.Request.Body); resource?.Invoke(requestBody); - var jsonResponse = JsonSerializer.Serialize(requestBody); + var jsonResponse = KubernetesJson.Serialize(requestBody);Also applies to: 59-63
test/K8sOperator.NET.Tests/OperatorService_Tests.cs (1)
1-5: Empty test class - tests needed forOperatorService.This test class is a placeholder with no test methods. Given that
OperatorServiceis the core hosted service orchestrating watchers, consider adding tests for:
- No watchers registered (early exit path)
- Multiple watchers starting in parallel
- Cancellation propagation via
stoppingTokenOperatorExceptionhandling and sibling cancellationWould you like me to generate test scaffolding for these scenarios or open an issue to track this?
src/K8sOperator.NET/EventWatcher.cs (1)
49-49: Misplaced comment.The comment
// Handle each watch eventappears after the closing brace of theelse ifblock rather than at a meaningful location. Consider removing it or relocating to describe the switch logic inProccessEventAsync.test/K8sOperator.NET.Tests/CommandDatasource_Tests.cs (1)
210-230: Order test doesn’t prove ordering by Order property yet.
BecauseOperatorCommandis added first, the test passes even if ordering is ignored. Consider adding it last to verify ordering byOrder.🧪 Strengthen the order-by-Order test
- datasource.Add<OperatorCommand>(); // Order = -2 - datasource.Add<InstallCommand>(); // Order = 1 (default) - datasource.Add<TestCommand>(); // Order = 1 (default) + datasource.Add<InstallCommand>(); // Order = 1 (default) + datasource.Add<TestCommand>(); // Order = 1 (default) + datasource.Add<OperatorCommand>(); // Order = -2
| var group = item.Metadata.OfType<KubernetesEntityAttribute>().First(); | ||
| var scope = item.Metadata.OfType<INamespaceMetadata>().FirstOrDefault(); | ||
| var scope = item.Metadata.OfType<NamespaceAttribute>().FirstOrDefault(); |
There was a problem hiding this comment.
.First() can throw if KubernetesEntityAttribute is missing.
Using .First() on line 51 will throw InvalidOperationException if the metadata contains no KubernetesEntityAttribute. Consider using .FirstOrDefault() with validation, consistent with the pattern used in EventWatcher.cs.
Proposed fix
- var group = item.Metadata.OfType<KubernetesEntityAttribute>().First();
+ var group = item.Metadata.OfType<KubernetesEntityAttribute>().FirstOrDefault()
+ ?? throw new InvalidOperationException($"Watcher metadata must include a {nameof(KubernetesEntityAttribute)}.");📝 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.
| var group = item.Metadata.OfType<KubernetesEntityAttribute>().First(); | |
| var scope = item.Metadata.OfType<INamespaceMetadata>().FirstOrDefault(); | |
| var scope = item.Metadata.OfType<NamespaceAttribute>().FirstOrDefault(); | |
| var group = item.Metadata.OfType<KubernetesEntityAttribute>().FirstOrDefault() | |
| ?? throw new InvalidOperationException($"Watcher metadata must include a {nameof(KubernetesEntityAttribute)}."); | |
| var scope = item.Metadata.OfType<NamespaceAttribute>().FirstOrDefault(); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/InstallCommand.cs` around lines 51 - 52, Replace
the unsafe use of .First() when retrieving the KubernetesEntityAttribute in
InstallCommand (the variable 'group') with a .FirstOrDefault() and validate the
result similar to EventWatcher.cs: check that the retrieved
KubernetesEntityAttribute is not null, and if it is null, throw or return a
clear descriptive error (or log and abort) indicating the missing attribute so
the failure is explicit; keep the existing .FirstOrDefault() usage for 'scope'
(NamespaceAttribute) and apply the same validation pattern for consistency.
| await controller.AddOrModifyAsync(resource, cancellationToken); | ||
|
|
||
| resource = await ReplaceAsync(resource, _cancellationToken); |
There was a problem hiding this comment.
Inconsistent cancellation token usage.
Line 245 uses the field _cancellationToken while the method receives cancellationToken as a parameter. This could lead to unexpected behavior if the tokens differ. Consider using the passed parameter consistently.
Proposed fix
await controller.AddOrModifyAsync(resource, cancellationToken);
- resource = await ReplaceAsync(resource, _cancellationToken);
+ resource = await ReplaceAsync(resource, cancellationToken);📝 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 controller.AddOrModifyAsync(resource, cancellationToken); | |
| resource = await ReplaceAsync(resource, _cancellationToken); | |
| await controller.AddOrModifyAsync(resource, cancellationToken); | |
| resource = await ReplaceAsync(resource, cancellationToken); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/EventWatcher.cs` around lines 243 - 245, The call
sequence in EventWatcher.cs uses the method parameter cancellationToken for
controller.AddOrModifyAsync but then calls ReplaceAsync(resource,
_cancellationToken) using the field _cancellationToken; replace the field usage
with the passed parameter to keep cancellation tokens consistent — update the
ReplaceAsync call to use cancellationToken (the method parameter) instead of
_cancellationToken, ensuring any callers of ReplaceAsync within this method
consistently propagate the same token.
| <PropertyGroup> | ||
| <CompanyName>PMDEvers</CompanyName> | ||
| <PackageId>$(ProjectName)</PackageId> | ||
| <Description> | ||
| K8sOperator.NET is a powerful and intuitive library designed for creating Kubernetes Operators using C#. It simplifies the development of robust, cloud-native operators by leveraging the full capabilities of the .NET ecosystem, making it easier than ever to manage complex Kubernetes workloads with custom automation. | ||
| </Description> | ||
| <Authors>Patrick Evers</Authors> | ||
| <Owners>$(CompanyName)</Owners> | ||
| <PackageTags> | ||
|
|
||
| </PackageTags> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ProjectName property definitions/usages
rg -n "ProjectName" --type-list | head -20
echo "---"
rg -n "ProjectName" -g '*.props' -g '*.targets' -g '*.csproj'Repository: pmdevers/K8sOperator.NET
Length of output: 1293
🏁 Script executed:
#!/bin/bash
# Check the project file structure
ls -la src/K8sOperator.NET/Repository: pmdevers/K8sOperator.NET
Length of output: 1318
🏁 Script executed:
#!/bin/bash
# Look for Directory.Build.props or other central property files
find . -name "Directory.Build.props" -o -name "Directory.Build.targets" -o -name "*.props" -o -name "*.targets" | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Read the full csproj file
cat -n src/K8sOperator.NET/K8sOperator.NET.csprojRepository: pmdevers/K8sOperator.NET
Length of output: 2893
🏁 Script executed:
#!/bin/bash
# Look for Directory.Build.props at repository root
find . -maxdepth 2 -name "Directory.Build.props" -o -name "Directory.Build.targets"Repository: pmdevers/K8sOperator.NET
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check if ProjectName is defined in any csproj or props files
rg -n "<ProjectName>" -g '*.csproj' -g '*.props'Repository: pmdevers/K8sOperator.NET
Length of output: 50
PackageId uses undefined $(ProjectName) — will be empty.
The ProjectName property is not defined anywhere in this codebase, resulting in an empty PackageId, which breaks NuGet package metadata. Use $(MSBuildProjectName) instead, which is a built-in MSBuild property that evaluates to the project filename.
Proposed fix
- <PackageId>$(ProjectName)</PackageId>
+ <PackageId>$(MSBuildProjectName)</PackageId>Also update lines 33 and 36 where $(ProjectName) is used in package URLs.
📝 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.
| <PropertyGroup> | |
| <CompanyName>PMDEvers</CompanyName> | |
| <PackageId>$(ProjectName)</PackageId> | |
| <Description> | |
| K8sOperator.NET is a powerful and intuitive library designed for creating Kubernetes Operators using C#. It simplifies the development of robust, cloud-native operators by leveraging the full capabilities of the .NET ecosystem, making it easier than ever to manage complex Kubernetes workloads with custom automation. | |
| </Description> | |
| <Authors>Patrick Evers</Authors> | |
| <Owners>$(CompanyName)</Owners> | |
| <PackageTags> | |
| </PackageTags> | |
| </PropertyGroup> | |
| <PropertyGroup> | |
| <CompanyName>PMDEvers</CompanyName> | |
| <PackageId>$(MSBuildProjectName)</PackageId> | |
| <Description> | |
| K8sOperator.NET is a powerful and intuitive library designed for creating Kubernetes Operators using C#. It simplifies the development of robust, cloud-native operators by leveraging the full capabilities of the .NET ecosystem, making it easier than ever to manage complex Kubernetes workloads with custom automation. | |
| </Description> | |
| <Authors>Patrick Evers</Authors> | |
| <Owners>$(CompanyName)</Owners> | |
| <PackageTags> | |
| </PackageTags> | |
| </PropertyGroup> |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/K8sOperator.NET.csproj` around lines 15 - 26, Replace the
undefined MSBuild property usage by changing the PackageId and any other
occurrences of $(ProjectName) to the built-in $(MSBuildProjectName);
specifically update the PackageId element and the package URL properties that
currently reference $(ProjectName) so they use $(MSBuildProjectName) (search for
the literal "$(ProjectName)" and replace with "$(MSBuildProjectName)" in the
csproj, e.g., the PackageId element and the two package URL properties).
| services.TryAddSingleton((sp) => | ||
| { | ||
| var config = builder?.Configuration | ||
| ?? KubernetesClientConfiguration.BuildDefaultConfig(); | ||
| return new Kubernetes(config); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP "IKubernetes\b" -g'*.cs' -C2Repository: pmdevers/K8sOperator.NET
Length of output: 1779
🏁 Script executed:
cat -n src/K8sOperator.NET/OperatorExtensions.cs | head -70Repository: pmdevers/K8sOperator.NET
Length of output: 2829
🏁 Script executed:
fd -e cs | xargs rg "class Kubernetes" | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 589
🏁 Script executed:
fd -e cs | xargs rg "class OperatorService" -A 10Repository: pmdevers/K8sOperator.NET
Length of output: 1252
🏁 Script executed:
fd -e cs | xargs rg "class EventWatcherDatasource" -A 10Repository: pmdevers/K8sOperator.NET
Length of output: 2081
🏁 Script executed:
fd -e cs | xargs rg "EventWatcher<" -C 3Repository: pmdevers/K8sOperator.NET
Length of output: 4065
🏁 Script executed:
rg "Kubernetes\b" -g'*.cs' -C 1 | grep -E "(new Kubernetes|typeof\(Kubernetes\)|: Kubernetes|<Kubernetes>)" | head -30Repository: pmdevers/K8sOperator.NET
Length of output: 377
🏁 Script executed:
# Check if EventWatcher uses Kubernetes directly or through the interface
cat -n src/K8sOperator.NET/EventWatcher.cs | head -30Repository: pmdevers/K8sOperator.NET
Length of output: 1128
Register IKubernetes interface: current registration only provides the concrete Kubernetes type.
EventWatcher<T> requires IKubernetes via constructor injection and ActivatorUtilities.CreateInstance will fail to resolve it. Change the registration to use the interface:
Suggested fix
- services.TryAddSingleton((sp) =>
+ services.TryAddSingleton<IKubernetes>(sp =>
{
var config = builder?.Configuration
?? KubernetesClientConfiguration.BuildDefaultConfig();
return new Kubernetes(config);
});📝 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.
| services.TryAddSingleton((sp) => | |
| { | |
| var config = builder?.Configuration | |
| ?? KubernetesClientConfiguration.BuildDefaultConfig(); | |
| return new Kubernetes(config); | |
| }); | |
| services.TryAddSingleton<IKubernetes>(sp => | |
| { | |
| var config = builder?.Configuration | |
| ?? KubernetesClientConfiguration.BuildDefaultConfig(); | |
| return new Kubernetes(config); | |
| }); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorExtensions.cs` around lines 49 - 54, The current
TryAddSingleton registration returns a concrete Kubernetes instance, which
prevents DI from resolving IKubernetes (used by EventWatcher<T> and
ActivatorUtilities.CreateInstance); change the registration to register the
IKubernetes service (e.g., services.TryAddSingleton<IKubernetes>(sp => { var
config = builder?.Configuration ??
KubernetesClientConfiguration.BuildDefaultConfig(); return new
Kubernetes(config); })) so the container can resolve IKubernetes via constructor
injection while still returning the Kubernetes implementation.
| services.TryAddSingleton<OperatorService>(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find OperatorService class definition and check its inheritance
rg -nP "class\s+OperatorService\b" -g'*.cs' -C5Repository: pmdevers/K8sOperator.NET
Length of output: 951
🏁 Script executed:
#!/bin/bash
# Check the full context of OperatorExtensions.cs around lines 55-56
cat -n src/K8sOperator.NET/OperatorExtensions.cs | head -70 | tail -30Repository: pmdevers/K8sOperator.NET
Length of output: 1260
🏁 Script executed:
#!/bin/bash
# Check if OperatorService is registered as IHostedService anywhere else
rg -n "IHostedService" -g'*.cs' -C2Repository: pmdevers/K8sOperator.NET
Length of output: 50
OperatorService must be registered as IHostedService to be executed by the host.
OperatorService inherits from BackgroundService (which implements IHostedService), but registering it only as a singleton won't start it. Add explicit hosted-service registration so the host knows to execute it:
Suggested fix
services.TryAddSingleton<OperatorService>();
+ services.TryAddEnumerable(
+ ServiceDescriptor.Singleton<IHostedService>(sp => sp.GetRequiredService<OperatorService>())
+ );📝 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.
| services.TryAddSingleton<OperatorService>(); | |
| services.TryAddSingleton<OperatorService>(); | |
| services.TryAddEnumerable( | |
| ServiceDescriptor.Singleton<IHostedService>(sp => sp.GetRequiredService<OperatorService>()) | |
| ); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorExtensions.cs` around lines 55 - 56,
OperatorService is only registered via
services.TryAddSingleton<OperatorService>(), but since it derives from
BackgroundService (IHostedService) you must register it as a hosted service so
the host will run it; replace or augment the registration in OperatorExtensions
(where TryAddSingleton<OperatorService>() is called) with a hosted-service
registration such as services.AddHostedService<OperatorService>() (or
services.TryAddEnumerable(ServiceDescriptor.Singleton<IHostedService,
OperatorService>())) to ensure OperatorService is started by the host.
| [Test] | ||
| public async Task GetCommands_Should_ApplyMultipleConventionsInOrder() | ||
| { | ||
| // Arrange | ||
| var serviceProvider = CreateServiceProvider(); | ||
| var datasource = new CommandDatasource(serviceProvider); | ||
| var host = serviceProvider.GetRequiredService<IHost>(); | ||
| var conventionOrder = new List<int>(); | ||
|
|
||
| datasource.Add<TestCommand>() | ||
| .Add(builder => conventionOrder.Add(1)) | ||
| .Add(builder => conventionOrder.Add(2)) | ||
| .Add(builder => conventionOrder.Add(3)); | ||
|
|
||
| // Act | ||
| var commands = datasource.GetCommands(host).ToList(); | ||
|
|
||
| // Assert | ||
| await Assert.That(conventionOrder).IsEquivalentTo([1, 2, 3]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/K8sOperator.NET.Tests && head -30 CommandDatasource_Tests.csRepository: pmdevers/K8sOperator.NET
Length of output: 920
🏁 Script executed:
find . -name "*.csproj" -path "*K8sOperator.NET.Tests*" -exec cat {} \;Repository: pmdevers/K8sOperator.NET
Length of output: 685
🌐 Web query:
TUnit IsEquivalentTo order guarantee list behavior
💡 Result:
Short answer: By default IsEquivalentTo is order‑independent (it only checks same elements and counts). If you need order guaranteed, request/order‑match explicitly (TUnit: pass CollectionOrdering.Matching; in NUnit use an order‑sensitive constraint such as Is.EqualTo). [1][2]
Sources:
- TUnit Collection assertions (IsEquivalentTo default ignores order; use CollectionOrdering.Matching to require matching order). [1]
- NUnit CollectionEquivalentConstraint / Is.EquivalentTo (equivalence ignores order; use Is.EqualTo to compare order). [2]
Use IsEqualTo instead of IsEquivalentTo to verify convention order.
The test name indicates it should validate conventions are applied in order, but IsEquivalentTo only checks that the same elements exist (regardless of order). The assertion would pass even if conventionOrder were [3, 2, 1]. Use IsEqualTo to enforce exact order matching.
✅ Suggested assertion fix
- await Assert.That(conventionOrder).IsEquivalentTo([1, 2, 3]);
+ await Assert.That(conventionOrder).IsEqualTo([1, 2, 3]);🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/CommandDatasource_Tests.cs` around lines 153 -
172, The test GetCommands_Should_ApplyMultipleConventionsInOrder currently uses
IsEquivalentTo which ignores order; update the assertion on conventionOrder to
assert exact sequence by replacing IsEquivalentTo with Is.EqualTo (i.e., assert
that conventionOrder equals the expected ordered sequence) in the Assert.That
call inside that test so the applied convention order is enforced.
| [Test] | ||
| public async Task GetWatchers_Should_ApplyMultipleConventionsInOrder() | ||
| { | ||
| // Arrange | ||
| var serviceProvider = CreateServiceProvider(); | ||
| var metadata = CreateMetadata(); | ||
| var datasource = new EventWatcherDatasource(serviceProvider, metadata); | ||
| var conventionOrder = new List<int>(); | ||
|
|
||
| datasource.Add<TestController>() | ||
| .Add(builder => conventionOrder.Add(1)) | ||
| .Add(builder => conventionOrder.Add(2)) | ||
| .Add(builder => conventionOrder.Add(3)); | ||
|
|
||
| // Act | ||
| var watchers = datasource.GetWatchers().ToList(); | ||
|
|
||
| // Assert | ||
| await Assert.That(conventionOrder).IsEquivalentTo([1, 2, 3]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the project file to identify dependencies
fd -e "\.csproj$" -o -e "\.sln$" | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 184
🏁 Script executed:
# Search for NUnit/Fluent Assertions references
fd -e "csproj" | xargs grep -l "Fluent\|NUnit" | head -5Repository: pmdevers/K8sOperator.NET
Length of output: 50
🏁 Script executed:
# Check the test file to see imports and understand the framework
head -30 test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.csRepository: pmdevers/K8sOperator.NET
Length of output: 875
🏁 Script executed:
# Find test project file
find . -name "*Tests.csproj" -o -name "*Test.csproj"Repository: pmdevers/K8sOperator.NET
Length of output: 125
🏁 Script executed:
# Read the test project file to check dependencies
find . -path "*/test/*" -name "*.csproj" | head -1 | xargs catRepository: pmdevers/K8sOperator.NET
Length of output: 685
🏁 Script executed:
# Also check for package references to confirm Fluent Assertions
rg -i "FluentAssertions|NUnit" --type xmlRepository: pmdevers/K8sOperator.NET
Length of output: 216
🌐 Web query:
TUnit IsEquivalentTo vs IsEqualTo assertion order checking
💡 Result:
Short answer: IsEqualTo checks value equality (uses Equals/==) — for collections it compares as-is (order matters / arrays may use reference equality), while IsEquivalentTo asserts that two collections contain the same elements ignoring order by default (use CollectionOrdering.Matching to require the same order). Use IsEqualTo when you need strict equality (including order/types); use IsEquivalentTo when you care about the same members irrespective of order. [1][2][3]
Sources:
- TUnit Collections (IsEquivalentTo, ordering option). [1]
- TUnit Equality & Comparison (IsEqualTo behavior). [2]
- TUnit Troubleshooting (collection comparison guidance). [3]
🏁 Script executed:
# Search for TUnit assertions usage in the test file
rg -A 3 "IsEquivalentTo|IsEqualTo" test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.csRepository: pmdevers/K8sOperator.NET
Length of output: 1962
🏁 Script executed:
# Search all test files for usage patterns of these assertions
rg "IsEquivalentTo|IsEqualTo" test/ -B 2 -A 2 | head -50Repository: pmdevers/K8sOperator.NET
Length of output: 3869
Use IsEqualTo instead of IsEquivalentTo to validate convention order.
The test GetWatchers_Should_ApplyMultipleConventionsInOrder() requires strict order validation, but IsEquivalentTo ignores collection order by default in TUnit. Switch to IsEqualTo to enforce the expected sequence [1, 2, 3].
Suggested fix
- await Assert.That(conventionOrder).IsEquivalentTo([1, 2, 3]);
+ await Assert.That(conventionOrder).IsEqualTo([1, 2, 3]);📝 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.
| [Test] | |
| public async Task GetWatchers_Should_ApplyMultipleConventionsInOrder() | |
| { | |
| // Arrange | |
| var serviceProvider = CreateServiceProvider(); | |
| var metadata = CreateMetadata(); | |
| var datasource = new EventWatcherDatasource(serviceProvider, metadata); | |
| var conventionOrder = new List<int>(); | |
| datasource.Add<TestController>() | |
| .Add(builder => conventionOrder.Add(1)) | |
| .Add(builder => conventionOrder.Add(2)) | |
| .Add(builder => conventionOrder.Add(3)); | |
| // Act | |
| var watchers = datasource.GetWatchers().ToList(); | |
| // Assert | |
| await Assert.That(conventionOrder).IsEquivalentTo([1, 2, 3]); | |
| } | |
| [Test] | |
| public async Task GetWatchers_Should_ApplyMultipleConventionsInOrder() | |
| { | |
| // Arrange | |
| var serviceProvider = CreateServiceProvider(); | |
| var metadata = CreateMetadata(); | |
| var datasource = new EventWatcherDatasource(serviceProvider, metadata); | |
| var conventionOrder = new List<int>(); | |
| datasource.Add<TestController>() | |
| .Add(builder => conventionOrder.Add(1)) | |
| .Add(builder => conventionOrder.Add(2)) | |
| .Add(builder => conventionOrder.Add(3)); | |
| // Act | |
| var watchers = datasource.GetWatchers().ToList(); | |
| // Assert | |
| await Assert.That(conventionOrder).IsEqualTo([1, 2, 3]); | |
| } |
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.cs` around lines 153
- 172, Update the assertion in the test
GetWatchers_Should_ApplyMultipleConventionsInOrder to validate order strictly:
replace the call to IsEquivalentTo on the conventionOrder assertion with
IsEqualTo and pass an ordered collection (e.g., new[] { 1, 2, 3 }) so the test
enforces the sequence produced by EventWatcherDatasource.GetWatchers().
| public static Watcher<T>.WatchEvent Added => CreateEvent(WatchEventType.Modified, | ||
| new T() | ||
| { | ||
| Metadata = new() | ||
| { | ||
| Name = "test", | ||
| NamespaceProperty = "default", | ||
| Finalizers = ["finalize"], | ||
| Uid = "1" | ||
| } | ||
| }); |
There was a problem hiding this comment.
Bug: Added property uses incorrect WatchEventType.Modified.
The property is named Added but creates a WatchEventType.Modified event. This will cause tests relying on this helper to simulate the wrong Kubernetes watch event type.
🐛 Proposed fix
- public static Watcher<T>.WatchEvent Added => CreateEvent(WatchEventType.Modified,
+ public static Watcher<T>.WatchEvent Added => CreateEvent(WatchEventType.Added,📝 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.
| public static Watcher<T>.WatchEvent Added => CreateEvent(WatchEventType.Modified, | |
| new T() | |
| { | |
| Metadata = new() | |
| { | |
| Name = "test", | |
| NamespaceProperty = "default", | |
| Finalizers = ["finalize"], | |
| Uid = "1" | |
| } | |
| }); | |
| public static Watcher<T>.WatchEvent Added => CreateEvent(WatchEventType.Added, | |
| new T() | |
| { | |
| Metadata = new() | |
| { | |
| Name = "test", | |
| NamespaceProperty = "default", | |
| Finalizers = ["finalize"], | |
| Uid = "1" | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/WatchEvents.cs` around lines 8 - 18, The Added
helper property incorrectly creates a Modified event; update the Added property
(the expression that calls CreateEvent) to use WatchEventType.Added instead of
WatchEventType.Modified so the Added watcher helper returns the correct
Kubernetes watch event type; ensure the change is made where the Added property
is defined and keeps the existing object payload unchanged.
1ccb469 to
1a46742
Compare
Excludes the `generate-launchsettings` command from the help output. Adds a `ShowInHelp` property to the `OperatorArgumentAttribute` to control visibility in the help command. This prevents cluttering the help command with utility commands not intended for direct user interaction.
pmdevers
force-pushed
the
entire-rewrite
branch
from
8b6f012 to
51158bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
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/Generation/DeploymentBuilderExtensions.cs (2)
211-215: Wrong parameter type: should beV1SecretKeySelector, notV1ConfigMapKeySelector.The method
AddEnvFromSecretKeyis documented as sourcing values from a secret key, but the parameter type isV1ConfigMapKeySelector. This will cause theAddEnvgeneric method to produce aConfigMapKeyRefinstead of aSecretKeyRefat runtime.🐛 Proposed fix
- public static TBuilder AddEnvFromSecretKey<TBuilder>(this TBuilder builder, string name, Action<V1ConfigMapKeySelector> action) + public static TBuilder AddEnvFromSecretKey<TBuilder>(this TBuilder builder, string name, Action<V1SecretKeySelector> action) where TBuilder : IKubernetesObjectBuilder<V1Container> { return builder.AddEnv(name, action); }
300-305: Logic inversion:RunAsRoot(true)setsRunAsNonRoot = true, which is the opposite of the intended behavior.When
runAsRoot = true, the method setsx.RunAsNonRoot = true, which prevents running as root. The assignment should be inverted.🐛 Proposed fix
public static TBuilder RunAsRoot<TBuilder>(this TBuilder builder, bool runAsRoot = true) where TBuilder : IKubernetesObjectBuilder<V1SecurityContext> { - builder.Add(x => x.RunAsNonRoot = runAsRoot); + builder.Add(x => x.RunAsNonRoot = !runAsRoot); return builder; }src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
254-262: Bug: Boolean type incorrectly mapped to "string" instead of "boolean".The
System.Booleantype is being mapped to JSON schema type"string"on line 258, but it should be"boolean". This will cause incorrect CRD schema generation for boolean properties.Proposed fix
if (type.FullName == "System.Boolean") { builder.Add(x => { - x.Type = "string"; + x.Type = "boolean"; x.Nullable = nullable; }); return builder; }
🤖 Fix all issues with AI agents
In `@examples/SimpleOperator/Controllers/TodoController.cs`:
- Around line 15-35: Add a null-guard for resource.Spec at the start of
AddOrModifyAsync (method in TodoController) to avoid dereferencing a malformed
CRD: check if resource.Spec is null, handle it by initializing a safe default
Spec or setting resource.Status appropriately (e.g., mark error or increment
ReconciliationCount and set State), log a warning including
resource.Metadata.Name, and then return or continue without accessing
resource.Spec; ensure subsequent uses of resource.Spec (e.g., Spec.Title,
Spec.Priority, Spec.DueDate) only occur after this guard so AddOrModifyAsync and
updates to resource.Status.ReconciliationCount and State are safe.
In `@examples/SimpleOperator/Properties/launchSettings.json`:
- Line 44: The launchSettings.json uses the incorrect key "schema" which tooling
won't recognize; replace the top-level "schema" property with "$schema" in the
launchSettings.json so the file binds to
"http://json.schemastore.org/launchsettings.json" and restores schema-based
validation and IntelliSense.
In `@src/K8sOperator.NET/LaunchSettings.json`:
- Around line 11-15: The Install profile in LaunchSettings.json uses shell
redirection in its commandLineArgs ("install > ./install.yaml"), which Visual
Studio will pass literally and not execute; update the Install profile's
commandLineArgs to just "install" (remove "> ./install.yaml") or replace the
profile with an Executable wrapper that runs cmd.exe /C "dotnet run -- install >
install.yaml" or a small script that performs the redirection, referencing the
Install profile and its commandLineArgs property to locate the change.
In `@src/K8sOperator.NET/Operator.targets`:
- Around line 67-69: The Exec task that runs "dotnet run --no-build --
generate-launchsettings" should pass the MSBuild configuration so the correct
build output is used; update the Command string in the Exec task (the Exec
element invoking dotnet run) to include "-c $(Configuration)" (e.g., "dotnet run
--no-build -c $(Configuration) -- generate-launchsettings") so Release builds
run the proper configuration.
In `@test/K8sOperator.NET.Target.Tests/K8sOperator.NET.Target.Tests.csproj`:
- Around line 1-17: Add the missing TestingPlatformDotnetTestSupport property to
the test project's PropertyGroup: set
<TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport> inside
the existing PropertyGroup that contains TargetFramework/net10.0,
EnablePreviewFeatures and CreateOperatorLaunchSettings in
K8sOperator.NET.Target.Tests.csproj so the .NET 10 SDK test support is enabled
for TUnit.
In `@test/K8sOperator.NET.Tests/CommandDatasource_Tests.cs`:
- Around line 210-229: The test currently relies on insertion order because
CommandDatasource.Add initializes Order = 1 for every entry; update the test to
explicitly set non-default Order values so ordering is validated: when creating
entries via CommandDatasource.Add (or by using any available overload/returned
entry), set the Order property on the added entries (or use a new Add overload
that accepts an order) for OperatorCommand, InstallCommand and TestCommand so
GetCommands(host) must sort by the Order property; reference
CommandDatasource.Add, CommandDatasource.GetCommands, and the Order property on
the command entries (and the concrete types OperatorCommand, InstallCommand,
TestCommand) when making the change.
In `@test/K8sOperator.NET.Tests/EventWatcher_Tests.cs`:
- Around line 28-31: The test config uses NamespaceAttribute("default") so
EventWatcher.GetWatchStream() will call the namespaced watch API, but the mock
registers WatchListClusterCustomObjectAsync, causing a route mismatch; fix by
either registering a namespaced watch on the mock or updating the test metadata:
add a WatchListNamespacedCustomObjectAsync handler to
MockKubeApiBuilder.CustomObjects (and have MockKubeApiServer call it in the
namespaced path) or remove NamespaceAttribute("default") from the test metadata
so the cluster-wide WatchListClusterCustomObjectAsync matches
EventWatcher.GetWatchStream().
- Around line 40-55: The test configures a cluster-scoped mock endpoint
(WatchListClusterCustomObjectAsync) while the TestResource metadata has
NamespaceAttribute("default"), so EventWatcher<TestResource> will perform a
namespaced watch and miss the mock; update the mock to the namespaced variant
(use WatchListNamespacedCustomObjectAsync and ensure
ReplaceNamespacedCustomObjectAsync remains) or alternatively change TestResource
metadata to be cluster-scoped; locate the calls to
WatchListClusterCustomObjectAsync and ReplaceNamespacedCustomObjectAsync in the
MockKubeApiServer setup and make the mock watch match the resource scope used by
EventWatcher.
- Around line 93-109: The test uses an async lambda in
ReplaceNamespacedCustomObjectAsync (which expects an Action<TestResource?>)
causing an async void and lost assertions; update the callback in the
EventWatcher_Tests.HandleAddOrModifyAsync_Should_AddFinalizer_IfNotPresent test
to use a synchronous Action that captures the incoming TestResource (or signals
a TaskCompletionSource) and then perform the Assert.That on that captured value
outside the callback (awaiting the TaskCompletionSource if used), so that the
assertion failures are observed by the test runner; change the
ReplaceNamespacedCustomObjectAsync delegate from async x => { await Assert... }
to a non-async capture/signal approach and assert afterward.
♻️ Duplicate comments (11)
src/K8sOperator.NET/LoggerExtensions.cs (1)
4-8: Namespace and formatting issues already flagged.The namespace change breaking the example file and the extra blank line at line 8 were already identified in a previous review.
test/K8sOperator.NET.Tests/CommandDatasource_Tests.cs (1)
153-171: Use an order-sensitive assertion for convention order.
IsEquivalentToignores ordering, so this can pass with the wrong sequence.✅ Suggested fix
- await Assert.That(conventionOrder).IsEquivalentTo([1, 2, 3]); + await Assert.That(conventionOrder).IsEqualTo([1, 2, 3]);src/K8sOperator.NET/Commands/InstallCommand.cs (4)
139-149: Null check needed fornameinCreateClusterRoleBinding.Same issue as other methods -
namecan benullifOperatorNameAttributeis missing, causing issues when used in string interpolation.Proposed fix
private static V1ClusterRoleBinding CreateClusterRoleBinding(IReadOnlyList<object> metadata) { - var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName); + var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName) + ?? throw new InvalidOperationException("OperatorNameAttribute is required.");
51-52:.First()can throw ifKubernetesEntityAttributeis missing.This issue was flagged in a previous review. Using
.First()will throwInvalidOperationExceptionif metadata contains noKubernetesEntityAttribute. Consider using.FirstOrDefault()with validation.Proposed fix
- var group = item.Metadata.OfType<KubernetesEntityAttribute>().First(); + var group = item.Metadata.OfType<KubernetesEntityAttribute>().FirstOrDefault() + ?? throw new InvalidOperationException($"Watcher metadata must include a {nameof(KubernetesEntityAttribute)}.");
78-82: PotentialNullReferenceExceptionif metadata attributes are missing.This issue was flagged in a previous review.
TryGetValuereturnsnullif the attribute is not found, and the null-forgiving operator (!) will cause NRE ifOperatorNameAttributeorDockerImageAttributeis missing.Proposed fix
- var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName)!; - var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())!; + var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName) + ?? throw new InvalidOperationException("OperatorNameAttribute is required for deployment generation."); + var image = metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage()) + ?? throw new InvalidOperationException("DockerImageAttribute is required for deployment generation.");
151-156: Null check needed fornameinCreateClusterRole.Same null-safety issue as flagged in previous review.
Proposed fix
private static V1ClusterRole CreateClusterRole(IReadOnlyList<object> metadata, IEnumerable<IEventWatcher> watchers) { - var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName); + var name = metadata.TryGetValue<OperatorNameAttribute, string>(x => x.OperatorName) + ?? throw new InvalidOperationException("OperatorNameAttribute is required.");src/K8sOperator.NET/EventWatcher.cs (1)
243-250: Inconsistent cancellation token usage persists.This was flagged in a previous review. Line 243 uses the
cancellationTokenparameter, but line 245 uses the field_cancellationToken. This could lead to unexpected behavior if the tokens differ.Proposed fix
await controller.AddOrModifyAsync(resource, cancellationToken); - resource = await ReplaceAsync(resource, _cancellationToken); + resource = await ReplaceAsync(resource, cancellationToken);src/K8sOperator.NET/OperatorService.cs (1)
29-46: Unhandled non-OperatorExceptionfaults will crash the BackgroundService.Any unexpected exception from
watcher.Startwill faultTask.WhenAlland terminate the service. Consider logging and canceling sibling watchers without letting the task fault.🛠️ Suggested fix
var task = Task.Run(async () => { try { await watcher.Start(cts.Token); } catch (OperatorException) { await cts.CancelAsync(); throw; } + catch (Exception ex) when (ex is not OperationCanceledException) + { + logger.LogError(ex, "Watcher failed unexpectedly"); + await cts.CancelAsync(); + } } , cts.Token);src/K8sOperator.NET/Operator.targets (1)
17-20: DefaultOperatorNamespaceshould followOperatorNamewhen set.Line 19 still defaults to the project name, which breaks compatibility when
OperatorNameis overridden butOperatorNamespaceis not.🛠️ Suggested fix
- <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace> + <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' and '$(OperatorName)' != '' ">$(OperatorName)-system</OperatorNamespace> + <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' and '$(OperatorName)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace>src/K8sOperator.NET/OperatorExtensions.cs (2)
50-55: RegisterIKubernetesso DI can resolve interface dependencies.Line 50 registers only the concrete
Kubernetestype; consumers that depend onIKuberneteswon’t resolve.🛠️ Suggested fix
- services.TryAddSingleton((sp) => + services.TryAddSingleton<IKubernetes>(sp => { var config = builder?.Configuration ?? KubernetesClientConfiguration.BuildDefaultConfig(); return new Kubernetes(config); });
56-56: RegisterOperatorServiceas a hosted service so it actually runs.Line 56 registers
OperatorServiceas a singleton only; it won’t be executed by the host without anIHostedServiceregistration.🛠️ Suggested fix
services.TryAddSingleton<OperatorService>(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton<IHostedService>(sp => sp.GetRequiredService<OperatorService>()) + );
🧹 Nitpick comments (11)
test/K8sOperator.NET.Tests/OperatorService_Tests.cs (1)
3-5: Empty test class — placeholder or missing implementation?This test class has no test methods. Based on the PR context, tests are expected for
OperatorService-related components. If this is a placeholder for future tests, consider adding a TODO comment or removing the file until tests are implemented.Would you like me to help generate test scaffolding for
OperatorServicebased on patterns fromControllerTests.cs?test/K8sOperator.NET.Tests/Logging/TestContextLoggerProvider.cs (2)
9-15: Duplicate summary lines and inconsistent parameter documentation.The constructor has two summary sentences (lines 10-11). Additionally, the
testContextparameter doc (line 14) still says "The output for the current test" which doesn't accurately describe aTestContext.📝 Suggested fix
/// <summary> - /// Initializes a new instance of the <see cref="TestContextLoggerProvider"/> class. - /// Create a new <see cref="TestContextLoggerProvider"/>. + /// Initializes a new instance of the <see cref="TestContextLoggerProvider"/> class. /// </summary> /// <param name="testContext"> - /// The output for the current test. + /// The current test context. /// </param>
34-37: Property documentation doesn't match the type.The summary says "The output for the current test" but the property is of type
TestContext.📝 Suggested fix
/// <summary> - /// The output for the current test. + /// The current test context. /// </summary> private TestContext Context { get; }test/K8sOperator.NET.Tests/Logging/TestContextLogger.cs (2)
9-15: Same documentation issues as the provider class.Duplicate summary lines and param doc that doesn't match the parameter type.
📝 Suggested fix
/// <summary> - /// Initializes a new instance of the <see cref="TestContextLogger"/> class. - /// Create a new <see cref="TestContextLogger"/>. + /// Initializes a new instance of the <see cref="TestContextLogger"/> class. /// </summary> /// <param name="context"> - /// The output for the current test. + /// The current test context. /// </param>
36-39: Property documentation doesn't match the type.📝 Suggested fix
/// <summary> - /// The output for the current test. + /// The current test context. /// </summary> public TestContext Context { get; }src/K8sOperator.NET/Commands/GenerateLaunchSettingsCommand.cs (1)
12-55: Materialize commands once to avoid repeated instantiation.
GetCommandsyields new instances per enumeration; this method enumerates it multiple times (profiles, count, list). Cache to a list for consistency and to avoid extra work.♻️ Suggested update
- var commands = commandDatasource.GetCommands(host); + var commands = commandDatasource.GetCommands(host).ToList(); @@ - Console.WriteLine($" Found {commands.Count()} command(s):"); + Console.WriteLine($" Found {commands.Count} command(s):");src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
404-418: Consider improving robustness and performance ofIsNullablemethods.Two concerns:
Line 410: Using
FullName?.Contains("Nullable")is fragile and could match unintended types (e.g., a custom type namedMyNullableWrapper). Consider checking against the specificNullable<>generic type definition instead.Line 417: Creating a new
NullabilityInfoContexton every property check is inefficient. Consider caching or passing a context instance.Proposed improvement
public static bool IsNullable(this Type type) - => type.FullName?.Contains("Nullable") == true; + => type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>); + private static readonly NullabilityInfoContext _nullabilityContext = new(); + public static bool IsNullable(this PropertyInfo prop) - => new NullabilityInfoContext().Create(prop).ReadState == NullabilityState.Nullable || - prop.PropertyType.FullName?.Contains("Nullable") == true; + => _nullabilityContext.Create(prop).ReadState == NullabilityState.Nullable || + prop.PropertyType.IsNullable();test/K8sOperator.NET.Tests/Mocks/MockKubeApiServer.cs (2)
56-66: Inconsistent JSON serializer usage.Line 59 uses
System.Text.Json.JsonSerializer.DeserializeAsync<T>()while line 63 usesSystem.Text.Json.JsonSerializer.Serialize(). However, line 40 usesKubernetesJson.Serialize(). Consider usingKubernetesJsonconsistently for Kubernetes object serialization to ensure proper handling of Kubernetes-specific types.Proposed fix for consistency
builder.MapPut($"/apis/{group}/{version}/namespaces/{ns}/{plural}/{{name}}", async context => { // Mock replacing a custom resource - var requestBody = await JsonSerializer.DeserializeAsync<T>(context.Request.Body); + var requestBody = await KubernetesJson.DeserializeAsync<T>(context.Request.Body); resource?.Invoke(requestBody); - var jsonResponse = JsonSerializer.Serialize(requestBody); + var jsonResponse = KubernetesJson.Serialize(requestBody); context.Response.ContentType = "application/json"; await context.Response.WriteAsync(jsonResponse); });
126-136: Dispose pattern silently swallows all exceptions.The empty
catchblock at line 132 silently swallows all exceptions duringStopAsync. While ignoring disposal errors can be acceptable, consider at least logging them at a debug/trace level for troubleshooting test failures.examples/SimpleOperator/Controllers/TodoController.cs (2)
33-38: CaptureDateTime.UtcNowonce for consistent overdue logic.Line 33 and Line 38 call
UtcNowtwice, which can drift between comparison and message. Capture once for consistency.♻️ Suggested refactor
- // Business logic: Auto-complete if due date is passed - if (resource.Spec.DueDate.HasValue && - resource.Spec.DueDate.Value < DateTime.UtcNow && + // Business logic: Auto-complete if due date is passed + var utcNow = DateTime.UtcNow; + if (resource.Spec.DueDate.HasValue && + resource.Spec.DueDate.Value < utcNow && resource.Status.State != "completed") { resource.Status.State = "overdue"; - resource.Status.Message = $"Task is overdue by {(DateTime.UtcNow - resource.Spec.DueDate.Value).Days} days"; + resource.Status.Message = $"Task is overdue by {(utcNow - resource.Spec.DueDate.Value).Days} days"; _logger.LogWarning("TodoItem {Name} is overdue!", resource.Metadata.Name); }
52-88: Avoidasyncstate machines when only returning completed tasks.Line 52 through Line 88 use
await Task.CompletedTask;inasyncmethods. This adds overhead without value.♻️ Suggested refactor
- public override async Task DeleteAsync(TodoItem resource, CancellationToken cancellationToken) + public override Task DeleteAsync(TodoItem resource, CancellationToken cancellationToken) { _logger.LogInformation( "Deleting TodoItem: {Name}. Final state was: {State}", resource.Metadata.Name, resource.Status?.State ?? "unknown"); // Cleanup logic here (e.g., remove external resources) - await Task.CompletedTask; + return Task.CompletedTask; } - public override async Task FinalizeAsync(TodoItem resource, CancellationToken cancellationToken) + public override Task FinalizeAsync(TodoItem resource, CancellationToken cancellationToken) { _logger.LogInformation( "Finalizing TodoItem: {Name}", resource.Metadata.Name); // Perform cleanup before resource is completely deleted // For example: remove related resources, notify external systems, etc. - await Task.CompletedTask; + return Task.CompletedTask; } - public override async Task ErrorAsync(TodoItem resource, CancellationToken cancellationToken) + public override Task ErrorAsync(TodoItem resource, CancellationToken cancellationToken) { _logger.LogError( "Error occurred for TodoItem: {Name}", resource.Metadata.Name); if (resource.Status != null) { resource.Status.State = "error"; resource.Status.Message = "An error occurred during reconciliation"; } - await Task.CompletedTask; + return Task.CompletedTask; }
| public override async Task AddOrModifyAsync(TodoItem resource, CancellationToken cancellationToken) | ||
| { | ||
| _logger.LogInformation( | ||
| "Processing TodoItem: {Name} - Title: {Title}, Priority: {Priority}", | ||
| resource.Metadata.Name, | ||
| resource.Spec.Title, | ||
| resource.Spec.Priority); | ||
|
|
||
| // Initialize status if needed | ||
| if (resource.Status == null) | ||
| { | ||
| resource.Status = new TodoItem.TodoStatus(); | ||
| } | ||
|
|
||
| // Update reconciliation count | ||
| resource.Status.ReconciliationCount++; | ||
|
|
||
| // Business logic: Auto-complete if due date is passed | ||
| if (resource.Spec.DueDate.HasValue && | ||
| resource.Spec.DueDate.Value < DateTime.UtcNow && | ||
| resource.Status.State != "completed") |
There was a problem hiding this comment.
Guard against null Spec to avoid reconciliation crashes.
Line 18 and Line 33 dereference resource.Spec without a null check. A malformed CRD will throw and break the controller loop.
🛠️ Suggested fix
public override async Task AddOrModifyAsync(TodoItem resource, CancellationToken cancellationToken)
{
+ if (resource.Spec is null)
+ {
+ _logger.LogWarning(
+ "TodoItem {Name} has no spec; skipping reconciliation",
+ resource.Metadata?.Name ?? "<unknown>");
+ return;
+ }
+
_logger.LogInformation(
"Processing TodoItem: {Name} - Title: {Title}, Priority: {Priority}",
resource.Metadata.Name,
resource.Spec.Title,📝 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.
| public override async Task AddOrModifyAsync(TodoItem resource, CancellationToken cancellationToken) | |
| { | |
| _logger.LogInformation( | |
| "Processing TodoItem: {Name} - Title: {Title}, Priority: {Priority}", | |
| resource.Metadata.Name, | |
| resource.Spec.Title, | |
| resource.Spec.Priority); | |
| // Initialize status if needed | |
| if (resource.Status == null) | |
| { | |
| resource.Status = new TodoItem.TodoStatus(); | |
| } | |
| // Update reconciliation count | |
| resource.Status.ReconciliationCount++; | |
| // Business logic: Auto-complete if due date is passed | |
| if (resource.Spec.DueDate.HasValue && | |
| resource.Spec.DueDate.Value < DateTime.UtcNow && | |
| resource.Status.State != "completed") | |
| public override async Task AddOrModifyAsync(TodoItem resource, CancellationToken cancellationToken) | |
| { | |
| if (resource.Spec is null) | |
| { | |
| _logger.LogWarning( | |
| "TodoItem {Name} has no spec; skipping reconciliation", | |
| resource.Metadata?.Name ?? "<unknown>"); | |
| return; | |
| } | |
| _logger.LogInformation( | |
| "Processing TodoItem: {Name} - Title: {Title}, Priority: {Priority}", | |
| resource.Metadata.Name, | |
| resource.Spec.Title, | |
| resource.Spec.Priority); | |
| // Initialize status if needed | |
| if (resource.Status == null) | |
| { | |
| resource.Status = new TodoItem.TodoStatus(); | |
| } | |
| // Update reconciliation count | |
| resource.Status.ReconciliationCount++; | |
| // Business logic: Auto-complete if due date is passed | |
| if (resource.Spec.DueDate.HasValue && | |
| resource.Spec.DueDate.Value < DateTime.UtcNow && | |
| resource.Status.State != "completed") |
🤖 Prompt for AI Agents
In `@examples/SimpleOperator/Controllers/TodoController.cs` around lines 15 - 35,
Add a null-guard for resource.Spec at the start of AddOrModifyAsync (method in
TodoController) to avoid dereferencing a malformed CRD: check if resource.Spec
is null, handle it by initializing a safe default Spec or setting
resource.Status appropriately (e.g., mark error or increment ReconciliationCount
and set State), log a warning including resource.Metadata.Name, and then return
or continue without accessing resource.Spec; ensure subsequent uses of
resource.Spec (e.g., Spec.Title, Spec.Priority, Spec.DueDate) only occur after
this guard so AddOrModifyAsync and updates to
resource.Status.ReconciliationCount and State are safe.
| "dotnetRunMessages": true | ||
| } | ||
| }, | ||
| "schema": "http://json.schemastore.org/launchsettings.json" |
There was a problem hiding this comment.
Use $schema for launchSettings.json schema binding.
Line 44 uses schema, which most tooling won’t recognize for launchSettings.json. This will disable schema-based validation and IntelliSense.
🛠️ Suggested fix
- "schema": "http://json.schemastore.org/launchsettings.json"
+ "$schema": "http://json.schemastore.org/launchsettings.json"📝 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.
| "schema": "http://json.schemastore.org/launchsettings.json" | |
| "$schema": "http://json.schemastore.org/launchsettings.json" |
🤖 Prompt for AI Agents
In `@examples/SimpleOperator/Properties/launchSettings.json` at line 44, The
launchSettings.json uses the incorrect key "schema" which tooling won't
recognize; replace the top-level "schema" property with "$schema" in the
launchSettings.json so the file binds to
"http://json.schemastore.org/launchsettings.json" and restores schema-based
validation and IntelliSense.
| "Install": { | ||
| "commandName": "Project", | ||
| "commandLineArgs": "install > ./install.yaml", | ||
| "environmentVariables": { | ||
| "ASPNETCORE_ENVIRONMENT": "Development" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
launchSettings.json commandLineArgs shell redirection > .NET Core Visual Studio
💡 Result:
Short answer: Visual Studio does not honor shell output redirection (>) when launching a project via launchSettings.json commandLineArgs. Input redirection (<) can work for Project profiles if the file is copied to the output folder, but stdout/stderr redirection is an IDE limitation — use a wrapper (batch/powershell) or run from a shell instead. [1][2][3]
Why: launchSettings.json only passes raw arguments to the process; the shell operator > is interpreted by a shell, not by your app, and Visual Studio does not run the launched process through a shell that interprets >. [3][5]
Workarounds
- Run from a real shell: dotnet run -- > out.txt (or run the built exe): do this in CMD/PowerShell outside Visual Studio.
- Use a wrapper script and launch that from Visual Studio:
- Create run-with-redirect.bat:
@echooff- dotnet "%~dp0YourApp.dll" %* > "%~dp0out.txt" 2>&1
- In launchSettings.json use an Executable profile:
- "commandName": "Executable",
- "executablePath": "path\to\run-with-redirect.bat",
- or set "executablePath": "cmd.exe", "commandLineArgs": "/C "dotnet YourApp.dll > out.txt 2>&1""
This forces shell redirection because cmd.exe (or the batch file) performs the >. [5][2]
- Create run-with-redirect.bat:
Example (launchSettings.json snippet using cmd.exe):
{
"profiles": {
"WithRedirect": {
"commandName": "Executable",
"executablePath": "C:\Windows\System32\cmd.exe",
"commandLineArgs": "/C "dotnet %USERPROFILE%\source\repos\MyApp\bin\Debug\netX\MyApp.dll > %USERPROFILE%\out.txt 2>&1"",
"workingDirectory": "%USERPROFILE%\source\repos\MyApp\bin\Debug\netX"
}
}
}
Notes
- If you use input redirection (<) for a Project profile, ensure the input file is copied to output (CopyToOutputDirectory) so the debugger-launched process can find it. [2]
- Visual Studio Code may behave differently (uses launch.json and can use a shell or call scripts), but the underlying issue (> is a shell operator) remains. [4]
Sources
- StackOverflow – redirect input to .NET Core Console App on debugging (input redirection and CopyToOutputDirectory). [2]
- PVQ / discussion — Visual Studio can't redirect stdout from command-line-arguments; use shell/manual run or wrapper. [5]
- Visual Studio / docs on launchSettings.json and how args are passed (and limitations). [4]
References:
[1] https://stackoverflow.com/questions/67322668/redirect-input-to-net-core-console-app-on-debugging-in-visual-studio-2019
[2] https://pvq.app/questions/16262394/how-to-redirect-stdout-of-a-csharp-project-to-file-using-the-visual-studio-command-line-arguments-option
[3] https://code.visualstudio.com/docs/csharp/debugger-settings
Remove shell redirection from commandLineArgs—it won't execute.
Visual Studio does not interpret shell operators like > in commandLineArgs; they are passed as literal arguments to the process instead. The current "install > ./install.yaml" won't redirect output to a file.
If output redirection is needed, run the command from a shell outside Visual Studio (e.g., dotnet run -- install > install.yaml from PowerShell/CMD), or use a wrapper script or Executable profile with cmd.exe to enable shell interpretation.
Suggested change
- "commandLineArgs": "install > ./install.yaml",
+ "commandLineArgs": "install",📝 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.
| "Install": { | |
| "commandName": "Project", | |
| "commandLineArgs": "install > ./install.yaml", | |
| "environmentVariables": { | |
| "ASPNETCORE_ENVIRONMENT": "Development" | |
| "Install": { | |
| "commandName": "Project", | |
| "commandLineArgs": "install", | |
| "environmentVariables": { | |
| "ASPNETCORE_ENVIRONMENT": "Development" |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/LaunchSettings.json` around lines 11 - 15, The Install
profile in LaunchSettings.json uses shell redirection in its commandLineArgs
("install > ./install.yaml"), which Visual Studio will pass literally and not
execute; update the Install profile's commandLineArgs to just "install" (remove
"> ./install.yaml") or replace the profile with an Executable wrapper that runs
cmd.exe /C "dotnet run -- install > install.yaml" or a small script that
performs the redirection, referencing the Install profile and its
commandLineArgs property to locate the change.
| <Exec Command="dotnet run --no-build -- generate-launchsettings" | ||
| WorkingDirectory="$(MSBuildProjectDirectory)" | ||
| IgnoreExitCode="false" /> |
There was a problem hiding this comment.
Pass build configuration to dotnet run --no-build.
Line 67 runs dotnet run --no-build without -c $(Configuration), so Release builds may fail or run stale Debug outputs.
🛠️ Suggested fix
- <Exec Command="dotnet run --no-build -- generate-launchsettings"
+ <Exec Command="dotnet run --no-build -c $(Configuration) -f $(TargetFramework) -- generate-launchsettings"
WorkingDirectory="$(MSBuildProjectDirectory)"
IgnoreExitCode="false" />📝 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.
| <Exec Command="dotnet run --no-build -- generate-launchsettings" | |
| WorkingDirectory="$(MSBuildProjectDirectory)" | |
| IgnoreExitCode="false" /> | |
| <Exec Command="dotnet run --no-build -c $(Configuration) -f $(TargetFramework) -- generate-launchsettings" | |
| WorkingDirectory="$(MSBuildProjectDirectory)" | |
| IgnoreExitCode="false" /> |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Operator.targets` around lines 67 - 69, The Exec task
that runs "dotnet run --no-build -- generate-launchsettings" should pass the
MSBuild configuration so the correct build output is used; update the Command
string in the Exec task (the Exec element invoking dotnet run) to include "-c
$(Configuration)" (e.g., "dotnet run --no-build -c $(Configuration) --
generate-launchsettings") so Release builds run the proper configuration.
| <Project Sdk="Microsoft.NET.Sdk.Web"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net10.0</TargetFramework> | ||
| <EnablePreviewFeatures>true</EnablePreviewFeatures> | ||
| <!-- Enable launch settings creation for testing --> | ||
| <CreateOperatorLaunchSettings>true</CreateOperatorLaunchSettings> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="TUnit" Version="1.11.28" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\K8sOperator.NET\K8sOperator.NET.csproj" /> | ||
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
.NET 10 preview release date current stable version 2025
💡 Result:
- .NET 10 preview: Preview 1 — March 12, 2025; Preview 3 — April 11, 2025. [2][3]
- Current stable in 2025: .NET 9 (STS, released November 12, 2024) was the latest stable during much of 2025; .NET 8 remained the supported LTS until .NET 10 GA (November 11, 2025). [1][2]
🌐 Web query:
TUnit 1.11.28 compatibility Microsoft.NET.Sdk.Web net10.0
💡 Result:
Short answer: Yes — TUnit (packages target .NET 8+/netstandard2.0) works with net10.0, but you must enable the new dotnet test support because Microsoft.Testing.Platform removed VSTest target support in the .NET 10 SDK. Add this to your test project's csproj (Microsoft.NET.Sdk.Web or other SDK):
trueReferences: Reqnroll/TUnit guidance on .NET 10 SDK compatibility. [1] TUnit NuGet package target frameworks (net8.0 / netstandard2.0 / net9.0 info). [2]
Sources:
[1] Reqnroll docs — TUnit .NET 10 SDK Compatibility.
[2] NuGet: TUnit package details.
Add required .NET 10 SDK testing platform configuration.
The test project is missing <TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport> in the <PropertyGroup>. With .NET 10 SDK, Microsoft.Testing.Platform removed VSTest target support, and this property must be enabled for tests to run properly with TUnit. Add this property to your csproj.
Required addition
<PropertyGroup>
<TargetFramework>net10.0</TargetFramework>
<EnablePreviewFeatures>true</EnablePreviewFeatures>
<CreateOperatorLaunchSettings>true</CreateOperatorLaunchSettings>
<TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport>
</PropertyGroup>🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Target.Tests/K8sOperator.NET.Target.Tests.csproj` around
lines 1 - 17, Add the missing TestingPlatformDotnetTestSupport property to the
test project's PropertyGroup: set
<TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport> inside
the existing PropertyGroup that contains TargetFramework/net10.0,
EnablePreviewFeatures and CreateOperatorLaunchSettings in
K8sOperator.NET.Target.Tests.csproj so the .NET 10 SDK test support is enabled
for TUnit.
| [Test] | ||
| public async Task GetCommands_Should_OrderCommandsByOrderProperty() | ||
| { | ||
| // Arrange | ||
| var serviceProvider = CreateServiceProvider(); | ||
| var datasource = new CommandDatasource(serviceProvider); | ||
| var host = serviceProvider.GetRequiredService<IHost>(); | ||
|
|
||
| // Add commands (they'll be ordered by Order property internally) | ||
| datasource.Add<OperatorCommand>(); // Order = -2 | ||
| datasource.Add<InstallCommand>(); // Order = 1 (default) | ||
| datasource.Add<TestCommand>(); // Order = 1 (default) | ||
|
|
||
| // Act | ||
| var commands = datasource.GetCommands(host).ToList(); | ||
|
|
||
| // Assert | ||
| await Assert.That(commands).Count().IsEqualTo(3); | ||
| // OperatorCommand should be first due to Order = -2 | ||
| await Assert.That(commands[0].Command).IsTypeOf<OperatorCommand>(); |
There was a problem hiding this comment.
Test does not actually validate ordering by Order property.
CommandDatasource.Add currently initializes Order = 1 for every entry, so this test passes due to insertion order, not ordering logic. Consider setting an explicit order via API (if available) or adjusting the test to exercise a non-default order.
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/CommandDatasource_Tests.cs` around lines 210 -
229, The test currently relies on insertion order because CommandDatasource.Add
initializes Order = 1 for every entry; update the test to explicitly set
non-default Order values so ordering is validated: when creating entries via
CommandDatasource.Add (or by using any available overload/returned entry), set
the Order property on the added entries (or use a new Add overload that accepts
an order) for OperatorCommand, InstallCommand and TestCommand so
GetCommands(host) must sort by the Order property; reference
CommandDatasource.Add, CommandDatasource.GetCommands, and the Order property on
the command entries (and the concrete types OperatorCommand, InstallCommand,
TestCommand) when making the change.
| using var server = new MockKubeApiServer(endpoints => | ||
| { | ||
| endpoints.CustomObjects.WatchListClusterCustomObjectAsync(WatchEvents<TestResource>.Added); | ||
| }); |
There was a problem hiding this comment.
Watch endpoint mismatch: Cluster endpoint configured but namespaced watch expected.
The test metadata at line 18 includes NamespaceAttribute("default"), which causes EventWatcher.GetWatchStream() to call WatchListNamespacedCustomObjectAsync. However, the mock at line 30 configures WatchListClusterCustomObjectAsync (a cluster-wide endpoint). This mismatch will cause the watch to hit the unhandled route fallback.
Consider using a namespaced watch mock or removing the NamespaceAttribute from metadata if cluster-wide behavior is intended.
Option 1: Add namespaced watch method to MockKubeApiBuilder
The MockKubeApiBuilder.CustomObjectsImpl currently only has WatchListClusterCustomObjectAsync. You may need to add a WatchListNamespacedCustomObjectAsync method that handles the namespaced path pattern.
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/EventWatcher_Tests.cs` around lines 28 - 31, The
test config uses NamespaceAttribute("default") so EventWatcher.GetWatchStream()
will call the namespaced watch API, but the mock registers
WatchListClusterCustomObjectAsync, causing a route mismatch; fix by either
registering a namespaced watch on the mock or updating the test metadata: add a
WatchListNamespacedCustomObjectAsync handler to MockKubeApiBuilder.CustomObjects
(and have MockKubeApiServer call it in the namespaced path) or remove
NamespaceAttribute("default") from the test metadata so the cluster-wide
WatchListClusterCustomObjectAsync matches EventWatcher.GetWatchStream().
| [Test] | ||
| public async Task OnEvent_Should_HandleAddedEventAndCallAddOrModifyAsync() | ||
| { | ||
| var cancellationToken = _tokenSource.Token; | ||
| using var server = new MockKubeApiServer(endpoints => | ||
| { | ||
| endpoints.CustomObjects.WatchListClusterCustomObjectAsync(WatchEvents<TestResource>.Added); | ||
| endpoints.CustomObjects.ReplaceNamespacedCustomObjectAsync<TestResource>(); | ||
| }); | ||
|
|
||
| var watcher = new EventWatcher<TestResource>(server.Client, _controller, _metadata, _loggerFactory); | ||
|
|
||
| await watcher.Start(cancellationToken); | ||
|
|
||
| await _controller.Received(1).AddOrModifyAsync(Arg.Any<TestResource>(), Arg.Any<CancellationToken>()); | ||
| } |
There was a problem hiding this comment.
Same watch endpoint mismatch issue.
This test also configures WatchListClusterCustomObjectAsync (line 46) while metadata includes NamespaceAttribute("default"). The EventWatcher will attempt a namespaced watch that won't match the configured mock endpoint.
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/EventWatcher_Tests.cs` around lines 40 - 55, The
test configures a cluster-scoped mock endpoint
(WatchListClusterCustomObjectAsync) while the TestResource metadata has
NamespaceAttribute("default"), so EventWatcher<TestResource> will perform a
namespaced watch and miss the mock; update the mock to the namespaced variant
(use WatchListNamespacedCustomObjectAsync and ensure
ReplaceNamespacedCustomObjectAsync remains) or alternatively change TestResource
metadata to be cluster-scoped; locate the calls to
WatchListClusterCustomObjectAsync and ReplaceNamespacedCustomObjectAsync in the
MockKubeApiServer setup and make the mock watch match the resource scope used by
EventWatcher.
| [Test] | ||
| public async Task HandleAddOrModifyAsync_Should_AddFinalizer_IfNotPresent() | ||
| { | ||
| var cancellationToken = _tokenSource.Token; | ||
| using var server = new MockKubeApiServer(endpoints => | ||
| { | ||
| endpoints.CustomObjects.WatchListClusterCustomObjectAsync(WatchEvents<TestResource>.Added); | ||
| endpoints.CustomObjects.ReplaceNamespacedCustomObjectAsync<TestResource>(resource: async x => | ||
| { | ||
| await Assert.That(x?.Metadata.Finalizers).Contains("finalize"); | ||
| }); | ||
| }); | ||
|
|
||
| var watcher = new EventWatcher<TestResource>(server.Client, _controller, _metadata, _loggerFactory); | ||
|
|
||
| await watcher.Start(cancellationToken); | ||
| } |
There was a problem hiding this comment.
Async assertion inside synchronous Action callback may not execute properly.
Lines 100-103 use async x => { await Assert.That(...) } inside ReplaceNamespacedCustomObjectAsync which expects Action<T?>. This creates an async void delegate that won't propagate exceptions to the test runner, potentially causing silent test failures.
Consider using a TaskCompletionSource pattern or capturing values for assertion outside the callback.
Proposed fix using captured value
[Test]
public async Task HandleAddOrModifyAsync_Should_AddFinalizer_IfNotPresent()
{
var cancellationToken = _tokenSource.Token;
+ TestResource? capturedResource = null;
+
using var server = new MockKubeApiServer(endpoints =>
{
endpoints.CustomObjects.WatchListClusterCustomObjectAsync(WatchEvents<TestResource>.Added);
- endpoints.CustomObjects.ReplaceNamespacedCustomObjectAsync<TestResource>(resource: async x =>
- {
- await Assert.That(x?.Metadata.Finalizers).Contains("finalize");
- });
+ endpoints.CustomObjects.ReplaceNamespacedCustomObjectAsync<TestResource>(resource: x =>
+ {
+ capturedResource = x;
+ });
});
var watcher = new EventWatcher<TestResource>(server.Client, _controller, _metadata, _loggerFactory);
await watcher.Start(cancellationToken);
+
+ await Assert.That(capturedResource?.Metadata.Finalizers).Contains("finalize");
}🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/EventWatcher_Tests.cs` around lines 93 - 109, The
test uses an async lambda in ReplaceNamespacedCustomObjectAsync (which expects
an Action<TestResource?>) causing an async void and lost assertions; update the
callback in the
EventWatcher_Tests.HandleAddOrModifyAsync_Should_AddFinalizer_IfNotPresent test
to use a synchronous Action that captures the incoming TestResource (or signals
a TaskCompletionSource) and then perform the Assert.That on that captured value
outside the callback (awaiting the TaskCompletionSource if used), so that the
assertion failures are observed by the test runner; change the
ReplaceNamespacedCustomObjectAsync delegate from async x => { await Assert... }
to a non-async capture/signal approach and assert afterward.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/Commands/GenerateLaunchSettingsCommand.cs`:
- Around line 14-55: Materialize the commands sequence and validate that
PascalCased profile keys are unique before calling ToDictionary to avoid
ArgumentException; e.g., change commands to a List by calling ToList() on
commandDatasource.GetCommands(host), compute keys via ToPascalCase on each
command's OperatorArgumentAttribute.Argument, detect duplicates using
GroupBy(key).Where(g => g.Count() > 1) and throw an informative exception naming
the conflicting commands/keys, then build the profiles dictionary from the
materialized list (use the stored attribute/argument to avoid repeated OfType
calls).
- Around line 34-35: In GenerateLaunchSettingsCommand (the code that constructs
the launch settings object where you currently set schema =
"http://json.schemastore.org/launchsettings.json"), change the emitted property
name from "schema" to "$schema" so editor tooling picks up the JSON schema;
locate the object initializer assigning schema and replace the key with
"$schema" (ensuring your JSON serializer preserves the $ in the property name).
| var commandDatasource = host.Services.GetRequiredService<CommandDatasource>(); | ||
| var commands = commandDatasource.GetCommands(host); | ||
|
|
||
| var launchSettings = new | ||
| { | ||
| profiles = commands | ||
| .Where(c => c.Metadata.OfType<OperatorArgumentAttribute>().Any()) | ||
| .ToDictionary( | ||
| c => ToPascalCase(c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument), | ||
| c => new | ||
| { | ||
| commandName = "Project", | ||
| commandLineArgs = c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument, | ||
| environmentVariables = new Dictionary<string, string> | ||
| { | ||
| ["ASPNETCORE_ENVIRONMENT"] = "Development" | ||
| }, | ||
| dotnetRunMessages = true | ||
| } | ||
| ), | ||
| schema = "http://json.schemastore.org/launchsettings.json" | ||
| }; | ||
|
|
||
| var json = JsonSerializer.Serialize(launchSettings, new JsonSerializerOptions | ||
| { | ||
| WriteIndented = true, | ||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | ||
| DictionaryKeyPolicy = null // Don't change dictionary keys | ||
| }); | ||
|
|
||
| var propertiesDir = Path.Combine(Directory.GetCurrentDirectory(), "Properties"); | ||
| Directory.CreateDirectory(propertiesDir); | ||
|
|
||
| var launchSettingsPath = Path.Combine(propertiesDir, "launchSettings.json"); | ||
| await File.WriteAllTextAsync(launchSettingsPath, json); | ||
|
|
||
| Console.WriteLine($"✅ Generated launchSettings.json at: {launchSettingsPath}"); | ||
| Console.WriteLine($" Found {commands.Count()} command(s):"); | ||
|
|
||
| foreach (var cmd in commands) | ||
| { | ||
| var arg = cmd.Metadata.OfType<OperatorArgumentAttribute>().FirstOrDefault(); |
There was a problem hiding this comment.
Prevent duplicate profile keys after PascalCase conversion.
Line 21 can throw ArgumentException if two commands map to the same PascalCase key (e.g., foo-bar vs foo_bar). Materialize once and validate uniqueness (also avoids re-instantiating commands on multiple enumerations).
🐛 Proposed fix
- var commandDatasource = host.Services.GetRequiredService<CommandDatasource>();
- var commands = commandDatasource.GetCommands(host);
+ var commandDatasource = host.Services.GetRequiredService<CommandDatasource>();
+ var commands = commandDatasource.GetCommands(host).ToList();
+
+ var duplicateProfiles = commands
+ .Select(c => c.Metadata.OfType<OperatorArgumentAttribute>().FirstOrDefault())
+ .Where(a => a != null)
+ .GroupBy(a => ToPascalCase(a!.Argument))
+ .Where(g => g.Count() > 1)
+ .Select(g => g.Key)
+ .ToList();
+
+ if (duplicateProfiles.Count > 0)
+ throw new InvalidOperationException(
+ $"Duplicate launchSettings profile names after PascalCase conversion: {string.Join(", ", duplicateProfiles)}");
var launchSettings = new
{
- profiles = commands
- .Where(c => c.Metadata.OfType<OperatorArgumentAttribute>().Any())
- .ToDictionary(
- c => ToPascalCase(c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument),
- c => new
+ profiles = commands
+ .Select(c => new { Command = c, Arg = c.Metadata.OfType<OperatorArgumentAttribute>().FirstOrDefault() })
+ .Where(x => x.Arg != null)
+ .ToDictionary(
+ x => ToPascalCase(x.Arg!.Argument),
+ x => new
{
commandName = "Project",
- commandLineArgs = c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument,
+ commandLineArgs = x.Arg!.Argument,
environmentVariables = new Dictionary<string, string>
{
["ASPNETCORE_ENVIRONMENT"] = "Development"
},
dotnetRunMessages = true
}
),
schema = "http://json.schemastore.org/launchsettings.json"
};
@@
- Console.WriteLine($" Found {commands.Count()} command(s):");
+ Console.WriteLine($" Found {commands.Count} command(s):");🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/GenerateLaunchSettingsCommand.cs` around lines
14 - 55, Materialize the commands sequence and validate that PascalCased profile
keys are unique before calling ToDictionary to avoid ArgumentException; e.g.,
change commands to a List by calling ToList() on
commandDatasource.GetCommands(host), compute keys via ToPascalCase on each
command's OperatorArgumentAttribute.Argument, detect duplicates using
GroupBy(key).Where(g => g.Count() > 1) and throw an informative exception naming
the conflicting commands/keys, then build the profiles dictionary from the
materialized list (use the stored attribute/argument to avoid repeated OfType
calls).
| schema = "http://json.schemastore.org/launchsettings.json" | ||
| }; |
There was a problem hiding this comment.
Use $schema key for tooling compatibility.
Line 34 emits "schema" instead of "$schema", so editor tooling won’t pick up the launchSettings schema.
🛠️ Suggested change
- var launchSettings = new
- {
- profiles = commands
- .Where(c => c.Metadata.OfType<OperatorArgumentAttribute>().Any())
- .ToDictionary(
- c => ToPascalCase(c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument),
- c => new
- {
- commandName = "Project",
- commandLineArgs = c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument,
- environmentVariables = new Dictionary<string, string>
- {
- ["ASPNETCORE_ENVIRONMENT"] = "Development"
- },
- dotnetRunMessages = true
- }
- ),
- schema = "http://json.schemastore.org/launchsettings.json"
- };
+ var launchSettings = new Dictionary<string, object?>
+ {
+ ["$schema"] = "http://json.schemastore.org/launchsettings.json",
+ ["profiles"] = commands
+ .Where(c => c.Metadata.OfType<OperatorArgumentAttribute>().Any())
+ .ToDictionary(
+ c => ToPascalCase(c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument),
+ c => new
+ {
+ commandName = "Project",
+ commandLineArgs = c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument,
+ environmentVariables = new Dictionary<string, string>
+ {
+ ["ASPNETCORE_ENVIRONMENT"] = "Development"
+ },
+ dotnetRunMessages = true
+ }
+ )
+ };📝 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.
| schema = "http://json.schemastore.org/launchsettings.json" | |
| }; | |
| var launchSettings = new Dictionary<string, object?> | |
| { | |
| ["$schema"] = "http://json.schemastore.org/launchsettings.json", | |
| ["profiles"] = commands | |
| .Where(c => c.Metadata.OfType<OperatorArgumentAttribute>().Any()) | |
| .ToDictionary( | |
| c => ToPascalCase(c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument), | |
| c => new | |
| { | |
| commandName = "Project", | |
| commandLineArgs = c.Metadata.OfType<OperatorArgumentAttribute>().First().Argument, | |
| environmentVariables = new Dictionary<string, string> | |
| { | |
| ["ASPNETCORE_ENVIRONMENT"] = "Development" | |
| }, | |
| dotnetRunMessages = true | |
| } | |
| ) | |
| }; |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/GenerateLaunchSettingsCommand.cs` around lines
34 - 35, In GenerateLaunchSettingsCommand (the code that constructs the launch
settings object where you currently set schema =
"http://json.schemastore.org/launchsettings.json"), change the emitted property
name from "schema" to "$schema" so editor tooling picks up the JSON schema;
locate the object initializer assigning schema and replace the key with
"$schema" (ensuring your JSON serializer preserves the $ in the property name).
Introduces a new command to generate a Dockerfile and .dockerignore file for containerizing the operator. This enhancement simplifies the deployment process by providing a standardized Dockerfile, and improves the developer experience by automating the creation of necessary Docker assets.
Updates the test step to run tests for each project in the `test` directory. This ensures that all test projects are executed during the build process.
Introduces git hooks to automate tasks and improve code quality. Specifically, it adds a pre-commit hook that runs unit tests before each commit, preventing commits with failing tests. Includes installation scripts for both PowerShell (Windows) and Bash (Linux/Mac) to simplify the setup process. Also includes a README with instructions on how to install and bypass the hooks.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@examples/SimpleOperator/Dockerfile`:
- Around line 38-40: The HEALTHCHECK currently runs "dotnet SimpleOperator.dll
version" which is invalid because Program.cs runs the ASP.NET Core server via
app.RunOperatorAsync() and expects the "operator" startup argument; update the
Dockerfile HEALTHCHECK to verify the HTTP server instead (e.g., use curl -f
against the app's health endpoint like http://localhost:8080/health) or remove
the HEALTHCHECK entirely and rely on Kubernetes liveness/readiness probes;
ensure you modify the HEALTHCHECK instruction (the HEALTHCHECK line in the
Dockerfile) rather than changing Program.cs.
In `@src/K8sOperator.NET/Commands/GenerateDockerfileCommand.cs`:
- Around line 10-12: GenerateDockerfileCommand currently accepts an IHost host
parameter but never uses it, causing unused-parameter warnings; update the
primary constructor for GenerateDockerfileCommand to store the injected IHost in
a private readonly field (e.g., _host) and include a minimal null-guard (throw
ArgumentNullException if host is null) so the intent is explicit and the object
is retained for potential use in RunAsync; ensure references use the class name
GenerateDockerfileCommand and the RunAsync method remains unchanged except for
accessing the stored _host if needed.
- Around line 51-57: The Dockerfile emits a restore step before copying full
sources which breaks restores for multi-project solutions (missing
Directory.Packages.props/NuGet.config/project refs); update the Dockerfile
generation in GenerateDockerfileCommand (where sb appends lines with projectName
and RUN dotnet restore) so that either the restore step is moved to after the
"COPY . ." and before "RUN dotnet build" or ensure additional files
(Directory.Packages.props, NuGet.config and referenced .csproj files) are copied
prior to the existing RUN dotnet restore; adjust the sb.AppendLine sequence
accordingly so restore runs when all required files are present.
In `@src/K8sOperator.NET/OperatorExtensions.cs`:
- Around line 18-22: Add a null guard at the start of AddOperator to validate
the incoming IServiceCollection parameter and throw ArgumentNullException when
services is null; specifically, inside the AddOperator method (and before
creating OperatorBuilder or invoking configure) check the services parameter and
throw new ArgumentNullException(nameof(services)) so the public API contract is
upheld for the AddOperator method and callers receive a clear exception when a
null services is passed.
♻️ Duplicate comments (5)
src/K8sOperator.NET/Operator.targets (2)
17-20: Preserve OperatorNamespace default when OperatorName is overridden.Currently the default ignores
OperatorNameoverrides. This regresses previous behavior for consumers who customizeOperatorName.🛠️ Proposed fix
- <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace> + <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' AND '$(OperatorName)' != '' ">$(OperatorName)-system</OperatorNamespace> + <OperatorNamespace Condition=" '$(OperatorNamespace)' == '' AND '$(OperatorName)' == '' ">$(MSBuildProjectName.Replace(" ", "_").ToLower())-system</OperatorNamespace>
66-82: Pass configuration/TFM todotnet run --no-build.Without
-c $(Configuration)(and-f $(TargetFramework)), Release builds may run stale Debug outputs. This applies to both launchSettings and Dockerfile generation.🛠️ Proposed fix
- <Exec Command="dotnet run --no-build -- generate-launchsettings" + <Exec Command="dotnet run --no-build -c $(Configuration) -f $(TargetFramework) -- generate-launchsettings" WorkingDirectory="$(MSBuildProjectDirectory)" IgnoreExitCode="false" /> @@ - <Exec Command="dotnet run --no-build -- generate-dockerfile" + <Exec Command="dotnet run --no-build -c $(Configuration) -f $(TargetFramework) -- generate-dockerfile" WorkingDirectory="$(MSBuildProjectDirectory)" IgnoreExitCode="false" />examples/SimpleOperator/Properties/launchSettings.json (1)
51-53: Use$schemafor launchSettings.json schema binding.Tooling won’t recognize
schemaand will skip schema-based validation/IntelliSense.🛠️ Proposed fix
- "schema": "http://json.schemastore.org/launchsettings.json" + "$schema": "http://json.schemastore.org/launchsettings.json"src/K8sOperator.NET/OperatorExtensions.cs (2)
51-56: RegisterIKubernetes(not justKubernetes) so DI can resolve the interface.
EventWatcher<T>expectsIKubernetes, but the current registration only provides the concrete type.🛠️ Proposed fix
- services.TryAddSingleton((sp) => + services.TryAddSingleton<IKubernetes>(sp => { var config = builder?.Configuration ?? KubernetesClientConfiguration.BuildDefaultConfig(); return new Kubernetes(config); });
57-58: EnsureOperatorServiceis registered as a hosted service.
OperatorServicederives fromBackgroundService, but singleton registration alone won’t start it.🛠️ Proposed fix
services.TryAddSingleton<OperatorService>(); + services.TryAddEnumerable( + ServiceDescriptor.Singleton<IHostedService>(sp => sp.GetRequiredService<OperatorService>()) + );
🧹 Nitpick comments (2)
examples/SimpleOperator/Dockerfile (1)
24-28: Consider using--chownflag on COPY to reduce image layers.You can combine the COPY and chown operations into a single layer, which slightly reduces the final image size.
♻️ Suggested optimization
# Copy published app -COPY --from=publish /app/publish . - -# Set ownership -RUN chown -R operator:operator /app +COPY --from=publish --chown=operator:operator /app/publish .examples/SimpleOperator/.dockerignore (1)
32-33: Consider removing redundant Docker file exclusions.Excluding
Dockerfileand.dockerignorein the.dockerignorefile is redundant and potentially confusing:
- The Dockerfile is read directly by the Docker client, not from the build context
- The .dockerignore file itself is never included in the build context
While this doesn't cause functional issues, it's non-standard and may confuse developers reviewing the file.
Suggested cleanup
-# Docker -Dockerfile -.dockerignore -
| # Health check | ||
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD dotnet SimpleOperator.dll version || exit 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find examples/SimpleOperator -type f -name "*.cs" | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 202
🏁 Script executed:
rg -n "version" examples/SimpleOperator/ --type cs -B 2 -A 2Repository: pmdevers/K8sOperator.NET
Length of output: 50
🏁 Script executed:
# Also check if there's a Program.cs or main entry point
find examples/SimpleOperator -name "Program.cs" -o -name "Program.fs" -o -name "Main.cs"Repository: pmdevers/K8sOperator.NET
Length of output: 102
🏁 Script executed:
cat -n examples/SimpleOperator/Program.csRepository: pmdevers/K8sOperator.NET
Length of output: 389
Fix the health check command—the version subcommand does not exist.
The HEALTHCHECK command attempts to run dotnet SimpleOperator.dll version, but the application (Program.cs) is a standard ASP.NET Core web server with no CLI subcommand handling. It runs app.RunOperatorAsync() and expects to accept the operator argument at startup. The health check should instead use an HTTP request to verify the application is responding, e.g., CMD curl -f http://localhost:8080/health || exit 1 (if a health endpoint is implemented), or remove the health check entirely and rely on Kubernetes liveness probes.
🤖 Prompt for AI Agents
In `@examples/SimpleOperator/Dockerfile` around lines 38 - 40, The HEALTHCHECK
currently runs "dotnet SimpleOperator.dll version" which is invalid because
Program.cs runs the ASP.NET Core server via app.RunOperatorAsync() and expects
the "operator" startup argument; update the Dockerfile HEALTHCHECK to verify the
HTTP server instead (e.g., use curl -f against the app's health endpoint like
http://localhost:8080/health) or remove the HEALTHCHECK entirely and rely on
Kubernetes liveness/readiness probes; ensure you modify the HEALTHCHECK
instruction (the HEALTHCHECK line in the Dockerfile) rather than changing
Program.cs.
| public class GenerateDockerfileCommand(IHost host) : IOperatorCommand | ||
| { | ||
| public async Task RunAsync(string[] args) |
There was a problem hiding this comment.
Consume the injected IHost to avoid unused-parameter warnings.
This parameter is required for ActivatorUtilities.CreateInstance but currently unused. A minimal null-guard also makes the intent explicit.
🛠️ Proposed fix
-public class GenerateDockerfileCommand(IHost host) : IOperatorCommand
+public class GenerateDockerfileCommand(IHost host) : IOperatorCommand
{
+ private readonly IHost _host = host ?? throw new ArgumentNullException(nameof(host));
public async Task RunAsync(string[] args)
{📝 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.
| public class GenerateDockerfileCommand(IHost host) : IOperatorCommand | |
| { | |
| public async Task RunAsync(string[] args) | |
| public class GenerateDockerfileCommand(IHost host) : IOperatorCommand | |
| { | |
| private readonly IHost _host = host ?? throw new ArgumentNullException(nameof(host)); | |
| public async Task RunAsync(string[] args) |
🧰 Tools
🪛 GitHub Check: Build
[warning] 10-10:
Parameter 'host' is unread.
[warning] 10-10:
Parameter 'host' is unread.
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/GenerateDockerfileCommand.cs` around lines 10 -
12, GenerateDockerfileCommand currently accepts an IHost host parameter but
never uses it, causing unused-parameter warnings; update the primary constructor
for GenerateDockerfileCommand to store the injected IHost in a private readonly
field (e.g., _host) and include a minimal null-guard (throw
ArgumentNullException if host is null) so the intent is explicit and the object
is retained for potential use in RunAsync; ensure references use the class name
GenerateDockerfileCommand and the RunAsync method remains unchanged except for
accessing the stored _host if needed.
| sb.AppendLine("# Copy project file and restore dependencies"); | ||
| sb.AppendLine($"COPY [\"{projectName}.csproj\", \"./\"]"); | ||
| sb.AppendLine("RUN dotnet restore"); | ||
| sb.AppendLine(); | ||
| sb.AppendLine("# Copy source code and build"); | ||
| sb.AppendLine("COPY . ."); | ||
| sb.AppendLine("RUN dotnet build -c Release -o /app/build"); |
There was a problem hiding this comment.
dotnet restore can fail for project references / central packages.
Only the .csproj is copied before restore, so Directory.Packages.props, NuGet.config, and referenced project files won’t be present. Consider restoring after copying full sources to avoid restore failures for multi-project solutions.
🛠️ Proposed fix (restore after full copy)
- sb.AppendLine($"COPY [\"{projectName}.csproj\", \"./\"]");
- sb.AppendLine("RUN dotnet restore");
- sb.AppendLine();
- sb.AppendLine("# Copy source code and build");
- sb.AppendLine("COPY . .");
- sb.AppendLine("RUN dotnet build -c Release -o /app/build");
+ sb.AppendLine($"COPY [\"{projectName}.csproj\", \"./\"]");
+ sb.AppendLine();
+ sb.AppendLine("# Copy source code and restore/build");
+ sb.AppendLine("COPY . .");
+ sb.AppendLine("RUN dotnet restore");
+ sb.AppendLine("RUN dotnet build -c Release -o /app/build");🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/GenerateDockerfileCommand.cs` around lines 51 -
57, The Dockerfile emits a restore step before copying full sources which breaks
restores for multi-project solutions (missing
Directory.Packages.props/NuGet.config/project refs); update the Dockerfile
generation in GenerateDockerfileCommand (where sb appends lines with projectName
and RUN dotnet restore) so that either the restore step is moved to after the
"COPY . ." and before "RUN dotnet build" or ensure additional files
(Directory.Packages.props, NuGet.config and referenced .csproj files) are copied
prior to the existing RUN dotnet restore; adjust the sb.AppendLine sequence
accordingly so restore runs when all required files are present.
| public IServiceCollection AddOperator(Action<OperatorBuilder>? configure = null) | ||
| { | ||
| var builder = new OperatorBuilder(); | ||
| configure?.Invoke(builder); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/K8sOperator.NET/OperatorExtensions.csRepository: pmdevers/K8sOperator.NET
Length of output: 4316
Add a null guard to match the public API contract.
AddOperator is expected to throw ArgumentNullException for a null collection, but it currently dereferences services directly on line 23.
🛠️ Proposed fix
public IServiceCollection AddOperator(Action<OperatorBuilder>? configure = null)
{
+ ArgumentNullException.ThrowIfNull(services);
var builder = new OperatorBuilder();
configure?.Invoke(builder);📝 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.
| public IServiceCollection AddOperator(Action<OperatorBuilder>? configure = null) | |
| { | |
| var builder = new OperatorBuilder(); | |
| configure?.Invoke(builder); | |
| public IServiceCollection AddOperator(Action<OperatorBuilder>? configure = null) | |
| { | |
| ArgumentNullException.ThrowIfNull(services); | |
| var builder = new OperatorBuilder(); | |
| configure?.Invoke(builder); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorExtensions.cs` around lines 18 - 22, Add a null
guard at the start of AddOperator to validate the incoming IServiceCollection
parameter and throw ArgumentNullException when services is null; specifically,
inside the AddOperator method (and before creating OperatorBuilder or invoking
configure) check the services parameter and throw new
ArgumentNullException(nameof(services)) so the public API contract is upheld for
the AddOperator method and callers receive a clear exception when a null
services is passed.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.