Conversation
📝 WalkthroughWalkthroughCentralizes operator configuration via OperatorConfiguration/provider, adds a BuildTasks project with MSBuild tasks for Dockerfile and launchSettings generation, and refactors builders, watchers, targets, commands, examples, and tests to use DI-driven configuration and task-based generation. Changes
Sequence Diagram(s)sequenceDiagram
participant MSBuild as MSBuild
participant Project as Project (.csproj / Operator.targets)
participant BuildTasksDll as BuildTasks.dll
participant Task as GenerateDockerfileTask / GenerateLaunchSettingsTask
participant Filesystem as Filesystem
Note over MSBuild,Project: Build evaluation & task invocation
MSBuild->>Project: Evaluate Operator.targets / _GetPackageFiles
Project->>BuildTasksDll: Resolve _BuildTasksPath and load UsingTask types
Project->>Task: Invoke GenerateLaunchSettingsTask(ProjectDirectory)
Task->>BuildTasksDll: Read embedded launchSettings.json.template
Task->>Filesystem: Write Properties/launchSettings.json (if missing)
Project->>Task: Invoke GenerateDockerfileTask(ProjectDirectory,ProjectName,TargetFramework,OperatorName,ContainerRegistry,ContainerRepository,ContainerTag)
Task->>BuildTasksDll: Read Dockerfile/.dockerignore templates
Task->>Filesystem: Write Dockerfile & .dockerignore (if missing)
Task-->>Project: Log results
sequenceDiagram
participant Startup as App Startup
participant DI as Dependency Injection
participant Provider as OperatorConfigurationProvider
participant Config as OperatorConfiguration
participant EventDS as EventWatcherDatasource
participant Watcher as EventWatcher
Startup->>DI: Register services and Provider
DI->>Provider: Instantiate (IConfiguration?, Assembly?)
Provider->>Config: Build() (apply assembly attributes -> bind IConfiguration)
DI-->>Config: Register singleton OperatorConfiguration
Startup->>EventDS: Resolve (requires OperatorConfiguration)
EventDS->>Config: Read values (Namespace, OperatorName, ContainerImage)
EventDS->>Watcher: Create watchers passing OperatorConfiguration
Watcher->>Config: Use Namespace / ContainerImage during API calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ 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: 5
🤖 Fix all issues with AI agents
In `@examples/SimpleOperator/Properties/launchSettings.json`:
- Around line 11-14: The "Install" profile in launchSettings.json uses shell
redirection in the "commandLineArgs" value ("install > install.yaml"), but
dotnet run does not interpret '>' so the file won't be created; update the
"Install" profile's commandLineArgs to remove the redirection and either use the
CLI's supported output flag (e.g., replace the redirection with the command's
explicit output option) or leave it as just "install" and document running the
redirected command manually in a shell instead; look for the "Install" object
and its "commandLineArgs" property to make this change.
In `@src/K8sOperator.NET.BuildTasks/GenerateDockerfileTask.cs`:
- Around line 36-75: The current logic returns early if dockerfilePath exists
and always overwrites dockerignorePath when creating the Dockerfile; change
Execute (or the method containing dockerfilePath/dockerignorePath logic in
GenerateDockerfileTask) to check and handle each file individually: if
Dockerfile exists, skip creating it but continue; if .dockerignore exists, do
not overwrite it (only write if missing); use ReadEmbeddedResource and the same
placeholder replacements when creating each file; and update the Log.LogMessage
calls to report exactly which files were created (referencing dockerfilePath,
dockerignorePath, ProjectName, ExtractDotNetVersion, ReadEmbeddedResource,
ContainerRegistry/ContainerRepository/ContainerTag) instead of a single
all-or-nothing message.
In `@src/K8sOperator.NET.BuildTasks/GenerateLaunchSettingsTask.cs`:
- Around line 36-38: Replace the corrupted emoji "?" in the log message with the
intended checkmark emoji (✅) in the Log.LogMessage call that writes $"?
Generated launchSettings.json at: {launchSettingsPath}"; locate the call inside
the GenerateLaunchSettingsTask class and update the string to $"✅ Generated
launchSettings.json at: {launchSettingsPath}" so the output shows the correct
emoji alongside the launchSettingsPath variable.
In `@src/K8sOperator.NET.BuildTasks/Templates/launchSettings.json.template`:
- Around line 11-14: The launchSettings profile "Install" currently uses shell
redirection in commandLineArgs ("install > install.yaml"), which dotnet run
won't interpret; update the "Install" profile (commandName "Project") to avoid
shell redirection by replacing the argument with a CLI-supported output flag
(e.g., "install --output install.yaml") or another method the install command
provides to write its output to a file, so the generated
launchSettings.json.template no longer relies on ">" for file creation.
In `@src/K8sOperator.NET/Configuration/OperatorConfiguration.cs`:
- Around line 24-38: The ContainerImage getter currently produces invalid values
when ContainerRegistry/ContainerRepository/ContainerTag are empty; update
OperatorConfiguration so ContainerTag defaults to "latest" and the
ContainerImage property validates inputs and fails fast: if ContainerRegistry or
ContainerRepository are null/empty (or after trimming) throw an
InvalidOperationException with a clear message; keep ContainerImage as the
getter but perform the validation there (or add a Validate/EnsureValid method
called during startup) so any Dockerfile/launchSettings generation never
receives values like "ghcr.io/:". Reference ContainerTag, ContainerRegistry,
ContainerRepository, and ContainerImage (or add a Validate method on
OperatorConfiguration).
🧹 Nitpick comments (11)
test/K8sOperator.NET.Tests/EventWatcher_Tests.cs (1)
17-22: Consider removingNamespaceAttributefrom test metadata.Since
EventWatchernow usesConfiguration.Namespaceinstead of extracting namespace from metadata, theNamespaceAttribute("default")in the metadata list (line 19) is no longer used byEventWatcher. While it doesn't cause issues (defaults align), it could be misleading for future maintainers.♻️ Remove unused attribute
private readonly List<object> _metadata = [ typeof(TestResource).GetCustomAttribute<KubernetesEntityAttribute>()!, - new NamespaceAttribute("default"), new LabelSelectorAttribute("label"), new FinalizerAttribute("finalize") ];src/K8sOperator.NET.BuildTasks/GenerateLaunchSettingsTask.cs (1)
50-59: Consider extracting sharedReadEmbeddedResourcehelper.This method is duplicated in
GenerateDockerfileTask(lines 97-106 in the relevant snippets). Extracting it to a shared utility class would reduce duplication.♻️ Extract to shared helper
Create a shared helper class:
// In a new file: EmbeddedResourceHelper.cs namespace K8sOperator.NET.BuildTasks; internal static class EmbeddedResourceHelper { public static string ReadTemplate(string resourceName) { var assembly = typeof(EmbeddedResourceHelper).Assembly; var fullResourceName = $"K8sOperator.NET.BuildTasks.Templates.{resourceName}"; using var stream = assembly.GetManifestResourceStream(fullResourceName) ?? throw new InvalidOperationException($"Could not find embedded resource: {fullResourceName}"); using var reader = new StreamReader(stream); return reader.ReadToEnd(); } }Then update both tasks to use it:
- private static string ReadEmbeddedResource(string resourceName) - { - var assembly = typeof(GenerateLaunchSettingsTask).Assembly; - var fullResourceName = $"K8sOperator.NET.BuildTasks.Templates.{resourceName}"; - - using var stream = assembly.GetManifestResourceStream(fullResourceName) - ?? throw new InvalidOperationException($"Could not find embedded resource: {fullResourceName}"); - using var reader = new StreamReader(stream); - return reader.ReadToEnd(); - } + // Use: EmbeddedResourceHelper.ReadTemplate("launchSettings.json.template")src/K8sOperator.NET.BuildTasks/K8sOperator.NET.BuildTasks.csproj (1)
11-14: Consider pinning package versions to specific releases for reproducibility.Using floating versions
17.12.*for MSBuild packages can lead to non-reproducible builds if a patch release introduces breaking changes or regressions. Pin to the latest patch version in the 17.12.x line (17.12.50).♻️ Pin to specific versions
<ItemGroup> - <PackageReference Include="Microsoft.Build.Framework" Version="17.12.*" /> - <PackageReference Include="Microsoft.Build.Utilities.Core" Version="17.12.*" /> + <PackageReference Include="Microsoft.Build.Framework" Version="17.12.50" /> + <PackageReference Include="Microsoft.Build.Utilities.Core" Version="17.12.50" /> </ItemGroup>test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.cs (1)
33-47: Rename “metadata” identifiers/test names to “configuration” for clarity.The tests now use
OperatorConfiguration, but several locals/test names still say “metadata,” which is slightly misleading. Consider renaming to match the new type.✏️ Example cleanup (apply similarly elsewhere)
- public async Task Constructor_Should_InitializeWithServiceProviderAndMetadata() + public async Task Constructor_Should_InitializeWithServiceProviderAndConfiguration() { // Arrange var serviceProvider = CreateServiceProvider(); - var metadata = CreateConfiguration(); + var configuration = CreateConfiguration(); // Act - var datasource = new EventWatcherDatasource(serviceProvider, metadata); + var datasource = new EventWatcherDatasource(serviceProvider, configuration); // Assert await Assert.That(datasource).IsNotNull(); await Assert.That(datasource.ServiceProvider).IsEqualTo(serviceProvider); - await Assert.That(datasource.Configuration).IsEqualTo(metadata); + await Assert.That(datasource.Configuration).IsEqualTo(configuration); }src/K8sOperator.NET/K8sOperator.NET.csproj (2)
40-44: AlignMicrosoft.Extensions.Configuration.Binderpatch level with the otherMicrosoft.Extensions.*packages.This project references 10.0.2 for other
Microsoft.Extensions.*packages (DependencyInjection, Hosting.Abstractions), but Binder is pinned to 10.0.0. Update it to 10.0.2 to maintain consistency across the dependency graph and avoid potential downgrade warnings.Suggested change
- <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="10.0.0" /> + <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="10.0.2" />
58-65: Consider movingPackBuildTasksto run before Pack to ensure robustness with alternative packaging workflows.The current
AfterTargets="Build"approach works with the standard workflow (dotnet buildauto-packs viaGeneratePackageOnBuild=true). However, if the packaging process ever usesdotnet pack --no-build, the Build step is skipped,PackBuildTasksnever runs, and the BuildTasks DLL won't be included. UsingBeforeTargets="Pack"instead would ensure the target runs before Pack regardless of which command invoked it, making the packaging more resilient to different build workflows.src/K8sOperator.NET/Configuration/OperatorConfigurationProvider.cs (1)
25-38: Consider adding validation for required configuration properties.The
Build()method returns configuration without validating that essential properties likeOperatorNameorContainerRepositoryare set. If these remain empty, downstream components (e.g.,ContainerImagecomputation) may produce invalid values likeghcr.io/:.💡 Suggested validation
public OperatorConfiguration Build() { var config = new OperatorConfiguration(); // 1. Start with assembly attributes (lowest priority) ApplyAssemblyAttributes(config); // 2. Apply configuration (e.g., appsettings.json) ApplyConfiguration(config); // 3. OperatorBuilder can override in AddOperator() (highest priority, done by caller) + // Validate required properties + if (string.IsNullOrEmpty(config.OperatorName)) + { + throw new InvalidOperationException("OperatorName must be configured via assembly attributes, appsettings.json, or OperatorBuilder."); + } + return config; }src/K8sOperator.NET/OperatorExtensions.cs (1)
24-47: Override logic may not work as intended for properties with defaults.The checks
!string.IsNullOrEmpty(builder.OperatorConfiguration.X)prevent overriding, butOperatorConfiguration.Namespacedefaults to"default"andContainerRegistrydefaults to"ghcr.io". If a user explicitly setsNamespace = "default"inOperatorBuilder, it would be applied. However, the intent seems to be "apply if user explicitly configured it" — currently there's no way to distinguish between "user set empty" vs "user didn't set anything."Consider using a separate flag or nullable properties in
OperatorBuilder.OperatorConfigurationto track explicit user intent, or document that empty strings in builder config mean "don't override."src/K8sOperator.NET/Operator.targets (3)
3-11: BuildTasks path resolution could be more robust.The hardcoded paths assume
netstandard2.0and specific build configurations. If the target framework changes or a custom configuration is used, the task won't be found.💡 Consider using MSBuild properties for flexibility
<PropertyGroup> - <_BuildTasksPath Condition="'$(_BuildTasksPath)' == '' AND Exists('$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Debug\netstandard2.0\K8sOperator.NET.BuildTasks.dll')">$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Debug\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath> - <_BuildTasksPath Condition="'$(_BuildTasksPath)' == '' AND Exists('$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Release\netstandard2.0\K8sOperator.NET.BuildTasks.dll')">$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Release\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath> + <_BuildTasksPath Condition="'$(_BuildTasksPath)' == '' AND Exists('$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\$(Configuration)\netstandard2.0\K8sOperator.NET.BuildTasks.dll')">$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\$(Configuration)\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath> <_BuildTasksPath Condition="'$(_BuildTasksPath)' == ''">$(MSBuildThisFileDirectory)..\build\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath> </PropertyGroup>
88-89: Inconsistent indentation on target element.Line 88 has extra leading whitespace compared to other target definitions.
🧹 Fix indentation
- <!-- Generate Dockerfile if it doesn't exist --> - <Target Name="GenerateDockerfile" BeforeTargets="Build" Condition=" '$(GenerateOperatorDockerfile)' == 'true' AND !Exists('$(MSBuildProjectDirectory)\Dockerfile') "> + <!-- Generate Dockerfile if it doesn't exist --> + <Target Name="GenerateDockerfile" BeforeTargets="Build" Condition=" '$(GenerateOperatorDockerfile)' == 'true' AND !Exists('$(MSBuildProjectDirectory)\Dockerfile') ">
100-103: Remove trailing blank lines.The file has unnecessary blank lines at the end after the closing
</Project>tag.
| "Install": { | ||
| "commandName": "Project", | ||
| "commandLineArgs": "install", | ||
| "commandLineArgs": "install > install.yaml", | ||
| "environmentVariables": { |
There was a problem hiding this comment.
Shell redirection won’t work in launchSettings.
commandLineArgs is passed directly to dotnet run; the > redirection isn’t interpreted, so install.yaml won’t be created. Prefer a supported output flag in the command or keep redirection as a manual shell step.
🔧 Suggested fix (avoid redirection in launchSettings)
- "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", | |
| "commandLineArgs": "install > install.yaml", | |
| "environmentVariables": { | |
| "Install": { | |
| "commandName": "Project", | |
| "commandLineArgs": "install", | |
| "environmentVariables": { |
🤖 Prompt for AI Agents
In `@examples/SimpleOperator/Properties/launchSettings.json` around lines 11 - 14,
The "Install" profile in launchSettings.json uses shell redirection in the
"commandLineArgs" value ("install > install.yaml"), but dotnet run does not
interpret '>' so the file won't be created; update the "Install" profile's
commandLineArgs to remove the redirection and either use the CLI's supported
output flag (e.g., replace the redirection with the command's explicit output
option) or leave it as just "install" and document running the redirected
command manually in a shell instead; look for the "Install" object and its
"commandLineArgs" property to make this change.
| Log.LogMessage(MessageImportance.High, ""); | ||
| Log.LogMessage(MessageImportance.High, $"? Generated launchSettings.json at: {launchSettingsPath}"); | ||
| Log.LogMessage(MessageImportance.High, $" Profiles: Operator, Install, Version, Help"); |
There was a problem hiding this comment.
Fix corrupted emoji character in log message.
Line 37 contains ? which appears to be a corrupted emoji character (likely intended to be ✅).
🐛 Fix the character
// Log success
Log.LogMessage(MessageImportance.High, "");
- Log.LogMessage(MessageImportance.High, $"? Generated launchSettings.json at: {launchSettingsPath}");
+ Log.LogMessage(MessageImportance.High, $"✅ Generated launchSettings.json at: {launchSettingsPath}");
Log.LogMessage(MessageImportance.High, $" Profiles: Operator, Install, Version, Help");📝 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.
| Log.LogMessage(MessageImportance.High, ""); | |
| Log.LogMessage(MessageImportance.High, $"? Generated launchSettings.json at: {launchSettingsPath}"); | |
| Log.LogMessage(MessageImportance.High, $" Profiles: Operator, Install, Version, Help"); | |
| Log.LogMessage(MessageImportance.High, ""); | |
| Log.LogMessage(MessageImportance.High, $"✅ Generated launchSettings.json at: {launchSettingsPath}"); | |
| Log.LogMessage(MessageImportance.High, $" Profiles: Operator, Install, Version, Help"); |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET.BuildTasks/GenerateLaunchSettingsTask.cs` around lines 36
- 38, Replace the corrupted emoji "?" in the log message with the intended
checkmark emoji (✅) in the Log.LogMessage call that writes $"? Generated
launchSettings.json at: {launchSettingsPath}"; locate the call inside the
GenerateLaunchSettingsTask class and update the string to $"✅ Generated
launchSettings.json at: {launchSettingsPath}" so the output shows the correct
emoji alongside the launchSettingsPath variable.
| "Install": { | ||
| "commandName": "Project", | ||
| "commandLineArgs": "install > install.yaml", | ||
| "environmentVariables": { |
There was a problem hiding this comment.
Shell redirection won’t work in generated launchSettings.
The task writes this into launchSettings, but dotnet run won’t interpret >; the file won’t be produced.
🔧 Suggested fix (avoid redirection in template)
- "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": { | |
| "Install": { | |
| "commandName": "Project", | |
| "commandLineArgs": "install", | |
| "environmentVariables": { |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET.BuildTasks/Templates/launchSettings.json.template` around
lines 11 - 14, The launchSettings profile "Install" currently uses shell
redirection in commandLineArgs ("install > install.yaml"), which dotnet run
won't interpret; update the "Install" profile (commandName "Project") to avoid
shell redirection by replacing the argument with a CLI-supported output flag
(e.g., "install --output install.yaml") or another method the install command
provides to write its output to a file, so the generated
launchSettings.json.template no longer relies on ">" for file creation.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/Commands/VersionCommand.cs`:
- Around line 18-25: The current VersionCommand code guards on
config.OperatorName and config.ContainerTag but then accesses
config.ContainerImage which can throw InvalidOperationException if
ContainerRepository is not set; update VersionCommand (the method that prints
operator/version info) to call config.Validate() before accessing ContainerImage
(or wrap the Console.WriteLine that references config.ContainerImage in a
try/catch that catches InvalidOperationException and prints a friendly message),
and remove the redundant ContainerTag null/whitespace check since ContainerTag
defaults to "latest"; reference the config.Validate() method and the properties
ContainerImage, ContainerRepository, and ContainerTag when making the change.
In `@src/K8sOperator.NET/Configuration/OperatorConfigurationProvider.cs`:
- Around line 71-82: ApplyConfiguration currently binds the entire "Operator"
section into the OperatorConfiguration instance with section.Bind(config), which
can overwrite existing assembly/default values with empty strings; change this
to selectively apply only non-empty values (e.g., use
configuration.GetSection("Operator").GetValue<string>("OperatorName") and set
config.OperatorName only when value is not null/empty) or perform post-bind
validation to restore defaults for any empty properties. Update the
ApplyConfiguration method to check each relevant property after reading the
section (or replace Bind with per-key GetValue calls) and only assign to
OperatorConfiguration fields when the config value is non-empty; keep references
to ApplyConfiguration, OperatorConfiguration,
configuration.GetSection("Operator") and section.Bind(config) to locate where to
modify.
In `@src/K8sOperator.NET/Operator.targets`:
- Around line 88-98: The GenerateDockerfile target can run before
$(_FullContainerRepository) and $(_FullContainerTag) are computed; update the
MSBuild ordering so GenerateDockerfile runs after/Adds a dependency on
AddDockerImageAttribute (the target that computes those properties) to guarantee
the properties are populated before invoking GenerateDockerfileTask.
Specifically, ensure the Target named "GenerateDockerfile" depends on or runs
after the "AddDockerImageAttribute" target so $(_FullContainerRepository) and
$(_FullContainerTag) are set when GenerateDockerfileTask executes.
In `@test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.cs`:
- Around line 124-128: The test captures an exception from Assert.That(() =>
config.ContainerImage).Throws<InvalidOperationException>() into the variable
exception but then dereferences exception.Message without guarding against a
null return from Throws<>; add a null check (e.g., Assert.NotNull(exception) or
use null-conditional access) before asserting on Message to satisfy the static
analyzer. Apply the same fix to the other test blocks that capture exceptions
(the other occurrences using the local variable exception at the ranges
corresponding to the ContainerImage/other property tests) so each assertion on
exception.Message is guarded.
♻️ Duplicate comments (3)
src/K8sOperator.NET.BuildTasks/GenerateDockerfileTask.cs (1)
39-59: Existing.dockerignoremay be overwritten.The task skips generation only if
Dockerfileexists, but always writes.dockerignorewhen creating a newDockerfile. This can wipe user customizations to.dockerignore.Consider checking each file independently:
🛠️ Suggested fix
- if (File.Exists(dockerfilePath)) + var dockerfileExists = File.Exists(dockerfilePath); + var dockerignoreExists = File.Exists(dockerignorePath); + + if (dockerfileExists) { Log.LogMessage(MessageImportance.Normal, $"Dockerfile already exists at {dockerfilePath}, skipping generation"); - return true; + if (dockerignoreExists) + return true; } // Extract .NET version from TargetFramework var dotnetVersion = ExtractDotNetVersion(TargetFramework); - // Read templates from embedded resources - var dockerfileContent = ReadEmbeddedResource("Dockerfile.template"); - var dockerignoreContent = ReadEmbeddedResource(".dockerignore.template"); - - // Replace placeholders - dockerfileContent = dockerfileContent - .Replace("{PROJECT_NAME}", ProjectName) - .Replace("{DOTNET_VERSION}", dotnetVersion); - - // Write files - File.WriteAllText(dockerfilePath, dockerfileContent); - File.WriteAllText(dockerignorePath, dockerignoreContent); + if (!dockerfileExists) + { + var dockerfileContent = ReadEmbeddedResource("Dockerfile.template") + .Replace("{PROJECT_NAME}", ProjectName) + .Replace("{DOTNET_VERSION}", dotnetVersion); + File.WriteAllText(dockerfilePath, dockerfileContent); + Log.LogMessage(MessageImportance.High, $"✅ Generated Dockerfile at: {dockerfilePath}"); + } + + if (!dockerignoreExists) + { + var dockerignoreContent = ReadEmbeddedResource(".dockerignore.template"); + File.WriteAllText(dockerignorePath, dockerignoreContent); + Log.LogMessage(MessageImportance.High, $"✅ Generated .dockerignore at: {dockerignorePath}"); + }src/K8sOperator.NET.BuildTasks/Templates/launchSettings.json.template (1)
11-18: Shell redirection incommandLineArgswon't work.The
> install.yamlredirection won't be interpreted bydotnet run; the output file won't be created. Consider using a CLI flag if theinstallcommand supports it (e.g.,--output install.yaml), or remove the redirection and let users handle output manually.🛠️ Suggested fix
"Install": { "commandName": "Project", - "commandLineArgs": "install > install.yaml", + "commandLineArgs": "install", "environmentVariables": {src/K8sOperator.NET.BuildTasks/GenerateLaunchSettingsTask.cs (1)
36-38: Fix corrupted emoji character in log message.This issue was previously identified - line 37 contains
?which should be✅.
🧹 Nitpick comments (11)
src/K8sOperator.NET.BuildTasks/K8sOperator.NET.BuildTasks.csproj (1)
11-14: Consider pinning exact versions for reproducible builds.Using wildcard versions (
17.12.*) for MSBuild packages can lead to non-reproducible builds. Consider pinning to exact versions:- <PackageReference Include="Microsoft.Build.Framework" Version="17.12.*" /> - <PackageReference Include="Microsoft.Build.Utilities.Core" Version="17.12.*" /> + <PackageReference Include="Microsoft.Build.Framework" Version="17.12.0" /> + <PackageReference Include="Microsoft.Build.Utilities.Core" Version="17.12.0" />test/K8sOperator.NET.Tests/EventWatcher_Tests.cs (1)
34-107: Consider extracting a helper method for test configuration.Multiple tests instantiate
new OperatorConfiguration(). For consistency withEventWatcherDatasource_Tests.CreateConfiguration()and easier maintenance if defaults need to change:private static OperatorConfiguration CreateConfiguration() => new();Then use
CreateConfiguration()in each test. This is optional since the current approach works correctly.src/K8sOperator.NET/Commands/InstallCommand.cs (1)
95-151: Consider adding config validation before generating manifests.If
OperatorConfigurationhas invalid values (emptyOperatorName, missingContainerImagecomponents), the generated manifests will be invalid. Consider callingconfig.Validate()at the start ofRunAsyncto fail fast with a clear error message rather than generating broken YAML.public async Task RunAsync(string[] args) { var dataSource = app.Services.GetRequiredService<EventWatcherDatasource>(); var config = app.Services.GetRequiredService<OperatorConfiguration>(); + config.Validate(); var watchers = dataSource.GetWatchers().ToList();test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.cs (1)
274-289: Async test method without await.This test is marked
asyncbut doesn't await anything. Either remove theasynckeyword or add an assertion that usesawait.♻️ Suggested fix
[Test] - public async Task Validate_Should_NotThrow_WhenAllPropertiesAreValid() + public Task Validate_Should_NotThrow_WhenAllPropertiesAreValid() { // Arrange var config = new OperatorConfiguration { OperatorName = "my-operator", Namespace = "default", ContainerRegistry = "ghcr.io", ContainerRepository = "myorg/myapp", ContainerTag = "1.0.0" }; // Act & Assert - Should not throw config.Validate(); + return Task.CompletedTask; }src/K8sOperator.NET.BuildTasks/GenerateLaunchSettingsTask.cs (1)
50-59: Consider extracting sharedReadEmbeddedResourcehelper to reduce duplication.This method is identical to the one in
GenerateDockerfileTask.cs. Consider extracting it to a shared utility class to follow DRY principles.♻️ Example extraction
// In a new file: EmbeddedResourceHelper.cs internal static class EmbeddedResourceHelper { public static string ReadEmbeddedResource(Type anchorType, string resourceName) { var assembly = anchorType.Assembly; var fullResourceName = $"K8sOperator.NET.BuildTasks.Templates.{resourceName}"; using var stream = assembly.GetManifestResourceStream(fullResourceName) ?? throw new InvalidOperationException($"Could not find embedded resource: {fullResourceName}"); using var reader = new StreamReader(stream); return reader.ReadToEnd(); } }src/K8sOperator.NET/Builder/ControllerBuilder.cs (1)
34-37: Remove trailing blank lines.Minor formatting issue - there are extra blank lines at the end of the file.
src/K8sOperator.NET/K8sOperator.NET.csproj (1)
58-65: Consider using the official NuGet pack extension pointTargetsForTfmSpecificContentInPackageinstead of a custom target.While the
PackBuildTaskstarget withAfterTargets="Build"should work correctly (since it runs before the pack phase and the Pack metadata is evaluated duringGenerateNuspec), the recommended approach for including additional files in a NuGet package is to use the official extension pointTargetsForTfmSpecificContentInPackage. This provides better integration with NuGet's build process and is more maintainable.src/K8sOperator.NET/EventWatcherDatasource.cs (1)
62-63: Remove trailing blank lines.Unnecessary trailing blank lines at end of file.
test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.cs (2)
33-47: Consider renaming local variable for clarity.The local variable
metadataholds anOperatorConfigurationinstance. Renaming it toconfigurationwould improve readability and align with the property name being asserted.Suggested variable rename
public async Task Constructor_Should_InitializeWithServiceProviderAndMetadata() { // Arrange var serviceProvider = CreateServiceProvider(); - var metadata = CreateConfiguration(); + var configuration = CreateConfiguration(); // Act - var datasource = new EventWatcherDatasource(serviceProvider, metadata); + var datasource = new EventWatcherDatasource(serviceProvider, configuration); // Assert await Assert.That(datasource).IsNotNull(); await Assert.That(datasource.ServiceProvider).IsEqualTo(serviceProvider); - await Assert.That(datasource.Configuration).IsEqualTo(metadata); + await Assert.That(datasource.Configuration).IsEqualTo(configuration); }
272-282: Test name no longer matches what it tests.The test
Metadata_Should_BeAccessiblePropertynow testsConfigurationaccessibility. Consider renaming for clarity.Suggested rename
[Test] - public async Task Metadata_Should_BeAccessibleProperty() + public async Task Configuration_Should_BeAccessibleProperty() {src/K8sOperator.NET/Operator.targets (1)
100-103: Remove trailing blank lines.Unnecessary trailing blank lines at end of file.
| if (string.IsNullOrWhiteSpace(config.OperatorName) || string.IsNullOrWhiteSpace(config.ContainerTag)) | ||
| { | ||
| Console.WriteLine("Operator name or version metadata is missing."); | ||
| return Task.CompletedTask; | ||
| } | ||
|
|
||
| Console.WriteLine($"{name} version {version}."); | ||
| Console.WriteLine($"Docker Info: {version.GetImage()}."); | ||
| Console.WriteLine($"{config.OperatorName} version {config.ContainerTag}."); | ||
| Console.WriteLine($"Docker Info: {config.ContainerImage}."); |
There was a problem hiding this comment.
Accessing ContainerImage may throw if ContainerRepository is not configured.
config.ContainerTag will never be null/whitespace (defaults to "latest"), so the check at line 18 is partially redundant. However, accessing config.ContainerImage at line 25 will throw InvalidOperationException if ContainerRepository is empty.
Consider either:
- Calling
config.Validate()before accessing properties, or - Wrapping in try-catch with a user-friendly message
🛠️ Suggested fix
public Task RunAsync(string[] args)
{
var config = app.Services.GetRequiredService<OperatorConfiguration>();
- if (string.IsNullOrWhiteSpace(config.OperatorName) || string.IsNullOrWhiteSpace(config.ContainerTag))
+ if (string.IsNullOrWhiteSpace(config.OperatorName))
{
- Console.WriteLine("Operator name or version metadata is missing.");
+ Console.WriteLine("Operator name metadata is missing.");
return Task.CompletedTask;
}
Console.WriteLine($"{config.OperatorName} version {config.ContainerTag}.");
- Console.WriteLine($"Docker Info: {config.ContainerImage}.");
+ try
+ {
+ Console.WriteLine($"Docker Info: {config.ContainerImage}.");
+ }
+ catch (InvalidOperationException)
+ {
+ Console.WriteLine("Docker Info: Not fully configured.");
+ }
return Task.CompletedTask;
}🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Commands/VersionCommand.cs` around lines 18 - 25, The
current VersionCommand code guards on config.OperatorName and
config.ContainerTag but then accesses config.ContainerImage which can throw
InvalidOperationException if ContainerRepository is not set; update
VersionCommand (the method that prints operator/version info) to call
config.Validate() before accessing ContainerImage (or wrap the Console.WriteLine
that references config.ContainerImage in a try/catch that catches
InvalidOperationException and prints a friendly message), and remove the
redundant ContainerTag null/whitespace check since ContainerTag defaults to
"latest"; reference the config.Validate() method and the properties
ContainerImage, ContainerRepository, and ContainerTag when making the change.
| private void ApplyConfiguration(OperatorConfiguration config) | ||
| { | ||
| if (configuration == null) | ||
| return; | ||
|
|
||
| // Bind from appsettings.json section: "Operator" | ||
| var section = configuration.GetSection("Operator"); | ||
| if (section.Exists()) | ||
| { | ||
| section.Bind(config); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential issue: Bind() may overwrite values with empty strings from config.
If the "Operator" section exists in appsettings.json with empty string values (e.g., "OperatorName": ""), Bind() will overwrite the assembly attribute values with empty strings. Consider using a more selective binding approach or adding validation after binding.
Alternative approach using individual value checks
private void ApplyConfiguration(OperatorConfiguration config)
{
if (configuration == null)
return;
// Bind from appsettings.json section: "Operator"
var section = configuration.GetSection("Operator");
if (section.Exists())
{
- section.Bind(config);
+ var operatorName = section["OperatorName"];
+ if (!string.IsNullOrEmpty(operatorName))
+ config.OperatorName = operatorName;
+
+ var ns = section["Namespace"];
+ if (!string.IsNullOrEmpty(ns))
+ config.Namespace = ns;
+
+ var registry = section["ContainerRegistry"];
+ if (!string.IsNullOrEmpty(registry))
+ config.ContainerRegistry = registry;
+
+ var repository = section["ContainerRepository"];
+ if (!string.IsNullOrEmpty(repository))
+ config.ContainerRepository = repository;
+
+ var tag = section["ContainerTag"];
+ if (!string.IsNullOrEmpty(tag))
+ config.ContainerTag = 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.
| private void ApplyConfiguration(OperatorConfiguration config) | |
| { | |
| if (configuration == null) | |
| return; | |
| // Bind from appsettings.json section: "Operator" | |
| var section = configuration.GetSection("Operator"); | |
| if (section.Exists()) | |
| { | |
| section.Bind(config); | |
| } | |
| } | |
| private void ApplyConfiguration(OperatorConfiguration config) | |
| { | |
| if (configuration == null) | |
| return; | |
| // Bind from appsettings.json section: "Operator" | |
| var section = configuration.GetSection("Operator"); | |
| if (section.Exists()) | |
| { | |
| var operatorName = section["OperatorName"]; | |
| if (!string.IsNullOrEmpty(operatorName)) | |
| config.OperatorName = operatorName; | |
| var ns = section["Namespace"]; | |
| if (!string.IsNullOrEmpty(ns)) | |
| config.Namespace = ns; | |
| var registry = section["ContainerRegistry"]; | |
| if (!string.IsNullOrEmpty(registry)) | |
| config.ContainerRegistry = registry; | |
| var repository = section["ContainerRepository"]; | |
| if (!string.IsNullOrEmpty(repository)) | |
| config.ContainerRepository = repository; | |
| var tag = section["ContainerTag"]; | |
| if (!string.IsNullOrEmpty(tag)) | |
| config.ContainerTag = tag; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/K8sOperator.NET/Configuration/OperatorConfigurationProvider.cs` around
lines 71 - 82, ApplyConfiguration currently binds the entire "Operator" section
into the OperatorConfiguration instance with section.Bind(config), which can
overwrite existing assembly/default values with empty strings; change this to
selectively apply only non-empty values (e.g., use
configuration.GetSection("Operator").GetValue<string>("OperatorName") and set
config.OperatorName only when value is not null/empty) or perform post-bind
validation to restore defaults for any empty properties. Update the
ApplyConfiguration method to check each relevant property after reading the
section (or replace Bind with per-key GetValue calls) and only assign to
OperatorConfiguration fields when the config value is non-empty; keep references
to ApplyConfiguration, OperatorConfiguration,
configuration.GetSection("Operator") and section.Bind(config) to locate where to
modify.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.cs`:
- Around line 283-297: The test method
Validate_Should_NotThrow_WhenAllPropertiesAreValid is declared async but
contains no awaits; remove the unnecessary async and change the signature to a
synchronous test (e.g., "public void
Validate_Should_NotThrow_WhenAllPropertiesAreValid()") in the
OperatorConfiguration_Tests class so you no longer get CS1998 warnings.
♻️ Duplicate comments (3)
src/K8sOperator.NET/Commands/VersionCommand.cs (1)
16-25: GuardContainerImageaccess or validate config first.
ContainerImagecan throw ifContainerRegistry/ContainerRepositoryis empty, and theContainerTagcheck is redundant because it defaults to"latest". Consider callingconfig.Validate()(or catchingInvalidOperationException) before printing to avoid unexpected crashes.src/K8sOperator.NET/Configuration/OperatorConfigurationProvider.cs (1)
71-81:Bind()can clobber defaults with empty strings.If
appsettings.jsoncontains empty values,Bindoverwrites assembly/default values. Consider applying only non-empty values.🛠️ Suggested selective binding
- if (section.Exists()) - { - section.Bind(config); - } + if (section.Exists()) + { + var operatorName = section["OperatorName"]; + if (!string.IsNullOrWhiteSpace(operatorName)) + config.OperatorName = operatorName; + + var ns = section["Namespace"]; + if (!string.IsNullOrWhiteSpace(ns)) + config.Namespace = ns; + + var registry = section["ContainerRegistry"]; + if (!string.IsNullOrWhiteSpace(registry)) + config.ContainerRegistry = registry; + + var repository = section["ContainerRepository"]; + if (!string.IsNullOrWhiteSpace(repository)) + config.ContainerRepository = repository; + + var tag = section["ContainerTag"]; + if (!string.IsNullOrWhiteSpace(tag)) + config.ContainerTag = tag; + }src/K8sOperator.NET/Operator.targets (1)
88-97: GenerateDockerfile may run before image properties are computed.
_FullContainerRepository/_FullContainerTagare built inAddDockerImageAttribute, butGenerateDockerfileruns beforeBuild, so the task can see empty values. EnsureGenerateDockerfiledepends onAddDockerImageAttribute.🛠️ Add dependency to guarantee ordering
- <Target Name="GenerateDockerfile" BeforeTargets="Build" Condition=" '$(GenerateOperatorDockerfile)' == 'true' AND !Exists('$(MSBuildProjectDirectory)\Dockerfile') "> + <Target Name="GenerateDockerfile" BeforeTargets="Build" DependsOnTargets="AddDockerImageAttribute" Condition=" '$(GenerateOperatorDockerfile)' == 'true' AND !Exists('$(MSBuildProjectDirectory)\Dockerfile') ">
🧹 Nitpick comments (5)
src/K8sOperator.NET.BuildTasks/GenerateLaunchSettingsTask.cs (1)
50-59: Consider extracting sharedReadEmbeddedResourcehelper.This method is duplicated in
GenerateDockerfileTask.cs(lines 100-109). Consider extracting it to a shared utility class to reduce duplication.♻️ Suggested extraction
// In a new file: EmbeddedResourceHelper.cs namespace K8sOperator.NET.BuildTasks; internal static class EmbeddedResourceHelper { public static string ReadEmbeddedResource(string resourceName) { var assembly = typeof(EmbeddedResourceHelper).Assembly; var fullResourceName = $"K8sOperator.NET.BuildTasks.Templates.{resourceName}"; using var stream = assembly.GetManifestResourceStream(fullResourceName) ?? throw new InvalidOperationException($"Could not find embedded resource: {fullResourceName}"); using var reader = new StreamReader(stream); return reader.ReadToEnd(); } }src/K8sOperator.NET.BuildTasks/GenerateDockerfileTask.cs (1)
66-76: Consider logging build instructions only when files are generated.The operator info and docker build/push instructions are logged on every build, even when both files already exist. This could create unnecessary noise in build output. Consider making this logging conditional.
♻️ Suggested improvement
+ var anyFileGenerated = false; + if (!File.Exists(dockerfilePath)) { File.WriteAllText(dockerfilePath, dockerfileContent); - Log.LogMessage(MessageImportance.High, $"Generated Dockerfile at: {dockerfilePath}"); - + anyFileGenerated = true; } if (!File.Exists(dockerignorePath)) { File.WriteAllText(dockerignorePath, dockerignoreContent); - Log.LogMessage(MessageImportance.High, $"Generated .dockerignore at: {dockerignorePath}"); + anyFileGenerated = true; } - // Log success - Log.LogMessage(MessageImportance.High, $"Operator: {OperatorName}"); - Log.LogMessage(MessageImportance.High, $" .NET Version: {dotnetVersion}"); - Log.LogMessage(MessageImportance.High, $" Image: {ContainerRegistry}/{ContainerRepository}:{ContainerTag}"); - Log.LogMessage(MessageImportance.High, ""); - Log.LogMessage(MessageImportance.High, "To build the image:"); - Log.LogMessage(MessageImportance.High, $" docker build -t {ContainerRegistry}/{ContainerRepository}:{ContainerTag} ."); - Log.LogMessage(MessageImportance.High, ""); - Log.LogMessage(MessageImportance.High, "To push the image:"); - Log.LogMessage(MessageImportance.High, $" docker push {ContainerRegistry}/{ContainerRepository}:{ContainerTag}"); - Log.LogMessage(MessageImportance.High, ""); + if (anyFileGenerated) + { + Log.LogMessage(MessageImportance.High, $"Operator: {OperatorName}"); + Log.LogMessage(MessageImportance.High, $" .NET Version: {dotnetVersion}"); + Log.LogMessage(MessageImportance.High, $" Image: {ContainerRegistry}/{ContainerRepository}:{ContainerTag}"); + Log.LogMessage(MessageImportance.High, ""); + Log.LogMessage(MessageImportance.High, "To build the image:"); + Log.LogMessage(MessageImportance.High, $" docker build -t {ContainerRegistry}/{ContainerRepository}:{ContainerTag} ."); + Log.LogMessage(MessageImportance.High, ""); + Log.LogMessage(MessageImportance.High, "To push the image:"); + Log.LogMessage(MessageImportance.High, $" docker push {ContainerRegistry}/{ContainerRepository}:{ContainerTag}"); + Log.LogMessage(MessageImportance.High, ""); + }test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.cs (1)
33-47: Renamemetadatalocals toconfigurationfor clarity.These variables now hold
OperatorConfiguration, so the current name is misleading. Consider renaming across the test file for readability.src/K8sOperator.NET/Commands/InstallCommand.cs (1)
18-27: Validate configuration before manifest generation.
CreateDeploymentaccessesconfig.ContainerImage, which throws if registry/repository are missing. Consider validating up-front and printing a friendly message before proceeding.♻️ Suggested change
public async Task RunAsync(string[] args) { var dataSource = app.Services.GetRequiredService<EventWatcherDatasource>(); var config = app.Services.GetRequiredService<OperatorConfiguration>(); + try + { + config.Validate(); + } + catch (InvalidOperationException ex) + { + Console.WriteLine(ex.Message); + return; + } var watchers = dataSource.GetWatchers().ToList(); var ns = CreateNamespace(config); var clusterrole = CreateClusterRole(config, watchers); var clusterrolebinding = CreateClusterRoleBinding(config); var deployment = CreateDeployment(config);src/K8sOperator.NET/OperatorExtensions.cs (1)
24-47: Consider validating OperatorConfiguration before returning.Fail fast on missing required settings instead of deferring to later runtime paths.
♻️ Proposed tweak
- return config; + config.Validate(); + return config;
| public async Task Validate_Should_NotThrow_WhenAllPropertiesAreValid() | ||
| { | ||
| // Arrange | ||
| var config = new OperatorConfiguration | ||
| { | ||
| OperatorName = "my-operator", | ||
| Namespace = "default", | ||
| ContainerRegistry = "ghcr.io", | ||
| ContainerRepository = "myorg/myapp", | ||
| ContainerTag = "1.0.0" | ||
| }; | ||
|
|
||
| // Act & Assert - Should not throw | ||
| config.Validate(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the test file to confirm the code
find . -name "OperatorConfiguration_Tests.cs" -type fRepository: pmdevers/K8sOperator.NET
Length of output: 127
🏁 Script executed:
# Search for project files and warning configuration
fd -e csproj -o fd -e editorconfig | head -20Repository: pmdevers/K8sOperator.NET
Length of output: 191
🏁 Script executed:
# Check for warning configuration in csproj files
rg -n "TreatWarningsAsErrors|WarningsAsErrors|CS1998" -g "*.csproj" -g "*.editorconfig"Repository: pmdevers/K8sOperator.NET
Length of output: 50
🏁 Script executed:
# Read the actual test file to confirm the code
cat -n ./test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.cs | sed -n '280,300p'Repository: pmdevers/K8sOperator.NET
Length of output: 775
🏁 Script executed:
# Find csproj files
find . -name "*.csproj" -type fRepository: pmdevers/K8sOperator.NET
Length of output: 357
🏁 Script executed:
# Search for warning configuration in all csproj and editorconfig files
find . \( -name "*.csproj" -o -name ".editorconfig" \) -type f -exec grep -l "TreatWarningsAsErrors\|WarningsAsErrors\|CS1998" {} \;Repository: pmdevers/K8sOperator.NET
Length of output: 50
🏁 Script executed:
# Read the test project csproj to check warning settings
cat ./test/K8sOperator.NET.Tests/K8sOperator.NET.Tests.csprojRepository: pmdevers/K8sOperator.NET
Length of output: 684
🏁 Script executed:
# Check for .editorconfig files
find . -name ".editorconfig" -type fRepository: pmdevers/K8sOperator.NET
Length of output: 83
🏁 Script executed:
# Check how many other async Task tests exist in this file without await
rg -n "public async Task" ./test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.cs -A 20 | grep -E "public async Task|await|^\s*\}" | head -50Repository: pmdevers/K8sOperator.NET
Length of output: 3823
🏁 Script executed:
# Check .editorconfig for CS1998 or warning settings
cat ./.editorconfigRepository: pmdevers/K8sOperator.NET
Length of output: 12941
🏁 Script executed:
# Check the line 300 test to see if it also has await
sed -n '300,320p' ./test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.csRepository: pmdevers/K8sOperator.NET
Length of output: 690
Remove unnecessary async from test method.
This test has async Task but no await, which produces CS1998 warnings. Since the test only calls synchronous methods, remove async and the method signature to Task or change to void. The suggested fix below removes the async keyword entirely, which is the cleanest approach.
✅ Suggested fix
- public async Task Validate_Should_NotThrow_WhenAllPropertiesAreValid()
+ public void Validate_Should_NotThrow_WhenAllPropertiesAreValid()
{
// Arrange
var config = new OperatorConfiguration
{
OperatorName = "my-operator",
Namespace = "default",
ContainerRegistry = "ghcr.io",
ContainerRepository = "myorg/myapp",
ContainerTag = "1.0.0"
};
// Act & Assert - Should not throw
config.Validate();
}🤖 Prompt for AI Agents
In `@test/K8sOperator.NET.Tests/OperatorConfiguration_Tests.cs` around lines 283 -
297, The test method Validate_Should_NotThrow_WhenAllPropertiesAreValid is
declared async but contains no awaits; remove the unnecessary async and change
the signature to a synchronous test (e.g., "public void
Validate_Should_NotThrow_WhenAllPropertiesAreValid()") in the
OperatorConfiguration_Tests class so you no longer get CS1998 warnings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/K8sOperator.NET/K8sOperator.NET.csproj`:
- Around line 53-64: The PackBuildTasks target currently only includes
K8sOperator.NET.BuildTasks.dll, so change the _BuildTasksFiles item to include
all outputs from the BuildTasks bin folder (e.g., use a wildcard like
Include="..\K8sOperator.NET.BuildTasks\bin\$(Configuration)\netstandard2.0\**\*.*"
or otherwise enumerate all *.dll and related files) and then pack that item list
to both PackagePath="build\netstandard2.0" and "buildTransitive\netstandard2.0"
(keep the existing None Include lines but reference @(_BuildTasksFiles) so
Microsoft.Build.Framework.dll, Microsoft.Build.Utilities.Core.dll and any other
outputs are included alongside K8sOperator.NET.BuildTasks.dll). Ensure the
changes are made inside the Target named PackBuildTasks and update the
_BuildTasksFiles item and subsequent None entries accordingly.
♻️ Duplicate comments (4)
src/K8sOperator.NET.BuildTasks/Templates/launchSettings.json.template (1)
11-14: Remove shell redirection from the Install profile.
dotnet runwon’t interpret>in launchSettings, soinstall.yamlwon’t be created at Line 13. Use a CLI output flag (if supported) or keep redirection as a manual shell step.🔧 Suggested fix
- "commandLineArgs": "install > install.yaml", + "commandLineArgs": "install",examples/SimpleOperator/Properties/launchSettings.json (1)
11-14: Remove shell redirection from the Install profile.
dotnet rundoesn’t interpret>in launchSettings, soinstall.yamlwon’t be created at Line 13. Prefer an explicit output flag or keep redirection as a manual shell step.🔧 Suggested fix
- "commandLineArgs": "install > install.yaml", + "commandLineArgs": "install",src/K8sOperator.NET/Configuration/OperatorConfigurationProvider.cs (1)
71-80: AvoidBind()overwriting attribute defaults with empty strings.If the "Operator" section contains empty values,
Bind()will overwrite non-empty assembly-derived defaults at Line 80. Prefer selective assignment for non-empty values.🔧 Suggested fix (selective binding)
private void ApplyConfiguration(OperatorConfiguration config) { if (configuration == null) return; // Bind from appsettings.json section: "Operator" var section = configuration.GetSection("Operator"); if (section.Exists()) { - section.Bind(config); + var operatorName = section["OperatorName"]; + if (!string.IsNullOrEmpty(operatorName)) + config.OperatorName = operatorName; + + var ns = section["Namespace"]; + if (!string.IsNullOrEmpty(ns)) + config.Namespace = ns; + + var registry = section["ContainerRegistry"]; + if (!string.IsNullOrEmpty(registry)) + config.ContainerRegistry = registry; + + var repository = section["ContainerRepository"]; + if (!string.IsNullOrEmpty(repository)) + config.ContainerRepository = repository; + + var tag = section["ContainerTag"]; + if (!string.IsNullOrEmpty(tag)) + config.ContainerTag = tag; } }In Microsoft.Extensions.Configuration.Binder, does section.Bind(...) overwrite existing property values with empty strings when the configuration value is an empty string?src/K8sOperator.NET/Commands/VersionCommand.cs (1)
16-25: GuardContainerImageaccess to avoid runtime failure.Line 18’s
ContainerTagcheck is redundant because it defaults to"latest", while Line 25 can throwInvalidOperationExceptionifContainerRegistry/ContainerRepositoryaren’t configured. Consider validating or handling the exception so the command doesn’t crash.🛠️ Suggested fix
- if (string.IsNullOrWhiteSpace(config.OperatorName) || string.IsNullOrWhiteSpace(config.ContainerTag)) + if (string.IsNullOrWhiteSpace(config.OperatorName)) { Console.WriteLine("Operator name or version metadata is missing."); return Task.CompletedTask; } Console.WriteLine($"{config.OperatorName} version {config.ContainerTag}."); - Console.WriteLine($"Docker Info: {config.ContainerImage}."); + try + { + Console.WriteLine($"Docker Info: {config.ContainerImage}."); + } + catch (InvalidOperationException) + { + Console.WriteLine("Docker Info: Not fully configured."); + }
🧹 Nitpick comments (4)
test/K8sOperator.NET.Tests/EventWatcherDatasource_Tests.cs (1)
33-47: Consider renamingmetadatalocals toconfigurationfor clarity.Those locals now hold
OperatorConfiguration, so the current name is misleading and easy to misread when scanning tests.✏️ Example rename (apply consistently)
- var metadata = CreateConfiguration(); + var configuration = CreateConfiguration(); - var datasource = new EventWatcherDatasource(serviceProvider, metadata); + var datasource = new EventWatcherDatasource(serviceProvider, configuration); - await Assert.That(datasource.Configuration).IsEqualTo(metadata); + await Assert.That(datasource.Configuration).IsEqualTo(configuration);src/K8sOperator.NET/Operator.targets (1)
3-11: Avoid hard‑coding Debug/Release in task assembly path.Custom configurations (e.g.,
CI,ReleaseSigned) won’t resolve the task DLL. Consider using$(Configuration)for a single, resilient lookup.♻️ Proposed refactor
- <_BuildTasksPath Condition="'$(_BuildTasksPath)' == '' AND Exists('$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Debug\netstandard2.0\K8sOperator.NET.BuildTasks.dll')">$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Debug\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath> - <_BuildTasksPath Condition="'$(_BuildTasksPath)' == '' AND Exists('$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Release\netstandard2.0\K8sOperator.NET.BuildTasks.dll')">$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\Release\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath> + <_BuildTasksPath Condition="'$(_BuildTasksPath)' == '' AND Exists('$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\$(Configuration)\netstandard2.0\K8sOperator.NET.BuildTasks.dll')">$(MSBuildThisFileDirectory)..\K8sOperator.NET.BuildTasks\bin\$(Configuration)\netstandard2.0\K8sOperator.NET.BuildTasks.dll</_BuildTasksPath>src/K8sOperator.NET/OperatorExtensions.cs (1)
24-47: Consider validating OperatorConfiguration after overrides.Fail-fast here prevents late runtime surprises if required fields are missing or malformed.
♻️ Proposed refinement
if (builder.OperatorConfiguration != null) { if (!string.IsNullOrEmpty(builder.OperatorConfiguration.OperatorName)) config.OperatorName = builder.OperatorConfiguration.OperatorName; if (!string.IsNullOrEmpty(builder.OperatorConfiguration.Namespace)) config.Namespace = builder.OperatorConfiguration.Namespace; if (!string.IsNullOrEmpty(builder.OperatorConfiguration.ContainerRegistry)) config.ContainerRegistry = builder.OperatorConfiguration.ContainerRegistry; if (!string.IsNullOrEmpty(builder.OperatorConfiguration.ContainerRepository)) config.ContainerRepository = builder.OperatorConfiguration.ContainerRepository; if (!string.IsNullOrEmpty(builder.OperatorConfiguration.ContainerTag)) config.ContainerTag = builder.OperatorConfiguration.ContainerTag; } + config.Validate(); return config;examples/SimpleOperator/Program.cs (1)
7-10: Leader election enabled—ensure Lease RBAC is granted.The example will now require
coordination.k8s.io/leasescreate/update/get in the operator namespace; otherwise startup can fail.
Moves code generation logic to MSBuild tasks, which improves the build process and allows for more flexibility in customizing the generated output. Removes the commands for generating launch settings and Dockerfile, replacing them with MSBuild tasks that automatically generate these files during the build process. Adds a configuration provider to centralize and prioritize operator configuration from assembly attributes, appsettings.json, and the OperatorBuilder, enabling easier customization. Updates EventWatcher and commands to use the new OperatorConfiguration for settings like operator name, image, and namespace. Adds new build tasks project and templates. Fixes #2
1 participant
Moves code generation logic to MSBuild tasks, which improves the build process and allows for more flexibility in customizing the generated output.
Removes the commands for generating launch settings and Dockerfile, replacing them with MSBuild tasks that automatically generate these files during the build process.
Adds a configuration provider to centralize and prioritize operator configuration from assembly attributes, appsettings.json, and the OperatorBuilder, enabling easier customization.
Updates EventWatcher and commands to use the new OperatorConfiguration for settings like operator name, image, and namespace.
Adds new build tasks project and templates.
Fixes #2
Summary by CodeRabbit
New Features
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.