WIP: feat(cli): OpenFrame cluster commands to apply public EKS/GKE TF modules (K3d alternative) - #223
WIP: feat(cli): OpenFrame cluster commands to apply public EKS/GKE TF modules (K3d alternative)#223oleg-flamingocx wants to merge 39 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds managed EKS and GKE cluster support, including Terraform provisioning, cloud authentication, registry-backed workspaces, provider routing, discovery, kubeconfig switching, cloud safeguards, build-output standardization, CI coverage, and documentation. ChangesManaged cloud cluster support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
oleg-flamingocx
changed the title
…visible cluster (#239) - resolves local k3d (k3d-<name>/exact context), openframe-managed cloud (context = cluster name), and external GKE clusters found via discovery - external clusters without a kubeconfig entry get credentials fetched via `gcloud container clusters get-credentials` (private-cluster hint on failure); for GKE the gcloud configuration matching the cluster's project is activated so gcloud and kubectl line up - k8s.SwitchContext flips only current-context — never rewrites contexts, clusters, or users; the cluster itself is never touched - no-arg form offers interactive selection; logged-out gcloud yields an actionable auth hint; the prerequisite gate is skipped (no tools needed)
- discovery.AuthFlow: check auth state → confirm → run `gcloud auth login`
attached to the terminal → re-verify; `create --type gke` additionally
ensures Application Default Credentials (terraform's google provider uses
ADC, not CLI credentials)
- integrated into create (after the tool gate and in the dry-run preview),
list --all (offers login instead of a hint), and use (before discovery)
- non-interactive sessions never prompt (CI rule) and get the exact command
to run; declined or failed logins surface actionable errors
- all branches covered hermetically via interactive/confirm/runLogin seams
- new getting-started/gke-workflow.md: zero-to-cluster in order — optional
dry-run plan, create (wizard/flags) with the automatic tools→login→
provision chain spelled out, verify, app install, day-2 (list --all,
use incl. external clusters), resume-after-failure, remote state, delete,
troubleshooting table for the new error messages
- docs index distinguishes the reference (cloud-clusters.md) from the
walkthrough; cloud-clusters.md notes the EKS coming-soon gate
…y guards (#243) Audit findings on idempotency/safety, all fixed: - resume was unreachable through the CLI: any registry record (even status=failed) short-circuited into the "already exists" reuse path and errored on GetRestConfig. Non-Ready cloud records now fall through to the provider, whose terraform apply resumes from state (shouldResumeCloudCreate) - the reuse box printed a hardcoded green "Running" and a k3d-<name> network line for every cluster type; it now shows the real status and k3d rows only for k3d - the GKE name-collision preflight was region-scoped and missed zonal clusters; it now uses a project-wide clusters list --filter - delete removed kubeconfig entries by name alone; it now only removes them when the context still points at the record's endpoint (mirror of the create-side no-clobber guard)
The generic deletion confirmation refuses before the cloud-specific typed-confirm gate is reached in non-interactive sessions, so the step asserted the wrong message. Assert the actual guarantee instead: non-zero exit, an actionable refusal (either gate's message), and the workspace surviving.
The counts alone ("12 to add") don't tell the user WHAT would be created.
PlanSummary now carries per-resource changes in terraform's diff notation
(+ / ~ / - / -/+ with the resource address, in plan order) and the cloud
dry-run prints them above the summary line
The apply stream dropped planned_change events, so a real create printed
only the plan's change_summary count while dry-run listed the resources.
The filter now forwards planned_change ("...: Plan to create") lines,
making plan detail visible in both flows.
…ere (#247) - dry-run preview: when infracost is installed and working, print the monthly estimate computed from the plan JSON (PlanSummary now carries terraform's machine-readable plan); any infracost failure falls back silently — figures are never invented - without infracost: an abstract cost warning plus the provider's pricing page and a tip to install infracost - CostHint and the docs drop the stale $73/$33 hardcoded figures
make build/build-all now write to build/ (already gitignored); the CI OF_BIN path, the WSL staging step, and the dev docs follow.
…iew (#248) - pinned infracost v0.10.45 (all supported platforms, SHA256 from the release's per-asset checksums) installed into ~/.openframe/bin; the archive's irregular member layout is handled by a dedicated installer - interactive dry-run without infracost now offers the install; the user's only manual step is a one-time free 'infracost auth login' (surfaced when an estimate fails without a key) - non-interactive sessions never prompt or download; hermetic seam-based tests cover offer/accept/decline/failure paths
…249) A failed estimate (typically the missing free API key) now offers to run 'infracost auth login' right there, attached to the terminal, and retries the estimate once after a successful login — no separate console needed. Non-interactive sessions never prompt; hermetic seam-based tests cover the declined, accepted-and-retried, and CI paths.
…create (#250) - create now plans, shows every resource with the summary, and asks for approval before applying — and applies the SAVED plan, so what the user approved is exactly what runs (PlanForApply/ApplyPlan in the engine) - declining a brand-new create removes the fresh workspace (nothing was applied); declining a resume keeps the workspace and its state - non-interactive sessions auto-approve, preserving scripted/CI behavior; a no-change plan asks nothing - docs: create flow updated (plan-confirm step, private nodes + NAT)
…create (#251) - create now plans, shows every resource with the summary, and asks for approval before applying — and applies the SAVED plan, so what the user approved is exactly what runs (PlanForApply/ApplyPlan in the engine) - declining a brand-new create removes the fresh workspace (nothing was applied); declining a resume keeps the workspace and its state - non-interactive sessions auto-approve, preserving scripted/CI behavior; a no-change plan asks nothing - docs: create flow updated (plan-confirm step, private nodes + NAT
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cluster/ui/wizard_steps.go (1)
44-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the advertised cloud support with the executable wizard flow.
The wizard prevents interactive EKS creation, while cluster help advertises EKS and root help says all cloud providers are only planned. This leaves users unable to rely on the documented capability.
internal/cluster/ui/wizard_steps.go#L44-L75: returnmodels.ClusterTypeEKSwhen selected if EKS is part of this PR; otherwise remove it from supported-flow messaging.cmd/cluster/cluster.go#L30-L30: describe both runnable cloud providers accurately; do not claim EKS until the wizard can create it.cmd/root.go#L67-L67: mention the available GKE flow rather than stating cloud providers are merely planned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/ui/wizard_steps.go` around lines 44 - 75, Align supported-cloud messaging across PromptClusterType, cmd/cluster/cluster.go, and cmd/root.go: keep EKS out of the executable wizard flow unless this PR implements it, update the cluster help to advertise only runnable cloud providers, and replace the root help’s “planned” wording with the available GKE flow. Apply the corresponding changes at internal/cluster/ui/wizard_steps.go lines 44-75, cmd/cluster/cluster.go line 30, and cmd/root.go line 67.
🧹 Nitpick comments (7)
internal/cluster/models/flags.go (1)
177-179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend the cloud-only guard beyond
--backend-config.Only
--backend-configis rejected for non-cloud types.--region,--profile,--project,--machine-type,--min-nodes,--max-nodes, and--spotare all documented as "(cloud only)" in their flag descriptions (Lines 76-82) but silently accepted (and presumably ignored) fork3d. This can mask user typos, e.g.--type k3d --min-nodes 2succeeds but the flag is a no-op.♻️ Proposed fix
if flags.BackendConfig != "" && !isCloud { return fmt.Errorf("--backend-config only applies to cloud cluster types (eks, gke)") } + if !isCloud && (flags.Region != "" || flags.Profile != "" || flags.Project != "" || flags.MachineType != "" || + flags.MinNodes != 0 || flags.MaxNodes != 0 || flags.Spot) { + return fmt.Errorf("cloud-only flags (--region, --profile, --project, --machine-type, --min-nodes, --max-nodes, --spot) only apply to cloud cluster types (eks, gke)") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/models/flags.go` around lines 177 - 179, Extend the non-cloud validation in the flags validation flow around the existing BackendConfig guard to reject any non-default cloud-only options: region, profile, project, machine type, min-nodes, max-nodes, and spot. Reuse the existing flags fields and return a clear error identifying that these options only apply to cloud cluster types, while preserving valid cloud configurations.internal/cluster/providers/terraform/workspace.go (1)
128-135: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
WriteRecordisn't actually atomic despite the doc's "atomically enough" claim.
os.WriteFiletruncates and writes in place; a process kill mid-write (crash, OOM, SIGKILL) leaves a truncated/corruptcluster.json.ReadRecord(line 116-126) does surface this as a "corrupt cluster.json" error rather than silently succeeding, so it's not catastrophic — the terraform state file still exists for manual recovery — but for a record whose own package doc calls out billing-critical resource tracking, a temp-file+rename write would remove this failure mode entirely for negligible cost.🛠️ Suggested fix: write-then-rename for durability
func (w *Workspace) WriteRecord(r Record) error { data, err := json.MarshalIndent(r, "", " ") if err != nil { return err } - return os.WriteFile(filepath.Join(w.dir, recordFile), data, 0o600) + dst := filepath.Join(w.dir, recordFile) + tmp := dst + ".tmp" + if err := os.WriteFile(tmp, data, 0o600); err != nil { + return err + } + return os.Rename(tmp, dst) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/providers/terraform/workspace.go` around lines 128 - 135, Update Workspace.WriteRecord to write the marshaled record to a temporary file in w.dir, apply the intended 0600 permissions, then atomically rename it to recordFile only after the write succeeds. Ensure failures clean up the temporary file and do not replace the existing record.internal/cluster/prerequisites/terraform/terraform.go (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
commandExistshelper across packages.The same 4-line
exec.LookPathwrapper is duplicated verbatim ininternal/cluster/prerequisites/infracost/infracost.go(lines 26-29). Consider extracting a shared helper (e.g. ininternal/platform) reused by all prerequisite installers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/prerequisites/terraform/terraform.go` around lines 29 - 32, Extract the duplicated commandExists helper into a shared platform utility, then update the Terraform and Infracost prerequisite installers to use that implementation and remove their local helpers. Preserve the existing exec.LookPath-based behavior and adjust references to the shared symbol.internal/cluster/providers/eks/kubeconfig.go (1)
26-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared kubeconfig merge/remove logic between EKS and GKE.
caData,mergeIntoDefaultKubeconfig, andremoveFromDefaultKubeconfigare near-identical in both packages, differing only in theexecConfigconstruction. A shared helper parameterized on name/endpoint/CACert/ExecConfig would eliminate the duplication and the risk of a fix landing in one provider but not the other.
internal/cluster/providers/eks/kubeconfig.go#L26-L128: movecaData,mergeIntoDefaultKubeconfig,removeFromDefaultKubeconfiginto a shared package/helper taking a generic record (name, endpoint, CACert, ExecConfig) instead oftfengine.Recorddirectly.internal/cluster/providers/gke/kubeconfig.go#L26-L119: switch to the same shared helper, passing GKE'sexecConfig()in place of EKS'sexecConfig(rec).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/providers/eks/kubeconfig.go` around lines 26 - 128, Extract the shared CA decoding and kubeconfig merge/removal behavior from internal/cluster/providers/eks/kubeconfig.go lines 26-128 into a common helper that accepts cluster name, endpoint, CACert, and an ExecConfig, then update EKS callers to use it while retaining EKS-specific execConfig construction. Update internal/cluster/providers/gke/kubeconfig.go lines 26-119 to use the same helper and pass GKE’s execConfig result, removing its duplicated caData, mergeIntoDefaultKubeconfig, and removeFromDefaultKubeconfig implementations.internal/cluster/providers/eks/templates/main.tf (2)
9-14: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAWS provider constraint has no upper bound.
version = ">= 6.52"allows any future major version, while theeks/vpcmodule pins use~>(pessimistic constraint). A future AWS provider major release could breakterraform init/planfor existing and new workspaces with no warning, since nothing in this root module caps it.♻️ Suggested fix
aws = { source = "hashicorp/aws" - version = ">= 6.52" + version = "~> 6.52" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/providers/eks/templates/main.tf` around lines 9 - 14, Update the aws constraint in the required_providers block to add an upper bound consistent with the module ~> constraints, preventing unintended future major provider versions while retaining compatibility with the current 6.52 baseline.
109-109: 🔒 Security & Privacy | 🔵 TrivialPublic EKS endpoint has no CIDR restriction.
endpoint_public_access = truewith noendpoint_public_access_cidrsexposes the API server to the whole internet, relying solely on IAM for protection. Consider a--endpoint-cidrs-style flag defaulting to the operator's current IP, mirroring howenable_cluster_creator_admin_permissionsis already scoped to the identity running terraform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/providers/eks/templates/main.tf` at line 109, Restrict the public EKS API endpoint configured by endpoint_public_access by adding an endpoint_public_access_cidrs value sourced from a configurable endpoint-CIDRs input, defaulting to the operator’s current public IP. Thread this input through the existing cluster configuration and Terraform template symbols, while preserving the current public-access behavior when explicitly configured.internal/cluster/providers/eks/provider_test.go (1)
99-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap vs. the GKE suite: decline/resume paths untested.
eks.Provider.CreateClustershares the identicalconfirmApply-decline logic (lines 202-208 of provider.go) and resume-template-refresh logic (lines 177-185) withgke.Provider.CreateCluster, but only GKE's test suite exercises them (TestCreateCluster_DeclinedPlanAppliesNothing,TestCreateCluster_DeclinedResumeKeepsWorkspace,TestCreateCluster_ResumeRefreshesTemplate). A regression in either path on the EKS side would go undetected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/providers/eks/provider_test.go` around lines 99 - 294, Add EKS tests covering the missing CreateCluster decline and resume behavior, using the existing test helpers and symbols such as newTestProvider, CreateCluster, and the executor call tracking. Verify a declined plan performs no apply, a declined resume preserves the workspace, and a resumed create refreshes the Terraform template before continuing, matching the corresponding GKE test expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cluster/create.go`:
- Around line 37-52: Update the create command’s Long description and examples
to reflect the actual cloud behavior: describe GKE as the supported cloud
provider, mention that EKS creation is temporarily disabled, and replace the EKS
example with a valid GKE-oriented example. Keep the local k3d guidance and other
creation details unchanged.
- Around line 279-337: Update the EKS creation gate in the cluster creation flow
to return an actionable error after showEKSComingSoonBanner() instead of
returning nil. Preserve the banner display, and ensure EKS create
invocations—including skip-wizard and non-interactive runs—exit nonzero without
proceeding to cluster creation.
In `@cmd/cluster/list.go`:
- Around line 96-139: Update isManaged in discoverExternalClusters so local k3d
entries with an empty Project do not match and suppress external GKE clusters
solely because their names collide. Require a project-aware match when filtering
discovered clusters, while preserving removal of genuinely managed GKE clusters.
In `@cmd/cluster/use.go`:
- Around line 96-139: Update the credential-fetch command in useExternalGKE to
pass found.Region via --location instead of --region, supporting both zonal and
regional GKE clusters. Add a test covering a context-less zonal cluster and
verify the generated context and command use the location value correctly.
In `@internal/cluster/prerequisites/aws/aws.go`:
- Around line 67-89: Update AwsInstaller.installLinux to install AWS CLI v2
packages: use the v2 package name for apt-based systems and aws-cli-v2 for
pacman, while removing or replacing unsupported dnf/yum entries with an official
AWS bundle fallback as appropriate. Preserve the existing sudo non-interactive
execution and manager iteration behavior.
In `@internal/cluster/provider/provider.go`:
- Around line 4-8: Update the package documentation in provider to reflect that
EKS and GKE are already routed to their real eks.New and gke.New backends,
removing the outdated statement that they return ErrProviderNotFound until their
backends land. Preserve the description of the shared Provider interface and
backend abstraction.
In `@internal/cluster/providers/eks/provider.go`:
- Around line 134-193: Add the existing name-collision preflight used by GKE to
EKS CreateCluster within the freshWorkspace block, after preparing the Terraform
variables and before ws.Scaffold. Pass the relevant context, region, profile,
and cluster name arguments expected by preflightNameCollision, and return its
error without creating the workspace when an unmanaged EKS cluster with the same
name exists.
In `@internal/cluster/providers/gke/templates/main.tf`:
- Around line 123-165: Add an explicit depends_on relationship to the module
"gke" block, referencing google_compute_router_nat.nat, so Cloud NAT is
provisioned before the private GKE node pool. Leave the existing module
configuration unchanged.
In `@internal/cluster/providers/terraform/confirm.go`:
- Around line 22-31: Update the confirmation prompt in the apply-plan flow to
describe the complete plan rather than always referencing summary.Add. Use the
existing summary counts (or an equivalent total) so plans containing only
changes or destroys produce an accurate message, while preserving the existing
ConfirmActionInteractive behavior and confirmation gate.
In `@internal/cluster/providers/terraform/cost.go`:
- Around line 38-70: The EstimateMonthlyCost function must locally bound the
infracost breakdown execution and use the existing fallback behavior when it
times out or cannot complete, including contexts without a deadline. Add a child
context with a finite timeout around execer.Execute, ensure it is canceled, and
handle timeout/failure without allowing the dry-run preview to hang; preserve
the existing successful parsing and cost formatting flow.
In `@internal/cluster/service.go`:
- Around line 272-288: Update ClusterService.ListClusters so errors from an
individual cloud provider’s ListAllClusters call are skipped rather than
returned, preserving local clusters and results already collected from earlier
providers; retain the existing error handling for the initial
manager.ListAllClusters call.
In `@internal/cluster/ui/wizard_steps.go`:
- Around line 93-105: Update the PromptRegion prompt label from “AWS Region” to
the provider-neutral “Region,” while preserving its validation, default value,
trimming, and error behavior for both AWS and GKE flows.
In `@internal/cluster/ui/wizard.go`:
- Around line 89-116: Update the PromptRegion method to accept a label argument
and use it for the prompt text instead of always displaying “AWS Region”. In the
cloud configuration flow around PromptRegion, pass “GCP Region” for GKE and “AWS
Region” for EKS, preserving the existing default-region behavior.
In `@internal/shared/download/pins_test.go`:
- Around line 315-335: Set Infracost’s Tarball field to true in its pin
definition, then update TestInfracost_Pins to assert Infracost.Tarball is
enabled alongside the existing version and asset checks.
In `@README.md`:
- Line 183: Update the EKS README example annotation to state that EKS cluster
creation is currently gated as coming soon and does not create a billed cloud
resource. Keep the command unchanged and align the wording with the existing
gating description in the cloud-clusters documentation.
---
Outside diff comments:
In `@internal/cluster/ui/wizard_steps.go`:
- Around line 44-75: Align supported-cloud messaging across PromptClusterType,
cmd/cluster/cluster.go, and cmd/root.go: keep EKS out of the executable wizard
flow unless this PR implements it, update the cluster help to advertise only
runnable cloud providers, and replace the root help’s “planned” wording with the
available GKE flow. Apply the corresponding changes at
internal/cluster/ui/wizard_steps.go lines 44-75, cmd/cluster/cluster.go line 30,
and cmd/root.go line 67.
---
Nitpick comments:
In `@internal/cluster/models/flags.go`:
- Around line 177-179: Extend the non-cloud validation in the flags validation
flow around the existing BackendConfig guard to reject any non-default
cloud-only options: region, profile, project, machine type, min-nodes,
max-nodes, and spot. Reuse the existing flags fields and return a clear error
identifying that these options only apply to cloud cluster types, while
preserving valid cloud configurations.
In `@internal/cluster/prerequisites/terraform/terraform.go`:
- Around line 29-32: Extract the duplicated commandExists helper into a shared
platform utility, then update the Terraform and Infracost prerequisite
installers to use that implementation and remove their local helpers. Preserve
the existing exec.LookPath-based behavior and adjust references to the shared
symbol.
In `@internal/cluster/providers/eks/kubeconfig.go`:
- Around line 26-128: Extract the shared CA decoding and kubeconfig
merge/removal behavior from internal/cluster/providers/eks/kubeconfig.go lines
26-128 into a common helper that accepts cluster name, endpoint, CACert, and an
ExecConfig, then update EKS callers to use it while retaining EKS-specific
execConfig construction. Update internal/cluster/providers/gke/kubeconfig.go
lines 26-119 to use the same helper and pass GKE’s execConfig result, removing
its duplicated caData, mergeIntoDefaultKubeconfig, and
removeFromDefaultKubeconfig implementations.
In `@internal/cluster/providers/eks/provider_test.go`:
- Around line 99-294: Add EKS tests covering the missing CreateCluster decline
and resume behavior, using the existing test helpers and symbols such as
newTestProvider, CreateCluster, and the executor call tracking. Verify a
declined plan performs no apply, a declined resume preserves the workspace, and
a resumed create refreshes the Terraform template before continuing, matching
the corresponding GKE test expectations.
In `@internal/cluster/providers/eks/templates/main.tf`:
- Around line 9-14: Update the aws constraint in the required_providers block to
add an upper bound consistent with the module ~> constraints, preventing
unintended future major provider versions while retaining compatibility with the
current 6.52 baseline.
- Line 109: Restrict the public EKS API endpoint configured by
endpoint_public_access by adding an endpoint_public_access_cidrs value sourced
from a configurable endpoint-CIDRs input, defaulting to the operator’s current
public IP. Thread this input through the existing cluster configuration and
Terraform template symbols, while preserving the current public-access behavior
when explicitly configured.
In `@internal/cluster/providers/terraform/workspace.go`:
- Around line 128-135: Update Workspace.WriteRecord to write the marshaled
record to a temporary file in w.dir, apply the intended 0600 permissions, then
atomically rename it to recordFile only after the write succeeds. Ensure
failures clean up the temporary file and do not replace the existing record.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e802beb-bce4-419c-a5c4-bb38571886bf
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (88)
.github/workflows/test.ymlMakefileREADME.mdcmd/cluster/cluster.gocmd/cluster/contract_test.gocmd/cluster/create.gocmd/cluster/create_behavior_test.gocmd/cluster/delete.gocmd/cluster/delete_test.gocmd/cluster/list.gocmd/cluster/list_test.gocmd/cluster/status_json_test.gocmd/cluster/use.gocmd/cluster/use_test.gocmd/root.godocs/README.mddocs/architecture/decisions.mddocs/development/setup/local-development.mddocs/getting-started/cloud-clusters.mddocs/getting-started/gke-workflow.mdgo.modinternal/chart/prerequisites/installer_test.gointernal/cluster/discovery/auth.gointernal/cluster/discovery/auth_test.gointernal/cluster/discovery/gke.gointernal/cluster/discovery/gke_test.gointernal/cluster/models/cluster.gointernal/cluster/models/cluster_test.gointernal/cluster/models/flags.gointernal/cluster/models/flags_test.gointernal/cluster/prerequisites/aws/aws.gointernal/cluster/prerequisites/checker.gointernal/cluster/prerequisites/checker_test.gointernal/cluster/prerequisites/docker/docker_test.gointernal/cluster/prerequisites/gcloud/gcloud.gointernal/cluster/prerequisites/infracost/infracost.gointernal/cluster/prerequisites/installer.gointernal/cluster/prerequisites/installer_test.gointernal/cluster/prerequisites/k3d/k3d_test.gointernal/cluster/prerequisites/terraform/terraform.gointernal/cluster/prerequisites/terraform/terraform_test.gointernal/cluster/provider/factory.gointernal/cluster/provider/factory_test.gointernal/cluster/provider/provider.gointernal/cluster/providers/eks/kubeconfig.gointernal/cluster/providers/eks/provider.gointernal/cluster/providers/eks/provider_test.gointernal/cluster/providers/eks/template.gointernal/cluster/providers/eks/template_validate_test.gointernal/cluster/providers/eks/templates/main.tfinternal/cluster/providers/gke/kubeconfig.gointernal/cluster/providers/gke/provider.gointernal/cluster/providers/gke/provider_test.gointernal/cluster/providers/gke/template.gointernal/cluster/providers/gke/template_validate_test.gointernal/cluster/providers/gke/templates/main.tfinternal/cluster/providers/k3d/manager.gointernal/cluster/providers/k3d/manager_test.gointernal/cluster/providers/terraform/backend.gointernal/cluster/providers/terraform/backend_test.gointernal/cluster/providers/terraform/confirm.gointernal/cluster/providers/terraform/cost.gointernal/cluster/providers/terraform/cost_test.gointernal/cluster/providers/terraform/engine.gointernal/cluster/providers/terraform/engine_test.gointernal/cluster/providers/terraform/progress.gointernal/cluster/providers/terraform/workspace.gointernal/cluster/providers/terraform/workspace_test.gointernal/cluster/service.gointernal/cluster/service_test.gointernal/cluster/types_test.gointernal/cluster/ui/operations.gointernal/cluster/ui/prompts.gointernal/cluster/ui/prompts_test.gointernal/cluster/ui/service.gointernal/cluster/ui/wizard.gointernal/cluster/ui/wizard_steps.gointernal/cluster/ui/wizard_steps_test.gointernal/cluster/ui/wizard_test.gointernal/cluster/utils/cmd_helpers.gointernal/k8s/contexts.gointernal/k8s/contexts_test.gointernal/platform/platform.gointernal/shared/config/system_test.gointernal/shared/config/transport_test.gointernal/shared/download/pins.gointernal/shared/download/pins_test.gointernal/shared/download/verify.go
💤 Files with no reviewable changes (4)
- internal/cluster/ui/wizard_test.go
- internal/cluster/ui/wizard_steps_test.go
- internal/cluster/ui/prompts_test.go
- internal/cluster/types_test.go
| // discoverExternalClusters runs GKE discovery, dropping entries that are | ||
| // already managed (same name+project as a registry cluster). Auth problems | ||
| // degrade to notices, never errors: a logged-out gcloud must not break list. | ||
| // EKS discovery is not implemented yet — a notice says so. | ||
| func discoverExternalClusters(ctx context.Context, managed []models.ClusterInfo) ([]models.ClusterInfo, []string) { | ||
| notices := []string{"AWS EKS discovery is coming soon — external EKS clusters are not shown yet"} | ||
|
|
||
| exec := utils.CommandExecutor() | ||
| d := discovery.NewGKEDiscoverer(exec) | ||
| switch d.AuthStatus(ctx) { | ||
| case discovery.CLIMissing: | ||
| return nil, append(notices, "GKE: gcloud is not installed — install it to discover external clusters") | ||
| case discovery.NotAuthenticated: | ||
| // One unambiguous flow: offer the login right here (interactive only — | ||
| // non-interactive sessions get the same message as before). | ||
| if err := discovery.NewAuthFlow(exec).Ensure(ctx, false); err != nil { | ||
| return nil, append(notices, "GKE: "+err.Error()) | ||
| } | ||
| } | ||
|
|
||
| result, err := d.Discover(ctx) | ||
| if err != nil { | ||
| return nil, append(notices, fmt.Sprintf("GKE discovery failed: %v", err)) | ||
| } | ||
| for _, w := range result.Warnings { | ||
| notices = append(notices, "GKE discovery skipped "+w) | ||
| } | ||
|
|
||
| isManaged := func(c models.ClusterInfo) bool { | ||
| for _, m := range managed { | ||
| if m.Name == c.Name && (m.Project == "" || m.Project == c.Project) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
| var external []models.ClusterInfo | ||
| for _, c := range result.Clusters { | ||
| if !isManaged(c) { | ||
| external = append(external, c) | ||
| } | ||
| } | ||
| return external, notices | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isManaged can mask a real external cluster when names collide across types.
isManaged (Lines 124-131) matches only on Name and Project, and local k3d entries always have Project == "", so the m.Project == "" branch is satisfied unconditionally for any k3d cluster. A k3d cluster sharing a name with an unrelated external GKE cluster (e.g., the common default openframe-dev) will cause that GKE cluster to be silently dropped from --all output.
🐛 Proposed fix
isManaged := func(c models.ClusterInfo) bool {
for _, m := range managed {
- if m.Name == c.Name && (m.Project == "" || m.Project == c.Project) {
+ if m.Name == c.Name && m.Type == c.Type && (m.Project == "" || m.Project == c.Project) {
return true
}
}
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // discoverExternalClusters runs GKE discovery, dropping entries that are | |
| // already managed (same name+project as a registry cluster). Auth problems | |
| // degrade to notices, never errors: a logged-out gcloud must not break list. | |
| // EKS discovery is not implemented yet — a notice says so. | |
| func discoverExternalClusters(ctx context.Context, managed []models.ClusterInfo) ([]models.ClusterInfo, []string) { | |
| notices := []string{"AWS EKS discovery is coming soon — external EKS clusters are not shown yet"} | |
| exec := utils.CommandExecutor() | |
| d := discovery.NewGKEDiscoverer(exec) | |
| switch d.AuthStatus(ctx) { | |
| case discovery.CLIMissing: | |
| return nil, append(notices, "GKE: gcloud is not installed — install it to discover external clusters") | |
| case discovery.NotAuthenticated: | |
| // One unambiguous flow: offer the login right here (interactive only — | |
| // non-interactive sessions get the same message as before). | |
| if err := discovery.NewAuthFlow(exec).Ensure(ctx, false); err != nil { | |
| return nil, append(notices, "GKE: "+err.Error()) | |
| } | |
| } | |
| result, err := d.Discover(ctx) | |
| if err != nil { | |
| return nil, append(notices, fmt.Sprintf("GKE discovery failed: %v", err)) | |
| } | |
| for _, w := range result.Warnings { | |
| notices = append(notices, "GKE discovery skipped "+w) | |
| } | |
| isManaged := func(c models.ClusterInfo) bool { | |
| for _, m := range managed { | |
| if m.Name == c.Name && (m.Project == "" || m.Project == c.Project) { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| var external []models.ClusterInfo | |
| for _, c := range result.Clusters { | |
| if !isManaged(c) { | |
| external = append(external, c) | |
| } | |
| } | |
| return external, notices | |
| } | |
| // discoverExternalClusters runs GKE discovery, dropping entries that are | |
| // already managed (same name+project as a registry cluster). Auth problems | |
| // degrade to notices, never errors: a logged-out gcloud must not break list. | |
| // EKS discovery is not implemented yet — a notice says so. | |
| func discoverExternalClusters(ctx context.Context, managed []models.ClusterInfo) ([]models.ClusterInfo, []string) { | |
| notices := []string{"AWS EKS discovery is coming soon — external EKS clusters are not shown yet"} | |
| exec := utils.CommandExecutor() | |
| d := discovery.NewGKEDiscoverer(exec) | |
| switch d.AuthStatus(ctx) { | |
| case discovery.CLIMissing: | |
| return nil, append(notices, "GKE: gcloud is not installed — install it to discover external clusters") | |
| case discovery.NotAuthenticated: | |
| // One unambiguous flow: offer the login right here (interactive only — | |
| // non-interactive sessions get the same message as before). | |
| if err := discovery.NewAuthFlow(exec).Ensure(ctx, false); err != nil { | |
| return nil, append(notices, "GKE: "+err.Error()) | |
| } | |
| } | |
| result, err := d.Discover(ctx) | |
| if err != nil { | |
| return nil, append(notices, fmt.Sprintf("GKE discovery failed: %v", err)) | |
| } | |
| for _, w := range result.Warnings { | |
| notices = append(notices, "GKE discovery skipped "+w) | |
| } | |
| isManaged := func(c models.ClusterInfo) bool { | |
| for _, m := range managed { | |
| if m.Name == c.Name && m.Type == c.Type && (m.Project == "" || m.Project == c.Project) { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| var external []models.ClusterInfo | |
| for _, c := range result.Clusters { | |
| if !isManaged(c) { | |
| external = append(external, c) | |
| } | |
| } | |
| return external, notices | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/cluster/list.go` around lines 96 - 139, Update isManaged in
discoverExternalClusters so local k3d entries with an empty Project do not match
and suppress external GKE clusters solely because their names collide. Require a
project-aware match when filtering discovered clusters, while preserving removal
of genuinely managed GKE clusters.
| func (a *AwsInstaller) installLinux() error { | ||
| type pm struct { | ||
| name string | ||
| args []string | ||
| } | ||
| managers := []pm{ | ||
| {"apt", []string{"apt", "install", "-y", "awscli"}}, | ||
| {"dnf", []string{"dnf", "install", "-y", "awscli"}}, | ||
| {"yum", []string{"yum", "install", "-y", "awscli"}}, | ||
| {"pacman", []string{"pacman", "-S", "--noconfirm", "aws-cli"}}, | ||
| } | ||
| for _, m := range managers { | ||
| if !commandExists(m.name) { | ||
| continue | ||
| } | ||
| // Package installs need root; sudo -n keeps this non-interactive (the | ||
| // prerequisite flow already runs under a user confirmation). | ||
| if err := runCommand("sudo", append([]string{"-n"}, m.args...)...); err == nil { | ||
| return nil | ||
| } | ||
| } | ||
| return fmt.Errorf("could not install the AWS CLI automatically. %s", a.GetInstallHelp()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'aws\.go$' . | sed 's#^\./##'
echo "== file outline =="
ast-grep outline internal/cluster/prerequisites/aws/aws.go --view compact || true
echo "== relevant lines =="
sed -n '1,140p' internal/cluster/prerequisites/aws/aws.go | nl -ba
echo "== all aws-cli package/install references =="
rg -n "awsci2|awscli|aws-cli|get-token|client\.authentication\.k8s\.io|Amazon EKS|aws eks" internal/cluster/prerequisites/aws/aws.go internal/cluster -SRepository: flamingo-stack/openframe-cli
Length of output: 480
🌐 Web query:
AWS CLI v2 install package manager apt dnf yum pacman awscli aws-cli awscli2 v1 vs v2 current package names
💡 Result:
AWS CLI v2 is the current, recommended major version of the AWS Command Line Interface [1][2]. While official AWS documentation focuses on a universal command-line installer (available as a.zip package) [3][4] and a snap package (aws-cli) [3][4], many Linux distributions provide their own packaged versions of v2 in their official repositories [5][6]. Key differences and installation patterns: Command Name and Versioning Both AWS CLI v1 and v2 use the same command name, aws [7]. If both are installed, the one appearing first in your system's PATH will execute [7]. If you must have both available, it is recommended to create a symlink or alias (e.g., aws2) for the v2 binary [7]. Package Names by Distribution Distribution-specific repositories may use different names to distinguish v2 from the legacy v1 (often named awscli): - Arch Linux: The package is named aws-cli-v2 [5][8]. - Fedora/RHEL: The package is frequently named awscli2 [6][9]. - Other Distributions: Depending on the maintainers, you may find it under awscli2 or sometimes bundled as a newer version of the legacy awscli package. Installation Recommendations - Official Recommendation: AWS officially recommends using their provided command-line installer for version control [3][4] or the snap package (snap install aws-cli --classic) if you prefer automatic updates [3][4]. - Distribution Packages: Using your system's native package manager (apt, dnf, yum, pacman) is a common alternative that integrates with the OS update lifecycle [8][10]. Always check your specific distribution's repository for the correct package name (e.g., searching with apt search awscli or dnf search awscli) before installing, as names vary by vendor. Migration from v1 AWS CLI v2 is not backwards-compatible with all v1 features [1]. If you are migrating, AWS provides a migration guide and a v1-to-v2 migration tool to help identify and address breaking changes in scripts or workflows [1].
Citations:
- 1: https://docs.aws.amazon.com/cli/latest/userguide/cliv2-migration.html
- 2: https://docs.aws.amazon.com/cli/latest/userguide/installing.html
- 3: https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html
- 4: https://docs.aws.amazon.com/cli/latest/userguide/cli-chap-install.html
- 5: https://archlinux.org/packages/extra/any/aws-cli-v2/
- 6: https://packages.fedoraproject.org/pkgs/awscli2/awscli2/
- 7: https://docs.aws.amazon.com/cli/latest/userguide/cliv2-migration-instructions.html
- 8: https://linuxcapable.com/how-to-install-aws-cli-on-arch-linux/
- 9: https://packages.fedoraproject.org/pkgs/awscli2/awscli2/fedora-rawhide.html
- 10: https://docs.aws.amazon.com/cli/latest/userguide/getting-started-source-install.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant file content =="
sed -n '1,140p' internal/cluster/prerequisites/aws/aws.go
echo "== references in aws prerequisite and related cluster code =="
rg -n "awsci2|awscli|aws-cli|get-token|client\.authentication\.k8s\.io|Amazon EKS|aws eks" internal/cluster/prerequisites/aws/aws.go internal/cluster -S || true
echo "== package names in repo =="
rg -n "aws-cli|awscli|aws-cli-v2|awscli2" . -S || true
echo "== parse package manager map and commands =="
python3 - <<'PY'
from pathlib import Path
p=Path('internal/cluster/prerequisites/aws/aws.go')
text=p.read_text()
start=text.index('func (a *AwsInstaller) installLinux()')
end=text.find('\n}', start)
print(text[start:end+2])
PYRepository: flamingo-stack/openframe-cli
Length of output: 6729
🌐 Web query:
Debian Ubuntu apt package awscli AWS CLI v2 awscli2 package
💡 Result:
For Debian and Ubuntu systems, there is no official apt package named awscli2. While official repositories for Debian and Ubuntu distributions may contain an awscli package, its versioning and availability depend on the specific release of the operating system [1][2][3]. AWS does not maintain these distribution-specific repositories and cannot guarantee that they contain the latest version of the AWS CLI [4][5][6]. If you require the most up-to-date version of AWS CLI v2, AWS explicitly recommends using their official installation methods rather than relying on system-packaged versions [4][5]. Official Recommended Methods: 1. Official AWS CLI v2 Installer (Recommended for version control): This method downloads a bundled installer directly from AWS, which installs the CLI to /usr/local/aws-cli and creates symlinks in /usr/local/bin [4][2][3]. - Download the bundle: curl "https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip" -o "awscliv2.zip" - Extract and install: unzip awscliv2.zip && sudo./aws/install [4][3] - To update, run the installer with the --update flag: sudo./aws/install --update [3] 2. Official Snap Package (Recommended for automatic updates): AWS provides an official, supported snap package that handles automatic updates [4][5]. - Install via snap: sudo snap install aws-cli --classic [4][2] Important Considerations: - Distribution Packages: You may see an awscli package available via sudo apt install awscli. Depending on your OS version (e.g., Debian Trixie or recent Ubuntu releases), this may provide a version of the v2 CLI, but it is managed by the distribution maintainers, not AWS [2][3][7]. - Third-Party Repositories: Avoid unofficial apt repositories claiming to provide awscli2 unless you have verified their security and maintenance practices, as these are not supported or endorsed by AWS [4][8].
Citations:
- 1: https://linuxcapable.com/how-to-install-aws-cli-on-ubuntu-linux/
- 2: https://linuxcapable.com/how-to-install-aws-cli-on-debian-linux/
- 3: https://thelinuxcode.com/how-to-install-aws-cli-on-ubuntu-aws-cli-v2-apt-and-python-based-options-with-sso-profiles-and-real-troubleshooting/
- 4: https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html
- 5: https://docs.aws.amazon.com/cli/latest/userguide/cli-chap-install.html
- 6: https://docs.aws.amazon.com/cli/latest/userguide/getting-started-install.html?sc_channel=sm&sc_campaign=Docs&sc_publisher=TWITTER&sc_country=Global&sc_outcome=awareness&trk=Docs_TWITTER&sc_content=Docs&linkId=77210096
- 7: https://packages.debian.org/stable/awscli
- 8: https://github.com/izumo-m/apt-awscli-v2/blob/main/README.md
🌐 Web query:
AWS EKS aws eks get-token client.authentication.k8s.io v1alpha1 removed Kubernetes 1.24 documentation
💡 Result:
The API version client.authentication.k8s.io/v1alpha1 was removed in Kubernetes 1.24 [1][2]. This removal meant that any credential plugins, including older versions of the aws eks get-token command, that continued to use v1alpha1 in their kubeconfig files caused Kubernetes clients (like kubectl 1.24+) to return an error stating that the API version was invalid [3][1][4]. To resolve this, users must update their kubeconfig files to use a supported API version, such as client.authentication.k8s.io/v1beta1 or client.authentication.k8s.io/v1 [3][1][4]. Key points regarding this change: - Migration: The recommended way to fix this is to regenerate your kubeconfig file using the AWS CLI, which will automatically update the API version to a supported one [3][1]. You can run: aws eks update-kubeconfig --name - Compatibility: AWS CLI versions 1.24.0 and later were updated to default to using the v1beta1 API version to ensure compatibility with Kubernetes 1.24+ [5][3]. - Manual Fix: Alternatively, you can manually edit your ~/.kube/config file and change the apiVersion field under the user's exec configuration from client.authentication.k8s.io/v1alpha1 to client.authentication.k8s.io/v1beta1 [3][1]. This change followed standard Kubernetes deprecation policies, where alpha APIs can be removed without prior deprecation notice [6]. The v1alpha1 version had been replaced by more stable v1beta1 and v1 versions, which are now required for cluster authentication [2][4].
Citations:
- 1: Authentication with AWS broke with version 1.24 kubernetes/kubectl#1210
- 2: Remove v1alpha1 of the execcredential kubernetes/kubernetes#108616
- 3: [eks] [request]: aws eks get-token incompatible with kubectl >= 1.24 aws/containers-roadmap#1736
- 4: https://support.hashicorp.com/hc/en-us/articles/30843405358739-Invalid-apiVersion-client-authentication-k8s-io-v1alpha1-in-Kubernetes-Provider-1-24
- 5: Default to beta API for eks get-token aws/aws-cli#6940
- 6: https://kubernetes.io/blog/2022/04/07/upcoming-changes-in-kubernetes-1-24/
Use AWS CLI v2 packages for Linux auto-install.
installLinux installs apt/dnf/yum awscli, which on Debian/Ubuntu is the legacy v1 package, and Arch’s aws-cli is also v1 (aws-cli-v2 is the v2 package). For the EKS kubeconfig flow, install the v2 packages instead, or drop the unsupported Fedora/RHEL/RHEL-derived paths and offer an official AWS bundle fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cluster/prerequisites/aws/aws.go` around lines 67 - 89, Update
AwsInstaller.installLinux to install AWS CLI v2 packages: use the v2 package
name for apt-based systems and aws-cli-v2 for pacman, while removing or
replacing unsupported dnf/yum entries with an official AWS bundle fallback as
appropriate. Preserve the existing sudo non-interactive execution and manager
iteration behavior.
| func TestInfracost_Pins(t *testing.T) { | ||
| if !strings.HasPrefix(Infracost.Version, "v") { | ||
| t.Fatalf("Infracost.Version must be a v-prefixed tag, got %q", Infracost.Version) | ||
| } | ||
| for _, p := range []struct{ os, arch string }{ | ||
| {"linux", "amd64"}, {"linux", "arm64"}, | ||
| {"darwin", "amd64"}, {"darwin", "arm64"}, | ||
| {"windows", "amd64"}, | ||
| } { | ||
| asset, ok := Infracost.Asset(p.os, p.arch) | ||
| if !ok { | ||
| t.Errorf("no infracost asset for %s/%s", p.os, p.arch) | ||
| continue | ||
| } | ||
| if len(asset.SHA256) != 64 { | ||
| t.Errorf("%s/%s: SHA256 must be 64 hex chars, got %q", p.os, p.arch, asset.SHA256) | ||
| } | ||
| if !strings.Contains(asset.URL, Infracost.Version) || !strings.HasSuffix(asset.URL, p.os+"-"+p.arch+".tar.gz") { | ||
| t.Errorf("%s/%s: URL %q must contain version and end with platform.tar.gz", p.os, p.arch, asset.URL) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mark Infracost as a tarball and assert it here.
Infracost uses .tar.gz assets, but its pin lacks Tarball: true; InstallPinnedTool will therefore install compressed archive bytes as the executable. Set Tarball: true in internal/shared/download/pins.go and add an Infracost.Tarball assertion in this test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/shared/download/pins_test.go` around lines 315 - 335, Set
Infracost’s Tarball field to true in its pin definition, then update
TestInfracost_Pins to assert Infracost.Tarball is enabled alongside the existing
version and asset checks.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 15 file(s) based on 15 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
- create: gated EKS create now exits non-zero with an actionable
coming-soon error (scripts must not see success); help texts updated
- list: isManaged match is project-aware so a k3d cluster no longer
shadows a same-name external GKE cluster
- use: get-credentials uses --location (works for zonal clusters too)
- gke: module depends_on Cloud NAT so nodes can pull images on boot
- eks: preflight name-collision guard (mirrors GKE); aws provider
pinned '>= 6.52, < 7.0'
- terraform: apply prompt shows full plan counts; infracost run capped
at 60s; cluster.json written atomically (temp file + rename)
- flags: non-cloud types reject all cloud-only flags
- service: ListClusters degrades gracefully when a cloud provider
errors instead of failing the whole listing
- wizard: region prompt labeled per provider (GCP Region / AWS Region)
- tests: EKS banner error + CI step expect non-zero exit; zonal use
test; EKS collision + declined-plan tests
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cluster/ui/wizard_steps.go (1)
50-53: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not expose EKS as a dead-end option.
The wizard currently presents EKS, then refuses the selection and loops back. This makes the EKS flow in
internal/cluster/ui/wizard.gounreachable, contradicting the PR’s managed EKS support objective. Returnmodels.ClusterTypeEKShere, or remove the option until EKS is intentionally unavailable.Also applies to: 68-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cluster/ui/wizard_steps.go` around lines 50 - 53, The wizard’s cluster-type selection must not expose EKS while rejecting it afterward. Update the items and selection mapping in the wizard steps flow to either return models.ClusterTypeEKS and allow the existing EKS path in wizard.go to proceed, or remove the EKS option entirely until support is intentionally unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/cluster/ui/wizard_steps.go`:
- Around line 50-53: The wizard’s cluster-type selection must not expose EKS
while rejecting it afterward. Update the items and selection mapping in the
wizard steps flow to either return models.ClusterTypeEKS and allow the existing
EKS path in wizard.go to proceed, or remove the EKS option entirely until
support is intentionally unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b7bb4e3-96d2-401d-a40e-77df2c5ec36a
📒 Files selected for processing (21)
.github/workflows/test.ymlREADME.mdcmd/cluster/cluster.gocmd/cluster/create.gocmd/cluster/create_behavior_test.gocmd/cluster/list.gocmd/cluster/use.gocmd/cluster/use_test.gocmd/root.gointernal/cluster/models/flags.gointernal/cluster/provider/provider.gointernal/cluster/providers/eks/provider.gointernal/cluster/providers/eks/provider_test.gointernal/cluster/providers/eks/templates/main.tfinternal/cluster/providers/gke/templates/main.tfinternal/cluster/providers/terraform/confirm.gointernal/cluster/providers/terraform/cost.gointernal/cluster/providers/terraform/workspace.gointernal/cluster/service.gointernal/cluster/ui/wizard.gointernal/cluster/ui/wizard_steps.go
🚧 Files skipped from review as they are similar to previous changes (19)
- internal/cluster/providers/terraform/confirm.go
- cmd/root.go
- README.md
- internal/cluster/providers/terraform/cost.go
- cmd/cluster/cluster.go
- cmd/cluster/use.go
- internal/cluster/provider/provider.go
- cmd/cluster/use_test.go
- internal/cluster/providers/eks/templates/main.tf
- cmd/cluster/create.go
- internal/cluster/models/flags.go
- .github/workflows/test.yml
- internal/cluster/providers/gke/templates/main.tf
- cmd/cluster/create_behavior_test.go
- cmd/cluster/list.go
- internal/cluster/providers/eks/provider_test.go
- internal/cluster/providers/terraform/workspace.go
- internal/cluster/providers/eks/provider.go
- internal/cluster/service.go
Fedora/RHEL ship v2 as 'awscli2' and Arch as 'aws-cli-v2' (the old
'awscli'/'aws-cli' names are the retired v1). apt keeps 'awscli' (v2 on
Debian 12+/Ubuntu 24.04+) but the install now verifies the resulting
binary is v2 — legacy v1 repos (e.g. Ubuntu 22.04) get an actionable
error instead of a CLI whose 'aws eks get-token' emits an auth API
kubectl >=1.24 rejects.
…one (#266) The module built a regional cluster, so node counts were per-zone: --nodes 3 provisioned 3 nodes in each of the region's ~3 zones (~9 total) while every display still said 3 — roughly 3x the expected node bill, silently. Default to a single-zone cluster (regional=false), so the node pool's counts are the real totals and the existing 'Nodes: N' display becomes truthful with no display change. The provider resolves a concrete zone from the region via gcloud (alphabetically-first, deterministic so a resume never moves the cluster). A new --ha flag opts back into a regional, multi-zone control plane and node pool for HA. NOTE: the terraform template change (regional/zone vars, zones wiring) could not be validated in this environment — run 'make test-tfvalidate' (needs a terraform binary) before merging. fix(gke): release PVC disks before destroy and report any orphans after cluster delete ran a bare terraform destroy under a live platform. PVC- provisioned Persistent Disks live outside terraform state, so when the node pool died they detached and survived as billable orphans, while the CLI reported a clean teardown. Before destroy, delete the app namespaces (argocd first so its controller stops re-syncing, then openframe) and wait, bounded, for their PVC-backed PersistentVolumes to drain so the GKE CSI driver deletes the underlying disks while the cluster is still alive. Every step is best-effort and never blocks the destroy the user asked for. After a successful destroy, sweep for disks still labeled goog-k8s-cluster-name=<name> and report them with a cleanup command (report-only — the CLI never deletes cloud disks directly), so a 'cleaned up' delete never silently leaves orphans
Pre-resolve the develop->main doc conflicts on develop so the eventual merge into main is clean. main's #263/#264 (automated Technical Writer) restructured the docs and regenerated README/docs; those changes are absorbed here, except for four files kept at develop's versions because develop's are newer and reflect shipped GKE/cloud features main lacks: - README.md - docs/README.md - docs/development/setup/local-development.md - docs/architecture/decisions.md (bot deleted it; kept as it is a substantive hand-written architecture doc)
Ctrl+C during a GKE create left the terminal silent about recovery: the
apply error carried a 're-run create to resume' hint, but the interruption
handler prints only 'Operation cancelled by user.' and discards the error
text, so the hint (though truthful — the workspace state is preserved) was
never shown.
Carry the hint structurally via a ResumeHint() method on the error, and
have the interruption handler surface it with errors.As after the cancel
notice. The cancellation chain still satisfies errors.Is(context.Canceled),
so it is classified as an interruption exactly as before
1 participant
Summary by CodeRabbit
cluster use), and delete.cluster list --allto discover external GKE clusters.--force) and is safer in non-interactive mode.