Adds additional printer columns to CRD #88
Conversation
📝 WalkthroughWalkthroughConsolidates builder APIs into generic object builders, replaces many concrete builders with ObjectBuilder/extensions, adds lease-based leader election and CLI Create command, embeds Docker/.dockerignore templates and generation changes, updates operator DI/wiring to support KubeConfig/leader-election, and expands README, examples, and launch/docker generation workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant CreateCmd as CreateCommand
participant Watchers as EventWatcherDatasource
participant YAML as KubernetesYaml
User->>CreateCmd: RunAsync(args)
CreateCmd->>Watchers: GetWatchers()
Watchers-->>CreateCmd: list of watcher metadata
CreateCmd->>CreateCmd: Find watcher by resource name
alt found
CreateCmd->>CreateCmd: Instantiate resource, Initialize()
CreateCmd->>YAML: KubernetesYaml.Serialize(resource)
YAML-->>User: Output YAML
else not found
CreateCmd-->>User: "Unknown resource" message
end
sequenceDiagram
participant Host as Host
participant LeaderSvc as LeaderElectionService
participant K8s as Kubernetes API
participant Operator as OperatorService
participant Watchers as Event Watchers
Host->>LeaderSvc: StartAsync(cancellation)
loop acquire leadership
LeaderSvc->>K8s: TryAcquireLease
K8s-->>LeaderSvc: Lease acquired / conflict
alt acquired
LeaderSvc->>Operator: Notify leadership acquired
Operator->>Watchers: StartWatchers()
Watchers->>K8s: Watch resources
loop renew loop
LeaderSvc->>K8s: RenewLease
K8s-->>LeaderSvc: Renewed / conflict
end
else conflict / lost
LeaderSvc->>Operator: Notify leadership lost
Operator->>Watchers: Cancel watchers
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
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/KubernetesObjectBuilderWithMetadataExtentions.cs (2)
19-24: Missingthiskeyword breaks extension method.The method is no longer an extension method because
thisis missing before the first parameter. This will cause compilation errors when trying to use it asbuilder.WithName(...).🐛 Proposed fix
- public static IObjectBuilder<T> WithName<T>(IObjectBuilder<T> builder, string name) + public static IObjectBuilder<T> WithName<T>(this IObjectBuilder<T> builder, string name) where T : IKubernetesObject<V1ObjectMeta> { builder.Add(x => x.Metadata.Name = name); return builder; }
14-15: XML documentation references non-existentTBuildertype parameter.The
<typeparam name="TBuilder">doc is stale since the method signature no longer has aTBuildergeneric parameter.📝 Suggested fix
/// <summary> /// Sets the name of the Kubernetes object. /// </summary> - /// <typeparam name="TBuilder">The type of the builder.</typeparam> /// <typeparam name="T">The type of the Kubernetes object.</typeparam> /// <param name="builder">The builder instance.</param> /// <param name="name">The name to assign to the Kubernetes object.</param> /// <returns>The configured builder.</returns>src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
274-291: Fix boolean and datetime schema type mappings in OpenAPI schema generation.Boolean fields must be typed as
"boolean"(not"string") to pass CRD validation. DateTime fields need the"date-time"format added alongside the"string"type per OpenAPI/Kubernetes schema specification. The current mappings will cause validation failures for boolean values and lose semantic type information for timestamps.Proposed fix
if (type.FullName == "System.Boolean") { builder.Add(x => { - x.Type = "string"; + x.Type = "boolean"; x.Nullable = nullable; }); return builder; } if (type.FullName == "System.DateTime") { builder.Add(x => { x.Type = "string"; + x.Format = "date-time"; x.Nullable = nullable; }); return builder; }
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/Commands/CreateCommand.cs`:
- Around line 32-34: The code calls
Activator.CreateInstance(watcher.Controller.ResourceType) and immediately uses
activator (CustomResource) without null-check, which can throw a
NullReferenceException if creation fails or the type isn't a CustomResource;
update the CreateCommand logic to validate the result: check that activator is
not null and is a CustomResource before calling activator.Initialize() and
KubernetesYaml.Serialize, and if the check fails either throw a descriptive
exception or log an error indicating watcher.Controller.ResourceType could not
be instantiated or does not derive from CustomResource so callers can diagnose
the missing parameterless constructor or wrong type.
In `@src/K8sOperator.NET/Commands/InstallCommand.cs`:
- Around line 166-173: The ClusterRoleBinding created by
KubernetesObjectBuilder.Create<V1ClusterRoleBinding>() hardcodes the
ServiceAccount namespace to "system" in the WithSubject call, causing RBAC to be
applied to the wrong namespace when a custom NamespaceAttribute is configured;
update the code to read the namespace from
metadata.OfType<NamespaceAttribute>().FirstOrDefault() (or
NamespaceAttribute.Default if missing) and pass that namespace into
WithSubject(ns: <configuredNamespace>) so the ServiceAccount binding targets the
operator's configured namespace instead of "system".
- Around line 48-57: The CreateNamespace method (and other builders in this
file) uses KubernetesObjectBuilder.Create<V1Namespace>() then calls
nsBuilder.WithName(...), which can NRE when Metadata is null in KubernetesClient
v18.0.13; fix by using the metadata-initializing builder or initializing
Metadata before mutations—replace KubernetesObjectBuilder.Create<V1Namespace>()
with KubernetesObjectBuilder.CreateMeta<V1Namespace>() (or call .Add(x =>
x.Metadata = new()) on the builder) so that nsBuilder.WithName(...) operates on
a non-null Metadata; apply the same change to other places where Create<T>() is
followed by WithName/WithNamespace/WithLabel.
In `@src/K8sOperator.NET/Commands/VersionCommand.cs`:
- Around line 16-28: The output uses the attribute instances themselves which
call DebuggerHelpers.GetDebugText(); update the user-facing writes to use
explicit properties: replace the interpolated {name} with name.OperatorName and
replace {version} with version.Tag (while keeping version.GetImage() for Docker
Info) so Console.WriteLine prints the actual operator name and version tag;
locate the logic around watcher.Metadata where OperatorNameAttribute and
DockerImageAttribute are selected and change the two Console.WriteLine calls
accordingly.
In `@src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs`:
- Around line 301-305: The RunAsRoot extension on
IObjectBuilder<V1SecurityContext> is assigning the wrong value to RunAsNonRoot;
change the lambda in RunAsRoot (method RunAsRoot<TBuilder>) so it sets
x.RunAsNonRoot = !runAsRoot (i.e., negate the runAsRoot parameter) instead of
x.RunAsNonRoot = runAsRoot to align the property with the method name and
parameter semantics.
- Around line 212-216: The AddEnvFromSecretKey method currently accepts a
V1ConfigMapKeySelector and therefore builds a ConfigMapKeyRef; change its
parameter type to V1SecretKeySelector and adjust any lambda/action usage so the
call to AddEnv uses the V1SecretKeySelector action so the resulting env var uses
a SecretKeyRef; specifically update the signature of AddEnvFromSecretKey (and
its action parameter type) to V1SecretKeySelector and ensure callers and the
inner AddEnv invocation work with V1SecretKeySelector to produce SecretKeyRef.
In `@src/K8sOperator.NET/Generation/LeaseBuilderExtensions.cs`:
- Around line 9-18: The XML docs for WithSpecs are incorrect: they reference a
holder identity parameter that doesn't exist and misdescribe the method; update
the summary to say WithSpecs creates and returns an IObjectBuilder<V1LeaseSpec>
to configure the Lease's Spec, remove the <param> tag (since there are no
parameters), and ensure the <returns> describes the returned spec builder
(matching the method WithSpecs and the ObjectBuilder<V1LeaseSpec> / specBuilder
usage).
In `@src/K8sOperator.NET/Metadata/AdditionalPrinterColumn.cs`:
- Around line 3-10: Make Name, Type, and Path required via a primary constructor
on AdditionalPrinterColumnAttribute so callers must supply them and you avoid
nulls at runtime; keep Description nullable and Priority optional with a default
(e.g., 0). Update the attribute declaration (AdditionalPrinterColumnAttribute)
to accept (string name, string type, string path) in the constructor and expose
those values via the Name, Type, and Path properties, leaving Description as
string? and Priority as int with a default value.
In `@src/K8sOperator.NET/Operator.targets`:
- Around line 48-62: The repository suffix logic is causing OperatorName to be
duplicated because ContainerRepository may already include the operator; update
the property construction so _ContainerRepositorySuffix represents only the base
repository (either "$(Company)/" when ContainerRepository is empty or
"$(ContainerRepository)/" when set) but do NOT unconditionally append
OperatorName into _FullContainerRepository; instead set _FullContainerRepository
to $(_ContainerRepositorySuffix)$(OperatorName) only when ContainerRepository
was empty, and use $(_ContainerRepositorySuffix) (or the provided
ContainerRepository value without adding OperatorName) when ContainerRepository
is non-empty, adjusting the conditions around _ContainerRepositorySuffix and
_FullContainerRepository to avoid double slashes and duplicate OperatorName.
In `@src/K8sOperator.NET/OperatorExtensions.cs`:
- Around line 118-130: The static attribute fields Namespace, Docker and
Operator should be made readonly and obtain attributes from
Assembly.GetEntryAssembly() with a fallback to Assembly.GetExecutingAssembly()
to ensure app-level attributes are used (GetEntryAssembly can be null in some
hosts); update the declarations for Namespace, Docker and Operator to be public
static readonly and replace Assembly.GetExecutingAssembly() calls with a pattern
like Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly() so existing
uses (e.g., the LeaderElection object that references Operator.OperatorName and
Namespace.Namespace) continue to work and CA2211 is addressed.
In `@src/K8sOperator.NET/OperatorService.cs`:
- Line 61: There is a race on the class field _runningTasks being cleared in
ExecuteAsync while StartWatchers populates it and Task.WhenAll(_runningTasks)
awaits it; change StartWatchers to create a local List<Task> (instead of writing
to the class field _runningTasks), have StartWatchers return a single aggregated
Task (e.g., Task returnedFromStartWatchers = Task.WhenAll(localList)) and await
that returned Task in ExecuteAsync (remove/stop using _runningTasks.Clear());
update any callers of StartWatchers to use the returned Task and remove
modifications to the _runningTasks field to eliminate the race.
In `@src/K8sOperator.NET/Templates/.dockerignore.template`:
- Around line 35-38: The .dockerignore.template currently excludes all YAML
files via the patterns "*.yaml" and "*.yml", which can unintentionally omit
important files (e.g., appsettings or deployment configs); update the template
by removing these blanket patterns and instead either narrow them to a specific
manifests directory (e.g., use "manifests/*.yaml" and "manifests/*.yml") or add
explicit allow rules for required files (e.g., "!appsettings*.yaml") so only
intended YAMLs are excluded.
In `@test/K8sOperator.NET.Target.Tests/K8sOperator.NET.Target.Tests.csproj`:
- Around line 6-7: The XML comment above the CreateOperatorLaunchSettings
property is inaccurate; update or remove it so it matches the property's intent:
either change the comment to "Disable launch settings creation for testing" (or
similar) when CreateOperatorLaunchSettings is false, or delete the comment
entirely; locate the CreateOperatorLaunchSettings element in the project file
and make the comment and its wording consistent with the property's value.
🧹 Nitpick comments (10)
README.md (1)
441-444: Use a heading instead of emphasis for the footer line.This triggers MD036 and is easy to align with markdownlint.
📝 Suggested tweak
-**Built with ❤️ using .NET 10** +### Built with ❤️ using .NET 10test/K8sOperator.NET.Tests/OperatorExtensions_Tests.cs (1)
118-118: Consider documenting the expected commands.The magic number
7is brittle—if default commands change, this test silently becomes incorrect. Consider adding a comment listing the expected commands, or asserting on specific command names/types for better maintainability.examples/SimpleOperator/Program.cs (1)
7-11: Avoid shadowing the outer variablexin nested lambda.The inner lambda parameter
xshadows the outerx, reducing readability. Use a distinct name for clarity.✨ Suggested fix
builder.Services.AddOperator(x => { - //x.LeaderElection.Add(x => x.Enabled = true); - x.LeaderElection.Add(x => x.LeaseNamespace = "default"); + //x.LeaderElection.Add(options => options.Enabled = true); + x.LeaderElection.Add(options => options.LeaseNamespace = "default"); });src/K8sOperator.NET/OperatorService.cs (2)
27-30: Busy-wait polling is inefficient; consider event-driven approach.Polling
IsLeaderevery 100ms consumes CPU cycles unnecessarily. IfLeaderElectionServiceexposed aTaskor event (e.g.,WaitForLeadershipAsync()), the service could await leadership changes directly instead of spinning.
78-78: Redundant linked CancellationTokenSource.
StartWatchersalready receivesstoppingTokenwhich is itself a linked token from line 38. Creating another linked CTS here is unnecessary nesting. Simply use the incomingstoppingTokendirectly.♻️ Suggested simplification
public async Task StartWatchers(CancellationToken stoppingToken) { var watchers = Datasource.GetWatchers().ToList(); var logger = ServiceProvider.GetRequiredService<ILogger<OperatorService>>(); if (!watchers.Any()) { logger.LogInformation("No event watchers registered."); return; } - using var cts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken); foreach (var watcher in watchers) { var task = Task.Run(async () => { try { - await watcher.Start(cts.Token); + await watcher.Start(stoppingToken); } catch (OperatorException) { - await cts.CancelAsync(); + // Consider propagating cancellation via a shared CTS if needed throw; } } - , cts.Token); + , stoppingToken); _runningTasks.Add(task); } await Task.WhenAll(_runningTasks); }src/K8sOperator.NET/Commands/CreateCommand.cs (1)
5-7: Unused imports.
System.Collections.GenericandSystem.Textappear unused in this file.src/K8sOperator.NET/EventWatcher.cs (1)
255-258: Duplicate attribute retrieval logic.The same pattern for fetching
NamespaceAttributeandScopeAttributewith defaults is repeated in bothResourceReplaceAsyncandGetWatchStream. Consider extracting these to lazy-initialized properties similar toFinalizerandLabelSelector.♻️ Suggested refactor
+ private NamespaceAttribute Namespace => Metadata.OfType<NamespaceAttribute>().FirstOrDefault() + ?? NamespaceAttribute.Default; + + private ScopeAttribute Scope => Metadata.OfType<ScopeAttribute>().FirstOrDefault() + ?? ScopeAttribute.Default; + private Task<T> ResourceReplaceAsync(T resource, CancellationToken cancellationToken) { - var ns = metadata.OfType<NamespaceAttribute>().FirstOrDefault() ?? - NamespaceAttribute.Default; - var scope = metadata.OfType<ScopeAttribute>().FirstOrDefault() ?? - ScopeAttribute.Default; - - return scope.Scope switch + return Scope.Scope switch { EntityScope.Cluster => kubernetes.CustomObjects.ReplaceClusterCustomObjectAsync<T>( ... - EntityScope.Namespaced => kubernetes.CustomObjects.ReplaceNamespacedCustomObjectAsync<T>( + EntityScope.Namespaced => kubernetes.CustomObjects.ReplaceNamespacedCustomObjectAsync<T>( ... - namespaceParameter: ns.Namespace, + namespaceParameter: Namespace.Namespace, ...Also applies to: 285-288
src/K8sOperator.NET/LeaderLectionService.cs (1)
45-63: Consider surfacing renewal failures for observability.The renewal loop suppresses all exceptions; adding logging (or telemetry hooks) would make leadership loss diagnosable.
src/K8sOperator.NET/Generation/KubernetesObjectBuilder.cs (1)
64-68: Potential typo inCreatMeta.If this is a new public API, consider
CreateMetafor consistency withCreate<T>().src/K8sOperator.NET/Generation/MetadataExtensions.cs (1)
75-108: Clarify RemoveMetadata semantics for multi‑instance attributes.
RemoveMetadataremoves all entries of the same type. If attributes likeAdditionalPrinterColumnAttributeare intended to be multi‑instance, consider documenting this behavior or adding a “remove by instance” overload.
| var activator = Activator.CreateInstance(watcher.Controller.ResourceType) as CustomResource; | ||
| activator.Initialize(); | ||
| Console.WriteLine(KubernetesYaml.Serialize(activator)); |
There was a problem hiding this comment.
Null dereference risk when Activator.CreateInstance fails.
If the resource type lacks a parameterless constructor or doesn't derive from CustomResource, activator will be null, causing a NullReferenceException on line 33.
🐛 Proposed fix
- var activator = Activator.CreateInstance(watcher.Controller.ResourceType) as CustomResource;
- activator.Initialize();
- Console.WriteLine(KubernetesYaml.Serialize(activator));
+ var resource = Activator.CreateInstance(watcher.Controller.ResourceType) as CustomResource;
+ if (resource == null)
+ {
+ Console.WriteLine($"Failed to create instance of {watcher.Controller.ResourceType.Name}. Ensure it has a parameterless constructor and derives from CustomResource.");
+ return Task.CompletedTask;
+ }
+ resource.Initialize();
+ Console.WriteLine(KubernetesYaml.Serialize(resource));📝 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 activator = Activator.CreateInstance(watcher.Controller.ResourceType) as CustomResource; | |
| activator.Initialize(); | |
| Console.WriteLine(KubernetesYaml.Serialize(activator)); | |
| var resource = Activator.CreateInstance(watcher.Controller.ResourceType) as CustomResource; | |
| if (resource == null) | |
| { | |
| Console.WriteLine($"Failed to create instance of {watcher.Controller.ResourceType.Name}. Ensure it has a parameterless constructor and derives from CustomResource."); | |
| return Task.CompletedTask; | |
| } | |
| resource.Initialize(); | |
| Console.WriteLine(KubernetesYaml.Serialize(resource)); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/CreateCommand.cs` around lines 32 - 34, The code
calls Activator.CreateInstance(watcher.Controller.ResourceType) and immediately
uses activator (CustomResource) without null-check, which can throw a
NullReferenceException if creation fails or the type isn't a CustomResource;
update the CreateCommand logic to validate the result: check that activator is
not null and is a CustomResource before calling activator.Initialize() and
KubernetesYaml.Serialize, and if the check fails either throw a descriptive
exception or log an error indicating watcher.Controller.ResourceType could not
be instantiated or does not derive from CustomResource so callers can diagnose
the missing parameterless constructor or wrong type.
| var name = watcher.Metadata.OfType<OperatorNameAttribute>().FirstOrDefault() | ||
| ?? OperatorNameAttribute.Default; | ||
| var version = watcher.Metadata.OfType<DockerImageAttribute>().FirstOrDefault() | ||
| ?? DockerImageAttribute.Default; | ||
|
|
||
| if (string.IsNullOrWhiteSpace(name) || string.IsNullOrWhiteSpace(version)) | ||
| if (string.IsNullOrWhiteSpace(name.OperatorName) || string.IsNullOrWhiteSpace(version.Tag)) | ||
| { | ||
| Console.WriteLine("Operator name or version metadata is missing."); | ||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| Console.WriteLine($"{name} version {version}."); | ||
| Console.WriteLine($"Docker Info: {watcher.Metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())}."); | ||
| Console.WriteLine($"Docker Info: {version.GetImage()}."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and examine OperatorNameAttribute
file=$(fd --type f 'OperatorNameAttribute.cs' | head -n 1)
echo "=== OperatorNameAttribute.cs ==="
if [ -n "$file" ]; then
sed -n '1,200p' "$file"
else
echo "File not found"
fiRepository: pmdevers/K8sOperator.NET
Length of output: 504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and examine DockerImageAttribute
file=$(fd --type f 'DockerImageAttribute.cs' | head -n 1)
echo "=== DockerImageAttribute.cs ==="
if [ -n "$file" ]; then
sed -n '1,200p' "$file"
else
echo "File not found"
fiRepository: pmdevers/K8sOperator.NET
Length of output: 735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and examine DebuggerHelpers
file=$(fd --type f 'DebuggerHelpers.cs' | head -n 1)
echo "=== DebuggerHelpers.cs ==="
if [ -n "$file" ]; then
cat -n "$file"
else
echo "File not found"
fiRepository: pmdevers/K8sOperator.NET
Length of output: 327
Use explicit properties for user-facing output.
While OperatorNameAttribute and DockerImageAttribute both override ToString() (via DebuggerHelpers.GetDebugText()), relying on them produces debugger-style output (e.g., 'OperatorName' - my-operator). For consistency with line 28 which explicitly calls version.GetImage(), use name.OperatorName instead of {name}.
🛠️ Proposed fix
- Console.WriteLine($"{name} version {version}.");
+ Console.WriteLine($"{name.OperatorName} version {version.Tag}.");📝 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 = watcher.Metadata.OfType<OperatorNameAttribute>().FirstOrDefault() | |
| ?? OperatorNameAttribute.Default; | |
| var version = watcher.Metadata.OfType<DockerImageAttribute>().FirstOrDefault() | |
| ?? DockerImageAttribute.Default; | |
| if (string.IsNullOrWhiteSpace(name) || string.IsNullOrWhiteSpace(version)) | |
| if (string.IsNullOrWhiteSpace(name.OperatorName) || string.IsNullOrWhiteSpace(version.Tag)) | |
| { | |
| Console.WriteLine("Operator name or version metadata is missing."); | |
| return Task.CompletedTask; | |
| } | |
| Console.WriteLine($"{name} version {version}."); | |
| Console.WriteLine($"Docker Info: {watcher.Metadata.TryGetValue<DockerImageAttribute, string>(x => x.GetImage())}."); | |
| Console.WriteLine($"Docker Info: {version.GetImage()}."); | |
| var name = watcher.Metadata.OfType<OperatorNameAttribute>().FirstOrDefault() | |
| ?? OperatorNameAttribute.Default; | |
| var version = watcher.Metadata.OfType<DockerImageAttribute>().FirstOrDefault() | |
| ?? DockerImageAttribute.Default; | |
| if (string.IsNullOrWhiteSpace(name.OperatorName) || string.IsNullOrWhiteSpace(version.Tag)) | |
| { | |
| Console.WriteLine("Operator name or version metadata is missing."); | |
| return Task.CompletedTask; | |
| } | |
| Console.WriteLine($"{name.OperatorName} version {version.Tag}."); | |
| Console.WriteLine($"Docker Info: {version.GetImage()}."); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/VersionCommand.cs` around lines 16 - 28, The
output uses the attribute instances themselves which call
DebuggerHelpers.GetDebugText(); update the user-facing writes to use explicit
properties: replace the interpolated {name} with name.OperatorName and replace
{version} with version.Tag (while keeping version.GetImage() for Docker Info) so
Console.WriteLine prints the actual operator name and version tag; locate the
logic around watcher.Metadata where OperatorNameAttribute and
DockerImageAttribute are selected and change the two Console.WriteLine calls
accordingly.
| public static TBuilder AddEnvFromSecretKey<TBuilder>(this TBuilder builder, string name, Action<V1ConfigMapKeySelector> action) | ||
| where TBuilder : IKubernetesObjectBuilder<V1Container> | ||
| where TBuilder : IObjectBuilder<V1Container> | ||
| { | ||
| return builder.AddEnv(name, action); | ||
| } |
There was a problem hiding this comment.
Use V1SecretKeySelector for secret-sourced env vars.
AddEnvFromSecretKey currently takes V1ConfigMapKeySelector, so it builds ConfigMapKeyRef instead of SecretKeyRef. That breaks secret-based env vars.
🔧 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)📝 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 AddEnvFromSecretKey<TBuilder>(this TBuilder builder, string name, Action<V1ConfigMapKeySelector> action) | |
| where TBuilder : IKubernetesObjectBuilder<V1Container> | |
| where TBuilder : IObjectBuilder<V1Container> | |
| { | |
| return builder.AddEnv(name, action); | |
| } | |
| public static TBuilder AddEnvFromSecretKey<TBuilder>(this TBuilder builder, string name, Action<V1SecretKeySelector> action) | |
| where TBuilder : IObjectBuilder<V1Container> | |
| { | |
| return builder.AddEnv(name, action); | |
| } |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs` around lines
212 - 216, The AddEnvFromSecretKey method currently accepts a
V1ConfigMapKeySelector and therefore builds a ConfigMapKeyRef; change its
parameter type to V1SecretKeySelector and adjust any lambda/action usage so the
call to AddEnv uses the V1SecretKeySelector action so the resulting env var uses
a SecretKeyRef; specifically update the signature of AddEnvFromSecretKey (and
its action parameter type) to V1SecretKeySelector and ensure callers and the
inner AddEnv invocation work with V1SecretKeySelector to produce SecretKeyRef.
| <_ContainerRepositorySuffix Condition=" '$(ContainerRepository)' == '' ">$(Company)/</_ContainerRepositorySuffix> | ||
| <_ContainerRepositorySuffix Condition=" '$(ContainerRepository)' != '' ">$(ContainerRepository)/</_ContainerRepositorySuffix> | ||
|
|
||
| <!-- Add hyphen before ContainerFamily if it's not empty --> | ||
| <_ContainerFamilySuffix Condition=" '$(ContainerFamily)' != '' ">-$(ContainerFamily)</_ContainerFamilySuffix> | ||
| <_ContainerFamilySuffix Condition=" '$(ContainerFamily)' == '' "></_ContainerFamilySuffix> | ||
|
|
||
| <_FullContainerTag>$(_ContainerImageTagValue)$(_ContainerFamilySuffix)</_FullContainerTag> | ||
| <_FullContainerRepository>$(_ContainerRepositorySuffix)$(OperatorName)</_FullContainerRepository> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <AssemblyAttribute Include="K8sOperator.NET.Metadata.DockerImageAttribute"> | ||
| <_Parameter1>$(ContainerRegistry)</_Parameter1> | ||
| <_Parameter2>$(ContainerRepository)</_Parameter2> | ||
| <_Parameter2>$(_FullContainerRepository)</_Parameter2> | ||
| <_Parameter3>$(_FullContainerTag)</_Parameter3> |
There was a problem hiding this comment.
Avoid duplicating OperatorName in the repository path.
With the default ContainerRepository = $(Company)/$(OperatorName), the new suffix logic produces {Company}/{OperatorName}/{OperatorName} (and potentially double slashes). This breaks the DockerImage repository and the generated tag.
🛠️ Suggested fix
- <_ContainerRepositorySuffix Condition=" '$(ContainerRepository)' == '' ">$(Company)/</_ContainerRepositorySuffix>
- <_ContainerRepositorySuffix Condition=" '$(ContainerRepository)' != '' ">$(ContainerRepository)/</_ContainerRepositorySuffix>
-
<!-- Add hyphen before ContainerFamily if it's not empty -->
<_ContainerFamilySuffix Condition=" '$(ContainerFamily)' != '' ">-$(ContainerFamily)</_ContainerFamilySuffix>
<_ContainerFamilySuffix Condition=" '$(ContainerFamily)' == '' "></_ContainerFamilySuffix>
<_FullContainerTag>$(_ContainerImageTagValue)$(_ContainerFamilySuffix)</_FullContainerTag>
- <_FullContainerRepository>$(_ContainerRepositorySuffix)$(OperatorName)</_FullContainerRepository>
+ <_FullContainerRepository Condition=" '$(ContainerRepository)' == '' ">$(Company)/$(OperatorName)</_FullContainerRepository>
+ <_FullContainerRepository Condition=" '$(ContainerRepository)' != '' ">$(ContainerRepository)</_FullContainerRepository>🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Operator.targets` around lines 48 - 62, The repository
suffix logic is causing OperatorName to be duplicated because
ContainerRepository may already include the operator; update the property
construction so _ContainerRepositorySuffix represents only the base repository
(either "$(Company)/" when ContainerRepository is empty or
"$(ContainerRepository)/" when set) but do NOT unconditionally append
OperatorName into _FullContainerRepository; instead set _FullContainerRepository
to $(_ContainerRepositorySuffix)$(OperatorName) only when ContainerRepository
was empty, and use $(_ContainerRepositorySuffix) (or the provided
ContainerRepository value without adding OperatorName) when ContainerRepository
is non-empty, adjusting the conditions around _ContainerRepositorySuffix and
_FullContainerRepository to avoid double slashes and duplicate OperatorName.
| public static NamespaceAttribute Namespace = Assembly.GetExecutingAssembly().GetCustomAttribute<NamespaceAttribute>() ?? | ||
| NamespaceAttribute.Default; | ||
| public static DockerImageAttribute Docker = Assembly.GetExecutingAssembly().GetCustomAttribute<DockerImageAttribute>() ?? | ||
| DockerImageAttribute.Default; | ||
| public static OperatorNameAttribute Operator = Assembly.GetExecutingAssembly().GetCustomAttribute<OperatorNameAttribute>() ?? | ||
| OperatorNameAttribute.Default; | ||
|
|
||
| public KubernetesClientConfiguration? KubeConfig { get; set; } | ||
| public IObjectBuilder<LeaderElectionOptions> LeaderElection { get; } = new ObjectBuilder<LeaderElectionOptions>().Add(x => | ||
| { | ||
| x.LeaseName = $"{Operator.OperatorName}-leader-election"; | ||
| x.LeaseNamespace = Namespace.Namespace; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/K8sOperator.NET/OperatorExtensions.cs | sed -n '110,135p'Repository: pmdevers/K8sOperator.NET
Length of output: 192
🌐 Web query:
Assembly.GetExecutingAssembly() vs Assembly.GetEntryAssembly() .NET difference entry assembly attributes
💡 Result:
Short summary
- Assembly.GetExecutingAssembly() returns the Assembly that contains the code that is currently executing (the assembly where the method call lives). (dotnetframework.org)
- Assembly.GetEntryAssembly() returns the process entry assembly (the assembly whose Main was the process entry point). It can return null when there is no managed entry (e.g., ASP.NET, unit test runners, native hosts, COM callers or other unmanaged entry points). (stackoverflow.com)
Practical implications for reading “entry assembly” attributes
- If you want attributes that belong to the process EXE (the app that started the process) use GetEntryAssembly(), but guard for null because many hosts don’t provide a managed entry assembly. Example usage in framework code checks GetEntryAssembly() for null before reading attributes. (dotnetframework.org)
- If you need the assembly that contains the running code (for resources, embedded files, or attributes on that library), use GetExecutingAssembly() or typeof(SomeType).Assembly — these will point to the assembly containing the code and won’t be null. (dotnetframework.org)
- Alternatives when GetEntryAssembly() is null: use Assembly.GetCallingAssembly(), Assembly.GetExecutingAssembly(), or explicitly pick the known assembly (typeof(MyStartupType).Assembly) depending on which assembly’s attributes you actually need. Many answers recommend fallback logic like GetEntryAssembly() ?? GetCallingAssembly(). (stackoverflow.com)
If you want, tell me which host (console, ASP.NET, test runner, native COM, etc.) and which attribute you need and I’ll give the exact code pattern to use.
Citations:
- 1: https://www.dotnetframework.org/default.aspx/DotNET/DotNET/8%400/untmp/whidbey/REDBITS/ndp/fx/src/CompMod/Microsoft/Win32/SystemEvents%40cs/1/SystemEvents%40cs?utm_source=openai
- 2: https://stackoverflow.com/questions/40217214/why-getentryassembly-returns-null?utm_source=openai
- 3: https://www.dotnetframework.org/default.aspx/DotNET/DotNET/8%400/untmp/whidbey/REDBITS/ndp/fx/src/CompMod/Microsoft/Win32/SystemEvents%40cs/1/SystemEvents%40cs?utm_source=openai
- 4: https://www.dotnetframework.org/default.aspx/DotNET/DotNET/8%400/untmp/whidbey/REDBITS/ndp/fx/src/CompMod/Microsoft/Win32/SystemEvents%40cs/1/SystemEvents%40cs?utm_source=openai
- 5: https://stackoverflow.com/questions/40217214/why-getentryassembly-returns-null?utm_source=openai
🏁 Script executed:
find . -name "*.cs" -type f | xargs grep -l "NamespaceAttribute\|DockerImageAttribute\|OperatorNameAttribute" | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 644
🏁 Script executed:
cat -n src/K8sOperator.NET/Metadata/NamespaceAttribute.csRepository: pmdevers/K8sOperator.NET
Length of output: 601
🏁 Script executed:
cat -n src/K8sOperator.NET/Metadata/DockerImageAttribute.csRepository: pmdevers/K8sOperator.NET
Length of output: 822
🏁 Script executed:
cat -n src/K8sOperator.NET/Metadata/OperatorNameAttribute.csRepository: pmdevers/K8sOperator.NET
Length of output: 576
🏁 Script executed:
grep -n "GetExecutingAssembly\|GetEntryAssembly" src/K8sOperator.NET/*.csRepository: pmdevers/K8sOperator.NET
Length of output: 1030
🌐 Web query:
CA2211 .NET code analysis rule public static field readonly warning
💡 Result:
CA2211 ("Non-constant fields should not be visible") warns when a publicly (or protected/protected‑internal) visible field is non-constant and mutable—in practice it flags public/protected static fields that are not const or readonly because mutable static fields are not thread‑safe and expose risky global mutable state. Fixes:
- Make the field const or readonly if its value is fixed.
- Make the field non‑public (private/internal) and expose it via a thread‑safe property or method.
- If the field must remain mutable and public (rare for libraries), you can suppress the rule (pragma or .editorconfig) but do so only for applications where you control all callers.
Related rule: CA1051 covers visible instance fields (prefer properties over public instance fields). Examples and guidance: Microsoft docs. [1][2]
Sources:
[1] Microsoft — CA2211: Non-constant fields should not be visible.
[2] Microsoft — CA1051: Do not declare visible instance fields.
Make static attribute fields readonly and use GetEntryAssembly() to read app-level attributes.
These static fields use Assembly.GetExecutingAssembly(), which returns the library assembly, not the application assembly where these attributes are defined (AttributeTargets.Assembly). This causes them to skip app-level attribute values. Additionally, public mutable static fields trigger CA2211 warnings and allow runtime mutation. The fix should use GetEntryAssembly() with a fallback (since it can return null in some hosting scenarios) and mark the fields as readonly. Note that this file already uses GetEntryAssembly() correctly at lines 45, 48, and 51 for the same attributes.
Proposed fix
- public static NamespaceAttribute Namespace = Assembly.GetExecutingAssembly().GetCustomAttribute<NamespaceAttribute>() ??
- NamespaceAttribute.Default;
- public static DockerImageAttribute Docker = Assembly.GetExecutingAssembly().GetCustomAttribute<DockerImageAttribute>() ??
- DockerImageAttribute.Default;
- public static OperatorNameAttribute Operator = Assembly.GetExecutingAssembly().GetCustomAttribute<OperatorNameAttribute>() ??
- OperatorNameAttribute.Default;
+ public static readonly NamespaceAttribute Namespace =
+ (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly())
+ .GetCustomAttribute<NamespaceAttribute>() ?? NamespaceAttribute.Default;
+ public static readonly DockerImageAttribute Docker =
+ (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly())
+ .GetCustomAttribute<DockerImageAttribute>() ?? DockerImageAttribute.Default;
+ public static readonly OperatorNameAttribute Operator =
+ (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly())
+ .GetCustomAttribute<OperatorNameAttribute>() ?? OperatorNameAttribute.Default;📝 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 NamespaceAttribute Namespace = Assembly.GetExecutingAssembly().GetCustomAttribute<NamespaceAttribute>() ?? | |
| NamespaceAttribute.Default; | |
| public static DockerImageAttribute Docker = Assembly.GetExecutingAssembly().GetCustomAttribute<DockerImageAttribute>() ?? | |
| DockerImageAttribute.Default; | |
| public static OperatorNameAttribute Operator = Assembly.GetExecutingAssembly().GetCustomAttribute<OperatorNameAttribute>() ?? | |
| OperatorNameAttribute.Default; | |
| public KubernetesClientConfiguration? KubeConfig { get; set; } | |
| public IObjectBuilder<LeaderElectionOptions> LeaderElection { get; } = new ObjectBuilder<LeaderElectionOptions>().Add(x => | |
| { | |
| x.LeaseName = $"{Operator.OperatorName}-leader-election"; | |
| x.LeaseNamespace = Namespace.Namespace; | |
| }); | |
| public static readonly NamespaceAttribute Namespace = | |
| (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly()) | |
| .GetCustomAttribute<NamespaceAttribute>() ?? NamespaceAttribute.Default; | |
| public static readonly DockerImageAttribute Docker = | |
| (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly()) | |
| .GetCustomAttribute<DockerImageAttribute>() ?? DockerImageAttribute.Default; | |
| public static readonly OperatorNameAttribute Operator = | |
| (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly()) | |
| .GetCustomAttribute<OperatorNameAttribute>() ?? OperatorNameAttribute.Default; | |
| public KubernetesClientConfiguration? KubeConfig { get; set; } | |
| public IObjectBuilder<LeaderElectionOptions> LeaderElection { get; } = new ObjectBuilder<LeaderElectionOptions>().Add(x => | |
| { | |
| x.LeaseName = $"{Operator.OperatorName}-leader-election"; | |
| x.LeaseNamespace = Namespace.Namespace; | |
| }); |
🧰 Tools
🪛 GitHub Check: Build
[warning] 122-122:
Non-constant fields should not be visible (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2211)
[warning] 118-118:
Non-constant fields should not be visible (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2211)
[warning] 120-120:
Non-constant fields should not be visible (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2211)
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorExtensions.cs` around lines 118 - 130, The static
attribute fields Namespace, Docker and Operator should be made readonly and
obtain attributes from Assembly.GetEntryAssembly() with a fallback to
Assembly.GetExecutingAssembly() to ensure app-level attributes are used
(GetEntryAssembly can be null in some hosts); update the declarations for
Namespace, Docker and Operator to be public static readonly and replace
Assembly.GetExecutingAssembly() calls with a pattern like
Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly() so existing uses
(e.g., the LeaderElection object that references Operator.OperatorName and
Namespace.Namespace) continue to work and CA2211 is addressed.
| // Expected when leadership is lost | ||
| } | ||
|
|
||
| _runningTasks.Clear(); |
There was a problem hiding this comment.
Potential race condition with _runningTasks field.
_runningTasks is a class-level field modified in StartWatchers (line 95) and cleared in ExecuteAsync (line 61). If leadership is lost while Task.WhenAll(_runningTasks) is executing, the outer loop clears the list before WhenAll completes. Consider making _runningTasks local to StartWatchers and returning the aggregated task.
♻️ Suggested fix
- private readonly List<Task> _runningTasks = [];
...
- public async Task StartWatchers(CancellationToken stoppingToken)
+ public async Task StartWatchers(CancellationToken stoppingToken)
{
var watchers = Datasource.GetWatchers().ToList();
var logger = ServiceProvider.GetRequiredService<ILogger<OperatorService>>();
+ var runningTasks = new List<Task>();
if (!watchers.Any())
{
logger.LogInformation("No event watchers registered.");
return;
}
- using var cts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken);
foreach (var watcher in watchers)
{
var task = Task.Run(async () =>
{
...
- }, cts.Token);
- _runningTasks.Add(task);
+ }, stoppingToken);
+ runningTasks.Add(task);
}
- await Task.WhenAll(_runningTasks);
+ await Task.WhenAll(runningTasks);
}Also applies to: 95-98
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorService.cs` at line 61, There is a race on the
class field _runningTasks being cleared in ExecuteAsync while StartWatchers
populates it and Task.WhenAll(_runningTasks) awaits it; change StartWatchers to
create a local List<Task> (instead of writing to the class field _runningTasks),
have StartWatchers return a single aggregated Task (e.g., Task
returnedFromStartWatchers = Task.WhenAll(localList)) and await that returned
Task in ExecuteAsync (remove/stop using _runningTasks.Clear()); update any
callers of StartWatchers to use the returned Task and remove modifications to
the _runningTasks field to eliminate the race.
| # Kubernetes | ||
| *.yaml | ||
| *.yml | ||
|
|
There was a problem hiding this comment.
Avoid blanket ignoring all YAML files.
Lines 35–38 ignore every *.yaml/*.yml, which can drop appsettings or deployment configs from the Docker build context. Consider removing these patterns or scoping them to a specific manifests directory.
🛠️ Suggested adjustment
-# Kubernetes
-*.yaml
-*.yml📝 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.
| # Kubernetes | |
| *.yaml | |
| *.yml |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Templates/.dockerignore.template` around lines 35 - 38,
The .dockerignore.template currently excludes all YAML files via the patterns
"*.yaml" and "*.yml", which can unintentionally omit important files (e.g.,
appsettings or deployment configs); update the template by removing these
blanket patterns and instead either narrow them to a specific manifests
directory (e.g., use "manifests/*.yaml" and "manifests/*.yml") or add explicit
allow rules for required files (e.g., "!appsettings*.yaml") so only intended
YAMLs are excluded.
| <!-- Enable launch settings creation for testing --> | ||
| <CreateOperatorLaunchSettings>true</CreateOperatorLaunchSettings> | ||
| <CreateOperatorLaunchSettings>false</CreateOperatorLaunchSettings> |
There was a problem hiding this comment.
Misleading comment: update or remove.
The comment states "Enable launch settings creation for testing" but CreateOperatorLaunchSettings is now set to false. Consider updating the comment to reflect the actual intent (e.g., "Disable launch settings creation...") or removing it.
🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Target.Tests/K8sOperator.NET.Target.Tests.csproj` around
lines 6 - 7, The XML comment above the CreateOperatorLaunchSettings property is
inaccurate; update or remove it so it matches the property's intent: either
change the comment to "Disable launch settings creation for testing" (or
similar) when CreateOperatorLaunchSettings is false, or delete the comment
entirely; locate the CreateOperatorLaunchSettings element in the project file
and make the comment and its wording consistent with the property's value.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (1)
274-282: Incorrect JSON Schema type mapping for Boolean.
System.Booleanis mapped to"string"but should be mapped to"boolean"per JSON Schema specification. This 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/Commands/InstallCommand.cs (1)
126-145: RemoveRunAsRoot()call—the method is semantically backwards and causes confusion.The
RunAsRoot()method at lines 301-305 ofDeploymentBuilderExtensions.cshas inverted semantics: it setsRunAsNonRoot = runAsRoot. CallingRunAsRoot()with the default parameter actually setsRunAsNonRoot = true, meaning "run as non-root"—the opposite of what the method name suggests.While the container will correctly run as non-root user 2024 due to line 142
RunAsUser(2024), theRunAsRoot()call at line 141 is confusing and should be removed since it's redundant with the pod-levelRunAsNonRoot = truealready set at line 129. The method itself needs to be fixed or renamed to avoid this semantic trap.
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/OperatorExtensions.cs`:
- Around line 125-132: The constructor for OperatorBuilder sets LeaderElection
defaults using Operator.OperatorName and Namespace.Namespace which come from
GetExecutingAssembly() and thus yield library assembly values; change the
default derivation to use the application/entry assembly instead (use
Assembly.GetEntryAssembly() or another entry-assembly-based helper) when
building LeaderElectionOptions in the OperatorBuilder constructor (where
LeaderElection, LeaderElectionOptions, ObjectBuilder<LeaderElectionOptions>.Add
are used) so the LeaseName and LeaseNamespace reflect the application assembly
rather than the library.
♻️ Duplicate comments (4)
src/K8sOperator.NET/Metadata/AdditionalPrinterColumn.cs (1)
3-11: Use primary constructor to enforce required properties and align with codebase conventions.This issue was previously raised:
Name,Type, andPathare non-nullable but uninitialized, causing compiler warnings and potential null reference issues at runtime. Other attributes in this codebase consistently use primary constructors (e.g.,LabelSelectorAttribute,FinalizerAttribute,DockerImageAttribute).✅ Suggested fix
[AttributeUsage(AttributeTargets.Class, AllowMultiple = true)] -public class AdditionalPrinterColumnAttribute : Attribute +public class AdditionalPrinterColumnAttribute(string name, string type, string path) : Attribute { - public string Name { get; set; } - public string Type { get; set; } - public string Description { get; set; } - public string Path { get; set; } + public string Name { get; } = name; + public string Type { get; } = type; + public string Path { get; } = path; + public string? Description { get; set; } public int Priority { get; set; } }Note: If this fix is applied, the attribute usage in
TodoItem.cswould need to change to positional arguments:[AdditionalPrinterColumn("Title", "string", ".spec.title", Description = "Todo Title")]src/K8sOperator.NET/OperatorExtensions.cs (1)
118-123: Static fields still useGetExecutingAssembly()and lackreadonly.This issue was previously flagged. The static fields still use
Assembly.GetExecutingAssembly()which returns the library assembly rather than the application assembly where these attributes would be defined. They should useGetEntryAssembly()with a fallback and be markedreadonlyto address CA2211 warnings.src/K8sOperator.NET/Commands/InstallCommand.cs (2)
48-57: Existing concern:Create<T>()may leaveMetadatanull.This issue was previously flagged. The same pattern here using
Create<V1Namespace>()followed byWithName()risks aNullReferenceExceptionifMetadatais not initialized byInitialize().
164-175: Existing concern: Hardcoded "system" namespace in ServiceAccount binding.This issue was previously flagged. The
WithSubjectcall hardcodesns: "system"but should use the configured namespace fromNamespaceAttributeto ensure RBAC applies to the operator's actual namespace.
🧹 Nitpick comments (5)
src/K8sOperator.NET/EventWatcher.cs (1)
5-5: Unused import statement.The
using K8sOperator.NET.Generation;directive appears to be unused in this file. The code only uses types fromK8sOperator.NET.Metadata(likeFinalizerAttribute,LabelSelectorAttribute,NamespaceAttribute,ScopeAttribute) andK8sOperator.NETnamespace types.Proposed fix
-using K8sOperator.NET.Generation;src/K8sOperator.NET/Generation/CustomResourceDefinitionBuilderExtensions.cs (2)
4-4: Unused import statement.The
using System.Reflection.Emit;directive does not appear to be used anywhere in this file. The code usesSystem.Reflection(forPropertyInfo,NullabilityInfoContext, etc.) but notSystem.Reflection.Emit.Proposed fix
-using System.Reflection.Emit;
145-162: Add XML documentation for consistency.This new method is missing XML documentation comments that are present on all other public methods in this file. Adding documentation would maintain consistency and improve API discoverability.
Proposed documentation
+ /// <summary> + /// Adds an additional printer column to the CustomResourceDefinition version. + /// </summary> + /// <typeparam name="TBuilder">The type of the builder.</typeparam> + /// <param name="builder">The builder instance.</param> + /// <param name="name">The name of the column to display.</param> + /// <param name="type">The type of the column (e.g., "string", "integer", "date").</param> + /// <param name="description">A description of the column.</param> + /// <param name="jsonPath">The JSON path to the field in the custom resource.</param> + /// <param name="priority">The priority of the column (default 0). Higher priority columns are shown first.</param> + /// <returns>The configured builder.</returns> public static TBuilder WithAdditionalPrinterColumn<TBuilder>(this TBuilder builder, string name, string type, string description, string jsonPath, int priority = 0) where TBuilder : IObjectBuilder<V1CustomResourceDefinitionVersion>src/K8sOperator.NET/Commands/InstallCommand.cs (2)
93-97: Remove debug remnants.The intermediate variable
testand extra blank lines appear to be leftover debug code. Consider returning the build result directly.♻️ Suggested cleanup
- var test = crdBuilder.Build(); - - - - return test; + return crdBuilder.Build();
190-193: Consider restricting lease verbs instead of using wildcard.The
"*"verb grants all permissions on leases, which is more permissive than necessary for leader election. Explicit verbs improve security posture and make the required permissions clear.♻️ Suggested change
clusterrole.AddRule() .WithGroups("coordination.k8s.io") .WithResources("leases") - .WithVerbs("*"); + .WithVerbs("get", "list", "watch", "create", "update", "delete");
| public OperatorBuilder() | ||
| { | ||
| LeaderElection = new ObjectBuilder<LeaderElectionOptions>().Add(x => | ||
| { | ||
| x.LeaseName = $"{Operator.OperatorName}-leader-election"; | ||
| x.LeaseNamespace = Namespace.Namespace; | ||
| }).Build(); | ||
| } |
There was a problem hiding this comment.
Leader election defaults may use incorrect assembly attributes.
The constructor initializes LeaderElection using Operator.OperatorName and Namespace.Namespace, which are derived from GetExecutingAssembly(). This means the lease name and namespace will default to values from the library assembly rather than the application assembly. This is a consequence of the static field issue flagged previously.
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorExtensions.cs` around lines 125 - 132, The
constructor for OperatorBuilder sets LeaderElection defaults using
Operator.OperatorName and Namespace.Namespace which come from
GetExecutingAssembly() and thus yield library assembly values; change the
default derivation to use the application/entry assembly instead (use
Assembly.GetEntryAssembly() or another entry-assembly-based helper) when
building LeaderElectionOptions in the OperatorBuilder constructor (where
LeaderElection, LeaderElectionOptions, ObjectBuilder<LeaderElectionOptions>.Add
are used) so the LeaseName and LeaseNamespace reflect the application assembly
rather than the library.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/LeaderLectionService.cs`:
- Around line 89-105: StartAsync's leader-election loop must be guarded so
transient Kubernetes/API exceptions don't break the loop; wrap the body that
calls TryAcquireLeaseAsync and sets IsLeader/_renewalTask in a try-catch that
mirrors RenewLeaseLoopAsync: catch OperationCanceledException and rethrow, catch
general Exception (or ApiException) as ex, set IsLeader = false, log the error,
then await Task.Delay(_options.RetryPeriod, stoppingToken) before continuing;
keep the existing flow for successful acquisition (assign _renewalTask and await
it) and ensure cancellation token is honored.
In `@src/K8sOperator.NET/OperatorService.cs`:
- Around line 35-55: The leadership wait can throw OperationCanceledException
and skip watcher cleanup; wrap the call to
LeaderElectionService.WaitForLeadershipLostAsync(stoppingToken) in a try/finally
so watcher cleanup always runs: create watcherCts and start watcherTask via
StartWatchers(watcherCts.Token) as before, then call WaitForLeadershipLostAsync
inside try, and in the finally call watcherCts.CancelAsync(), await watcherTask
(handling OperationCanceledException as expected), and then call
_runningTasks.Clear(); ensure you reference watcherCts, watcherTask,
StartWatchers, LeaderElectionService.WaitForLeadershipLostAsync, CancelAsync and
_runningTasks.Clear when making the change.
♻️ Duplicate comments (1)
src/K8sOperator.NET/OperatorExtensions.cs (1)
115-120: Make static metadata fields readonly and entry‑assembly aware.These mutable public static fields still trigger CA2211 and read attributes from the library assembly rather than the app entry assembly.
♻️ Proposed fix
- public static NamespaceAttribute Namespace = Assembly.GetExecutingAssembly().GetCustomAttribute<NamespaceAttribute>() ?? - NamespaceAttribute.Default; - public static DockerImageAttribute Docker = Assembly.GetExecutingAssembly().GetCustomAttribute<DockerImageAttribute>() ?? - DockerImageAttribute.Default; - public static OperatorNameAttribute Operator = Assembly.GetExecutingAssembly().GetCustomAttribute<OperatorNameAttribute>() ?? - OperatorNameAttribute.Default; + public static readonly NamespaceAttribute Namespace = + (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly()) + .GetCustomAttribute<NamespaceAttribute>() ?? NamespaceAttribute.Default; + public static readonly DockerImageAttribute Docker = + (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly()) + .GetCustomAttribute<DockerImageAttribute>() ?? DockerImageAttribute.Default; + public static readonly OperatorNameAttribute Operator = + (Assembly.GetEntryAssembly() ?? Assembly.GetExecutingAssembly()) + .GetCustomAttribute<OperatorNameAttribute>() ?? OperatorNameAttribute.Default;
| public async Task StartAsync(CancellationToken stoppingToken) | ||
| { | ||
| while (!stoppingToken.IsCancellationRequested) | ||
| { | ||
| if (await TryAcquireLeaseAsync(stoppingToken)) | ||
| { | ||
| IsLeader = true; | ||
| _renewalTask = RenewLeaseLoopAsync(stoppingToken); | ||
| await _renewalTask; | ||
| } | ||
| else | ||
| { | ||
| IsLeader = false; | ||
| await Task.Delay(_options.RetryPeriod, stoppingToken); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the LeaderLectionService.cs file
find . -name "LeaderLectionService.cs" -type fRepository: pmdevers/K8sOperator.NET
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Read the entire LeaderLectionService.cs file to understand the context
wc -l ./src/K8sOperator.NET/LeaderLectionService.csRepository: pmdevers/K8sOperator.NET
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Read the file to see the code at lines 89-105 and surrounding context
cat -n ./src/K8sOperator.NET/LeaderLectionService.cs | sed -n '80,115p'Repository: pmdevers/K8sOperator.NET
Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Read more to see the complete RenewLeaseLoopAsync and understand exception handling patterns
cat -n ./src/K8sOperator.NET/LeaderLectionService.cs | sed -n '107,150p'Repository: pmdevers/K8sOperator.NET
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Read the rest of TryAcquireLeaseAsync method
cat -n ./src/K8sOperator.NET/LeaderLectionService.cs | sed -n '130,180p'Repository: pmdevers/K8sOperator.NET
Length of output: 2457
🏁 Script executed:
#!/bin/bash
# Read the rest of TryUpdateLeaseAsync method
cat -n ./src/K8sOperator.NET/LeaderLectionService.cs | sed -n '159,201p'Repository: pmdevers/K8sOperator.NET
Length of output: 1949
🏁 Script executed:
#!/bin/bash
# Let me check if there's any exception handling in the service's StartAsync that wraps the entire method
cat -n ./src/K8sOperator.NET/LeaderLectionService.cs | sed -n '1,50p'Repository: pmdevers/K8sOperator.NET
Length of output: 2092
🏁 Script executed:
#!/bin/bash
# Check the ILeaderElectionService interface to see if there's any contract about exception handling
grep -r "ILeaderElectionService" --include="*.cs" -A 10 | head -50Repository: pmdevers/K8sOperator.NET
Length of output: 4015
🏁 Script executed:
#!/bin/bash
# Check how StartAsync is called in OperatorService
grep -n "StartAsync" src/K8sOperator.NET/OperatorService.cs -B 5 -A 5Repository: pmdevers/K8sOperator.NET
Length of output: 622
Guard the leader-election loop against transient Kubernetes API failures.
The StartAsync method lacks exception handling. TryAcquireLeaseAsync can throw unhandled exceptions (network errors, timeouts, non-409 HTTP errors) that would exit the loop and leave the operator unable to retry leadership acquisition without a restart. Wrap the loop body in try-catch to retry with RetryPeriod on transient failures, matching the exception handling pattern already used in RenewLeaseLoopAsync.
Proposed fix
public async Task StartAsync(CancellationToken stoppingToken)
{
while (!stoppingToken.IsCancellationRequested)
{
- if (await TryAcquireLeaseAsync(stoppingToken))
- {
- IsLeader = true;
- _renewalTask = RenewLeaseLoopAsync(stoppingToken);
- await _renewalTask;
- }
- else
- {
- IsLeader = false;
- await Task.Delay(_options.RetryPeriod, stoppingToken);
- }
+ try
+ {
+ if (await TryAcquireLeaseAsync(stoppingToken))
+ {
+ IsLeader = true;
+ _renewalTask = RenewLeaseLoopAsync(stoppingToken);
+ await _renewalTask;
+ }
+ else
+ {
+ IsLeader = false;
+ await Task.Delay(_options.RetryPeriod, stoppingToken);
+ }
+ }
+ catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested)
+ {
+ // Shutdown path
+ }
+ catch (Exception)
+ {
+ IsLeader = false;
+ await Task.Delay(_options.RetryPeriod, stoppingToken);
+ }
}
}📝 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 async Task StartAsync(CancellationToken stoppingToken) | |
| { | |
| while (!stoppingToken.IsCancellationRequested) | |
| { | |
| if (await TryAcquireLeaseAsync(stoppingToken)) | |
| { | |
| IsLeader = true; | |
| _renewalTask = RenewLeaseLoopAsync(stoppingToken); | |
| await _renewalTask; | |
| } | |
| else | |
| { | |
| IsLeader = false; | |
| await Task.Delay(_options.RetryPeriod, stoppingToken); | |
| } | |
| } | |
| } | |
| public async Task StartAsync(CancellationToken stoppingToken) | |
| { | |
| while (!stoppingToken.IsCancellationRequested) | |
| { | |
| try | |
| { | |
| if (await TryAcquireLeaseAsync(stoppingToken)) | |
| { | |
| IsLeader = true; | |
| _renewalTask = RenewLeaseLoopAsync(stoppingToken); | |
| await _renewalTask; | |
| } | |
| else | |
| { | |
| IsLeader = false; | |
| await Task.Delay(_options.RetryPeriod, stoppingToken); | |
| } | |
| } | |
| catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) | |
| { | |
| // Shutdown path | |
| } | |
| catch (Exception) | |
| { | |
| IsLeader = false; | |
| await Task.Delay(_options.RetryPeriod, stoppingToken); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/LeaderLectionService.cs` around lines 89 - 105,
StartAsync's leader-election loop must be guarded so transient Kubernetes/API
exceptions don't break the loop; wrap the body that calls TryAcquireLeaseAsync
and sets IsLeader/_renewalTask in a try-catch that mirrors RenewLeaseLoopAsync:
catch OperationCanceledException and rethrow, catch general Exception (or
ApiException) as ex, set IsLeader = false, log the error, then await
Task.Delay(_options.RetryPeriod, stoppingToken) before continuing; keep the
existing flow for successful acquisition (assign _renewalTask and await it) and
ensure cancellation token is honored.
| using var watcherCts = CancellationTokenSource.CreateLinkedTokenSource(stoppingToken); | ||
| var watcherTask = Task.Run(() => StartWatchers(watcherCts.Token), watcherCts.Token); | ||
|
|
||
| // Wait until leadership is lost (event-driven) | ||
| await LeaderElectionService.WaitForLeadershipLostAsync(stoppingToken); | ||
|
|
||
| logger.LogInformation("Lost leadership. Stopping watchers..."); | ||
|
|
||
| // Leadership lost or stopping, cancel watchers | ||
| await watcherCts.CancelAsync(); | ||
|
|
||
| try | ||
| { | ||
| await watcherTask; | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Expected when leadership is lost | ||
| } | ||
|
|
||
| _runningTasks.Clear(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/K8sOperator.NET/OperatorService.cs | head -70Repository: pmdevers/K8sOperator.NET
Length of output: 3170
🏁 Script executed:
find . -type f -name "*.cs" | xargs grep -l "WaitForLeadershipLostAsync" | head -10Repository: pmdevers/K8sOperator.NET
Length of output: 202
🏁 Script executed:
cat -n src/K8sOperator.NET/ILeaderElectionService.csRepository: pmdevers/K8sOperator.NET
Length of output: 1121
🏁 Script executed:
cat -n src/K8sOperator.NET/LeaderLectionService.csRepository: pmdevers/K8sOperator.NET
Length of output: 7856
Wrap the leadership wait in try/finally to ensure watcher cleanup runs on shutdown.
When stoppingToken is cancelled during WaitForLeadershipLostAsync (line 39), it throws OperationCanceledException before reaching the cleanup code (lines 44–55: watcher cancellation, task await, and _runningTasks.Clear). A try/finally block ensures cleanup runs regardless of how the wait exits.
Proposed fix
- // Wait until leadership is lost (event-driven)
- await LeaderElectionService.WaitForLeadershipLostAsync(stoppingToken);
-
- logger.LogInformation("Lost leadership. Stopping watchers...");
-
- // Leadership lost or stopping, cancel watchers
- await watcherCts.CancelAsync();
-
- try
- {
- await watcherTask;
- }
- catch (OperationCanceledException)
- {
- // Expected when leadership is lost
- }
-
- _runningTasks.Clear();
+ try
+ {
+ // Wait until leadership is lost (event-driven)
+ await LeaderElectionService.WaitForLeadershipLostAsync(stoppingToken);
+ logger.LogInformation("Lost leadership. Stopping watchers...");
+ }
+ catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested)
+ {
+ // Shutdown path
+ }
+ finally
+ {
+ // Leadership lost or stopping, cancel watchers
+ await watcherCts.CancelAsync();
+ try
+ {
+ await watcherTask;
+ }
+ catch (OperationCanceledException)
+ {
+ // Expected when leadership is lost
+ }
+ _runningTasks.Clear();
+ }🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/OperatorService.cs` around lines 35 - 55, The leadership
wait can throw OperationCanceledException and skip watcher cleanup; wrap the
call to LeaderElectionService.WaitForLeadershipLostAsync(stoppingToken) in a
try/finally so watcher cleanup always runs: create watcherCts and start
watcherTask via StartWatchers(watcherCts.Token) as before, then call
WaitForLeadershipLostAsync inside try, and in the finally call
watcherCts.CancelAsync(), await watcherTask (handling OperationCanceledException
as expected), and then call _runningTasks.Clear(); ensure you reference
watcherCts, watcherTask, StartWatchers,
LeaderElectionService.WaitForLeadershipLostAsync, CancelAsync and
_runningTasks.Clear when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/EventWatcher.cs`:
- Around line 321-330: The null-coalescing defaults in EventWatcher for
FinalizerAttribute and LabelSelectorAttribute cause a type mismatch
(FinalizerAttribute.Default is a string constant and LabelSelectorAttribute
lacks a Default), so replace the fallbacks with concrete attribute instances or
add static Default properties on those attribute types; e.g., in EventWatcher
change the Finalizer property to return
Metadata.OfType<FinalizerAttribute>().FirstOrDefault() ?? new
FinalizerAttribute(/* appropriate empty/default value */) and similarly replace
LabelSelectorAttribute.Default with
Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault() ?? new
LabelSelectorAttribute(/* appropriate empty/default value */), or alternatively
add a public static Default instance to FinalizerAttribute and
LabelSelectorAttribute and keep the current null-coalescing pattern.
♻️ Duplicate comments (6)
src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs (2)
301-306:RunAsRootinverts the intended semantics.
RunAsRoot(true)setsRunAsNonRoot = true, which contradicts the method name and parameter. When a caller specifiesRunAsRoot(true), they expect the container to run as root, but this actually forces it to run as non-root.🛠️ Proposed fix
public static TBuilder RunAsRoot<TBuilder>(this TBuilder builder, bool runAsRoot = true) where TBuilder : IObjectBuilder<V1SecurityContext> { - builder.Add(x => x.RunAsNonRoot = runAsRoot); + builder.Add(x => x.RunAsNonRoot = !runAsRoot); return builder; }
212-216: UseV1SecretKeySelectorfor secret-sourced env vars.
AddEnvFromSecretKeycurrently takesV1ConfigMapKeySelector, so it buildsConfigMapKeyRefinstead ofSecretKeyRef. This breaks secret-based environment variables.🛠️ 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 : IObjectBuilder<V1Container> { return builder.AddEnv(name, action); }src/K8sOperator.NET/Commands/InstallCommand.cs (4)
47-56: UseCreateMeta<T>()or initializeMetadatabefore applying metadata mutations.
KubernetesObjectBuilder.Create<T>()callsInitialize(), which does not guarantee thatMetadatawill be non-null in KubernetesClient v18.0.13. The subsequent call toWithName()will throw aNullReferenceExceptionbecause it accessesx.Metadata.Nameon a potentially nullMetadataobject.🛠️ Suggested fix
private static V1Namespace CreateNamespace(IReadOnlyList<object> metadata) { var ns = metadata.OfType<NamespaceAttribute>().FirstOrDefault() ?? NamespaceAttribute.Default; - var nsBuilder = KubernetesObjectBuilder.Create<V1Namespace>(); + var nsBuilder = KubernetesObjectBuilder.CreateMeta<V1Namespace>(); nsBuilder.WithName(ns.Namespace); return nsBuilder.Build(); }
66-68: SameMetadatanull risk applies to CRD builder.
KubernetesObjectBuilder.Create<V1CustomResourceDefinition>()followed byWithName()has the same potentialNullReferenceExceptionissue. UseCreateMeta<T>()instead.🛠️ Suggested fix
- var crdBuilder = KubernetesObjectBuilder.Create<V1CustomResourceDefinition>(); + var crdBuilder = KubernetesObjectBuilder.CreateMeta<V1CustomResourceDefinition>();
108-113: SameMetadatanull risk applies to Deployment builder.
KubernetesObjectBuilder.Create<V1Deployment>()followed byWithName(),WithNamespace(), andWithLabel()has the same potentialNullReferenceExceptionissue.🛠️ Suggested fix
- var deployment = KubernetesObjectBuilder.Create<V1Deployment>(); + var deployment = KubernetesObjectBuilder.CreateMeta<V1Deployment>();
184-185: SameMetadatanull risk applies to ClusterRole builder.
KubernetesObjectBuilder.Create<V1ClusterRole>()followed byWithName()has the same potentialNullReferenceExceptionissue.🛠️ Suggested fix
- var clusterrole = KubernetesObjectBuilder.Create<V1ClusterRole>() - .WithName($"{name.OperatorName}-role"); + var clusterrole = KubernetesObjectBuilder.CreateMeta<V1ClusterRole>() + .WithName($"{name.OperatorName}-role");
🧹 Nitpick comments (2)
src/K8sOperator.NET/Commands/InstallCommand.cs (1)
92-96: Rename debug variable and remove unnecessary blank lines.The variable
testappears to be leftover debug naming. Consider renaming it to something more descriptive likecrd. The extra blank lines (93-95) can also be removed.♻️ Suggested cleanup
- var test = crdBuilder.Build(); - - - - return test; + return crdBuilder.Build();src/K8sOperator.NET/Generation/KubernetesObjectBuilder.cs (1)
64-68: Add XML documentation forCreateMeta<T>()method.The
Create<T>()method has XML documentation butCreateMeta<T>()lacks it. Consider adding documentation to explain its purpose and when to use it overCreate<T>().📝 Suggested documentation
+ /// <summary> + /// Creates a new Kubernetes object builder for types with metadata, + /// explicitly initializing the Metadata property. + /// </summary> + /// <typeparam name="T">The type of the Kubernetes object with metadata.</typeparam> + /// <returns>A new instance of <see cref="IObjectBuilder{T}"/> with Metadata initialized.</returns> + /// <remarks> + /// Use this method instead of <see cref="Create{T}"/> when chaining metadata + /// mutation methods like WithName, WithNamespace, or WithLabel to avoid + /// NullReferenceException on uninitialized Metadata. + /// </remarks> public static IObjectBuilder<T> CreateMeta<T>() where T : IMetadata<V1ObjectMeta>, new() { return new ObjectBuilder<T>().Add(x => x.Metadata = new()); }
src/K8sOperator.NET/EventWatcher.cs
Outdated
| private NamespaceAttribute Namespace => metadata.OfType<NamespaceAttribute>().FirstOrDefault() ?? | ||
| NamespaceAttribute.Default; | ||
|
|
||
| private ScopeAttribute Scope => metadata.OfType<ScopeAttribute>().FirstOrDefault() ?? | ||
| ScopeAttribute.Default; | ||
| private FinalizerAttribute Finalizer => Metadata.OfType<FinalizerAttribute>().FirstOrDefault() | ||
| ?? FinalizerAttribute.Default; | ||
|
|
||
| private LabelSelectorAttribute LabelSelector => Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault() | ||
| ?? LabelSelectorAttribute.Default; |
There was a problem hiding this comment.
Fix default attribute fallback type mismatch (compile error).
FinalizerAttribute.Default is a string constant (per src/K8sOperator.NET/Metadata/FinalizerAttribute.cs), and LabelSelectorAttribute doesn’t expose a Default in the provided snippet, so these null-coalescing defaults won’t type-check. Use concrete default attribute instances (or add static Default attributes on those types).
🐛 Proposed fix (local defaults in EventWatcher)
+ private static readonly FinalizerAttribute DefaultFinalizer =
+ new(FinalizerAttribute.Default);
+ private static readonly LabelSelectorAttribute DefaultLabelSelector =
+ new(string.Empty);
+
private NamespaceAttribute Namespace => metadata.OfType<NamespaceAttribute>().FirstOrDefault() ??
NamespaceAttribute.Default;
private ScopeAttribute Scope => metadata.OfType<ScopeAttribute>().FirstOrDefault() ??
ScopeAttribute.Default;
private FinalizerAttribute Finalizer => Metadata.OfType<FinalizerAttribute>().FirstOrDefault()
- ?? FinalizerAttribute.Default;
+ ?? DefaultFinalizer;
private LabelSelectorAttribute LabelSelector => Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault()
- ?? LabelSelectorAttribute.Default;
+ ?? DefaultLabelSelector;📝 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 NamespaceAttribute Namespace => metadata.OfType<NamespaceAttribute>().FirstOrDefault() ?? | |
| NamespaceAttribute.Default; | |
| private ScopeAttribute Scope => metadata.OfType<ScopeAttribute>().FirstOrDefault() ?? | |
| ScopeAttribute.Default; | |
| private FinalizerAttribute Finalizer => Metadata.OfType<FinalizerAttribute>().FirstOrDefault() | |
| ?? FinalizerAttribute.Default; | |
| private LabelSelectorAttribute LabelSelector => Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault() | |
| ?? LabelSelectorAttribute.Default; | |
| private static readonly FinalizerAttribute DefaultFinalizer = | |
| new(FinalizerAttribute.Default); | |
| private static readonly LabelSelectorAttribute DefaultLabelSelector = | |
| new(string.Empty); | |
| private NamespaceAttribute Namespace => metadata.OfType<NamespaceAttribute>().FirstOrDefault() ?? | |
| NamespaceAttribute.Default; | |
| private ScopeAttribute Scope => metadata.OfType<ScopeAttribute>().FirstOrDefault() ?? | |
| ScopeAttribute.Default; | |
| private FinalizerAttribute Finalizer => Metadata.OfType<FinalizerAttribute>().FirstOrDefault() | |
| ?? DefaultFinalizer; | |
| private LabelSelectorAttribute LabelSelector => Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault() | |
| ?? DefaultLabelSelector; |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/EventWatcher.cs` around lines 321 - 330, The
null-coalescing defaults in EventWatcher for FinalizerAttribute and
LabelSelectorAttribute cause a type mismatch (FinalizerAttribute.Default is a
string constant and LabelSelectorAttribute lacks a Default), so replace the
fallbacks with concrete attribute instances or add static Default properties on
those attribute types; e.g., in EventWatcher change the Finalizer property to
return Metadata.OfType<FinalizerAttribute>().FirstOrDefault() ?? new
FinalizerAttribute(/* appropriate empty/default value */) and similarly replace
LabelSelectorAttribute.Default with
Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault() ?? new
LabelSelectorAttribute(/* appropriate empty/default value */), or alternatively
add a public static Default instance to FinalizerAttribute and
LabelSelectorAttribute and keep the current null-coalescing pattern.
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 (2)
src/K8sOperator.NET/Commands/InstallCommand.cs (1)
198-208: Ensure consistent lowercase for RBAC resource names.The CRD generation lowercases
PluralName(line 74:group.PluralName.ToLower()), but the ClusterRole rules usex.PluralNamewithout lowercasing. If thePluralNamecontains uppercase characters, the RBAC rules won't match the actual resource names, causing permission denials.🛠️ Proposed fix
foreach (var item in rules) { clusterrole.AddRule() .WithGroups(item.Key) - .WithResources([.. item.Select(x => x.PluralName)]) + .WithResources([.. item.Select(x => x.PluralName.ToLower())]) .WithVerbs("*"); clusterrole.AddRule() .WithGroups(item.Key) - .WithResources([.. item.Select(x => $"{x.PluralName}/status")]) + .WithResources([.. item.Select(x => $"{x.PluralName.ToLower()}/status")]) .WithVerbs("get", "update", "patch"); }src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs (1)
246-364: Fix the XML doc param name forRunAsNonRoot.The doc still refers to
runAsRoot, which is misleading now that the parameter isrunAsNonRoot. The rest of the security-context refactor reads fine.✏️ Proposed doc fix
- /// <param name="runAsRoot">A value indicating whether to run as a root user. Defaults to true.</param> + /// <param name="runAsNonRoot">A value indicating whether to run as a non-root user. Defaults to true.</param>
♻️ Duplicate comments (2)
src/K8sOperator.NET/EventWatcher.cs (1)
326-330: Fix default attribute fallback type mismatch (compile error).This issue was previously flagged:
FinalizerAttribute.DefaultandLabelSelectorAttribute.Defaultmay not be valid attribute instances. IfFinalizerAttribute.Defaultis a string constant rather than an attribute instance, this will fail to compile.🐛 Proposed fix using concrete default instances
+ private static readonly FinalizerAttribute DefaultFinalizer = + new(FinalizerAttribute.Default); + private static readonly LabelSelectorAttribute DefaultLabelSelector = + new(string.Empty); + private FinalizerAttribute Finalizer => Metadata.OfType<FinalizerAttribute>().FirstOrDefault() - ?? FinalizerAttribute.Default; + ?? DefaultFinalizer; private LabelSelectorAttribute LabelSelector => Metadata.OfType<LabelSelectorAttribute>().FirstOrDefault() - ?? LabelSelectorAttribute.Default; + ?? DefaultLabelSelector;#!/bin/bash # Verify FinalizerAttribute.Default type - is it a string or attribute instance? ast-grep --pattern $'class FinalizerAttribute { $$$ static $_ Default $$$ $$$ }' # Also check with simpler pattern rg -n "public static.*Default" --type cs -A2 -B2 | head -50src/K8sOperator.NET/Generation/DeploymentBuilderExtensions.cs (1)
212-216: UseV1SecretKeySelectorfor secret-sourced env vars.This still builds a
ConfigMapKeyRef, not aSecretKeyRef.🔧 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)Kubernetes C# client: V1EnvVarSource SecretKeyRef expects V1SecretKeySelector (not V1ConfigMapKeySelector)
🧹 Nitpick comments (1)
src/K8sOperator.NET/Commands/InstallCommand.cs (1)
188-191: Consider tightening lease permissions.Using
"*"for lease verbs is common for operator leader election but grants more permissions than needed. If you want to follow least-privilege, consider restricting to the specific verbs required:"get", "list", "watch", "create", "update", "patch", "delete".
Adds support for specifying additional printer columns in the Custom Resource Definition (CRD). This allows users to view important information about their custom resources directly from the
kubectl getcommand, improving usability and observability.The changes include:
AdditionalPrinterColumnAttributeto define printer columns within the custom resource class.Summary by CodeRabbit
New Features
Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.