Conversation
Bumps the default Kubernetes API version to 1.32 in CreateClusterCommand. Hosts.toml is now written to a temporary file to ensure accessibility for Docker, replacing the previous static path usage.
Introduces a WaitForKustomizations method to FluxClient and IFluxClient, and calls it in CreateClusterCommand after bootstrapping Flux. This ensures all Flux kustomizations are reconciled before proceeding with reverse proxy configuration, improving cluster setup reliability.
There was a problem hiding this comment.
Pull request overview
This PR updates the Kubernetes cluster creation process by upgrading the default Kubernetes version from 1.29 to 1.32, relocating the containerd hosts.toml configuration to a temporary file for improved accessibility, and introducing a waiting mechanism to ensure Flux kustomizations are ready before proceeding with cluster configuration.
Changes:
- Updated default Kubernetes API version from 1.29 to 1.32
- Moved hosts.toml file generation from static ConfigMounts directory to a dynamically created temporary file
- Added WaitForKustomizations method to FluxClient to poll and verify Flux kustomization reconciliation status before continuing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cli/src/Vdk/Commands/CreateClusterCommand.cs | Updated default Kubernetes version to 1.32; changed hosts.toml to be written to temp file; added call to WaitForKustomizations after Flux bootstrap |
| cli/src/Vdk/Services/FluxClient.cs | Added WaitForKustomizations method to poll Flux API and verify all kustomizations are ready; added System.Text.Json import |
| cli/src/Vdk/Services/IFluxClient.cs | Added WaitForKustomizations method signature to interface |
| .claude/settings.local.json | Added additional Bash command permissions for Docker, Kubernetes, and shell scripting operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public bool WaitForKustomizations(string clusterName, int maxAttempts = 60, int delaySeconds = 5) | ||
| { |
There was a problem hiding this comment.
This synchronous method uses Thread.Sleep which blocks the calling thread. Since it's called from InvokeAsync (an async method at line 209), this creates a blocking call in an async context. Consider making this method async and using Task.Delay instead of Thread.Sleep. This would improve responsiveness and follow async/await best practices. The pattern should be: public async Task<bool> WaitForKustomizations(...) with await Task.Delay(delaySeconds * 1000) instead of Thread.Sleep.
| public interface IFluxClient | ||
| { | ||
| void Bootstrap(string clusterName, string path, string branch = FluxClient.DefaultBranch); | ||
| bool WaitForKustomizations(string clusterName, int maxAttempts = 60, int delaySeconds = 5); |
There was a problem hiding this comment.
The interface should be updated to return Task<bool> instead of bool to properly support async operations. Since this method performs network I/O and waits (currently with Thread.Sleep), it should be async to avoid blocking the calling thread.
| var name = item.GetProperty("metadata").GetProperty("name").GetString(); | ||
| bool isReady = false; | ||
|
|
||
| if (item.TryGetProperty("status", out var status) && | ||
| status.TryGetProperty("conditions", out var conditions)) | ||
| { | ||
| foreach (var condition in conditions.EnumerateArray()) | ||
| { | ||
| if (condition.GetProperty("type").GetString() == "Ready") | ||
| { | ||
| var condStatus = condition.GetProperty("status").GetString(); |
There was a problem hiding this comment.
The GetString() calls on lines 151, 159, and 161 could potentially return null if the JSON property values are null. This could lead to NullReferenceException when these values are used. Consider using null-conditional operators or null-coalescing operators to handle potential null values, e.g., GetString() ?? "unknown" for name comparisons.
| var workers = new Option<int>("--Workers") { DefaultValueFactory = _ => Defaults.WorkerNodes, Description = "The number of worker nodes in the cluster." }; | ||
| workers.Aliases.Add("-w"); | ||
| var kubeVersion = new Option<string>("--KubeVersion") { DefaultValueFactory = _ => "1.29", Description = "The kubernetes api version." }; | ||
| var kubeVersion = new Option<string>("--KubeVersion") { DefaultValueFactory = _ => "1.32", Description = "The kubernetes api version." }; |
There was a problem hiding this comment.
The documentation still references Kubernetes version 1.29 as the default, but the code has been updated to use 1.32. The documentation files docs/usage/command-reference.md (line 39) and docs/usage/creating-clusters.md (line 31) should be updated to reflect the new default version of 1.32.
| [host."http://host.docker.internal:5000"] | ||
| capabilities = ["pull", "resolve"] | ||
| """; | ||
| var hostsTomlPath = _fileSystem.Path.Combine(_fileSystem.Path.GetTempPath(), $"hosts-{Guid.NewGuid()}.toml"); |
There was a problem hiding this comment.
The temporary hosts.toml file created at this location is never cleaned up after the cluster is created. This will lead to accumulation of orphaned files in the temp directory over time. Consider using a try-finally block or ensuring the file is deleted after the Kind cluster creation completes, or use a deterministic naming scheme that allows reuse of the same file.
| var hostsTomlPath = _fileSystem.Path.Combine(_fileSystem.Path.GetTempPath(), $"hosts-{Guid.NewGuid()}.toml"); | |
| var hostsTomlPath = _fileSystem.Path.Combine(_fileSystem.Path.GetTempPath(), "vdk-hosts-hub.dev-k8s.cloud.toml"); |
| foreach (var condition in conditions.EnumerateArray()) | ||
| { | ||
| if (condition.GetProperty("type").GetString() == "Ready") | ||
| { | ||
| var condStatus = condition.GetProperty("status").GetString(); | ||
| if (condStatus == "True") | ||
| { | ||
| isReady = true; | ||
| } | ||
| else if (attempt % 5 == 0) | ||
| { | ||
| var reason = condition.TryGetProperty("reason", out var r) | ||
| ? r.GetString() : "Unknown"; | ||
| var message = condition.TryGetProperty("message", out var m) | ||
| ? m.GetString() : ""; | ||
| _console.WriteLine( | ||
| $" Kustomization '{name}' not ready: {reason} - {message}"); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
1 participant
This pull request introduces improvements to the Kubernetes cluster creation process and enhances the robustness of the Flux deployment workflow. The main changes include updating the default Kubernetes version, ensuring proper configuration file handling for containerd, and adding a mechanism to wait for all Flux kustomizations to be ready before proceeding.
Cluster creation and configuration improvements:
CreateClusterCommandhas been updated from1.29to1.32to ensure clusters are created with a more recent version.hosts.tomlfile for containerd registry configuration is now written to a temporary file location, ensuring Docker can reliably access it regardless of the working directory. All references to this file in the cluster setup process have been updated to use the temporary path. [1] [2] [3]Flux deployment reliability:
WaitForKustomizations, has been added to theFluxClientservice. This method polls the Flux API to ensure all kustomizations are reconciled and ready before proceeding, improving the reliability of post-deployment configuration steps. TheCreateClusterCommandnow calls this method after bootstrapping Flux. [1] [2] [3]Development tooling:
.claude/settings.local.jsonfile has been updated to allow additional Bash commands related to Docker, Kubernetes, and shell scripting, supporting more flexible local development and automation.