GKE Phase 2c — cluster + node pool edit flows (e key + diff preview)#92
Open
slayer wants to merge 15 commits into
Open
GKE Phase 2c — cluster + node pool edit flows (e key + diff preview)#92slayer wants to merge 15 commits into
slayer wants to merge 15 commits into
Conversation
…t nil + drop Description from Design.md
…+ pool edit methods
…n with edit fields
…false-positive diff
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds Phase 2c GKE edit flows for clusters and node pools, including an e key binding to open edit forms, a diff preview before deploy, and sequential multi-endpoint edit execution under a single footer task.
Changes:
- Introduces new UI edit views (cluster + node pool) with 3-state lifecycle (form → diff → saving) and “no-op” submit rejection.
- Adds new GCP edit request builders +
ContainerClientmethods plus projection extensions for edit pre-population. - Wires navigation, rendering, and key bindings (
e) plus updates docs and tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ui/views/gke_node_pool_edit.go | Adds node pool edit form/diff/saving view and diff rendering. |
| internal/ui/views/gke_node_pool_edit_test.go | Adds tests for node pool edit no-op rejection, diffs, and request emission. |
| internal/ui/views/gke_cluster_edit.go | Adds cluster edit form/diff/saving view, validation, and diff rendering. |
| internal/ui/views/gke_cluster_edit_test.go | Adds tests for cluster edit no-op rejection, diffs, and request emission. |
| internal/ui/views/gke_edit_messages.go | Defines open/request/canceled/result messages for both edit flows. |
| internal/ui/views/gke_cluster_details.go | Wires e key to open cluster/pool edit and exposes Details/FocusedPool getters. |
| internal/ui/views/gke_cluster_details_test.go | Adds regression tests for the new e key behavior (Standard vs Autopilot). |
| internal/ui/app.go | Registers new view enums, view pointers, Update() routing, and size propagation. |
| internal/ui/app_navigation.go | Adds edit open/cancel/request/result handlers and sequential multi-step runner. |
| internal/ui/app_render.go | Adds header + view rendering for the new edit views. |
| internal/gcp/gke_edit.go | Introduces edit projections, request builders, and ContainerClient edit methods. |
| internal/gcp/gke_edit_test.go | Adds unit tests for request builder shapes and edit method guardrails. |
| internal/gcp/gke.go | Extends ClusterDetails/NodePool projections with edit pre-population fields. |
| internal/gcp/gke_test.go | Adds projection tests for the new edit-relevant fields. |
| doc/2026-05-19-gke-phase2c-edit/Design.md | Documents Phase 2c scope, architecture, and edge cases. |
| doc/2026-05-19-gke-phase2c-edit/TODO.md | Implementation plan detailing steps, tests, and wiring points. |
| .claude/rules/key-bindings.md | Documents the new e key bindings for edit flows. |
| README.md | Updates product feature list to include Phase 2c edit flows. |
| CLAUDE.md | Updates phase summary and adds Phase 2c bullets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4437
to
+4443
| a.viewStack = append(a.viewStack, a.currentView) | ||
| a.currentView = ViewGKEClusterEdit | ||
|
|
||
| var details *gcp.ClusterDetails | ||
| if a.gkeClusterDetailsView != nil { | ||
| details = a.gkeClusterDetailsView.Details() | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb — handleGKEClusterEditOpen now early-returns nil when details haven't loaded, preventing the buildForm nil-deref.
| // ── Basic section ──────────────────────────────────────────────────────── | ||
| // Resource labels: deferred (no key/value map editor in the forms framework yet). | ||
| // Show a read-only summary and a help-text note. | ||
| labelsDisplay := labelsToDisplay(v.details.ResourceLabels) |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb — when LoggingService/MonitoringService isn't in the curated dropdown list, computeEdit now uses "none" (the dropdown's default) as the baseline. Mirrors the pool-edit unknown-strategy fix. Regression test TestGKEClusterEdit_UnknownLoggingNoFalsePositive added.
Comment on lines
+153
to
+156
| if !isKnownLoggingValue(v.details.LoggingService) && v.details.LoggingService != "" { | ||
| loggingField.SetPlaceholder(v.details.LoggingService). | ||
| SetHelpText(fmt.Sprintf("Current value %q is not in the curated list; select an option to change it", v.details.LoggingService)) | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb (same fix as the other line — see above).
Comment on lines
+169
to
+180
| v.Form.SetData(map[string]any{ | ||
| "maintenance_kind": initialKind, | ||
| "maintenance_daily_start": v.details.MaintenanceDaily, | ||
| }) | ||
|
|
||
| // Pre-populate dropdowns only when the value is in the curated list. | ||
| if isKnownLoggingValue(v.details.LoggingService) { | ||
| v.Form.SetData(map[string]any{"logging_service": v.details.LoggingService}) | ||
| } | ||
| if isKnownMonitoringValue(v.details.MonitoringService) { | ||
| v.Form.SetData(map[string]any{"monitoring_service": v.details.MonitoringService}) | ||
| } |
Comment on lines
+384
to
+392
|
|
||
| var basic *gcp.ClusterEdit | ||
| if newLogging != initialLogging || newMonitoring != initialMonitoring { | ||
| basic = &gcp.ClusterEdit{} | ||
| if newLogging != initialLogging { | ||
| s := newLogging | ||
| basic.LoggingService = &s | ||
| } | ||
| if newMonitoring != initialMonitoring { |
Comment on lines
+4552
to
+4565
| func (a *App) handleGKENodePoolEditOpen(msg views.GKENodePoolEditOpenMsg) tea.Cmd { | ||
| var pool *gcp.NodePool | ||
| if a.gkeClusterDetailsView != nil { | ||
| pool = a.gkeClusterDetailsView.FocusedPool() | ||
| } | ||
| if pool == nil { | ||
| // Shouldn't happen — the details view guards before emitting the | ||
| // open message. Defensively stay on the current view. | ||
| return nil | ||
| } | ||
|
|
||
| a.viewStack = append(a.viewStack, a.currentView) | ||
| a.currentView = ViewGKENodePoolEdit | ||
| a.gkeNodePoolEditView = views.NewGKENodePoolEditView(msg.ProjectID, msg.Location, msg.ClusterName, pool) |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb — handler now resolves the pool by msg.PoolName from details.NodePools instead of trusting current focus. Defensive nil-check on Details too. Contract is now honored regardless of where the message originates.
| unknownStrategy := "" | ||
|
|
||
| if v.pool.UpgradeSettings == nil { | ||
| upgradeHelpText = "Pool currently has no explicit upgrade settings; submitting will set defaults." |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb — help text changed to "Pool has no explicit upgrade settings; change a value to set them." to match the no-op submit guard.
| return nil | ||
| } | ||
| if fields == nil && management == nil { | ||
| v.err = errNodePoolEditNoChanges |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb (same text update — see above).
|
|
||
| // labelsToDisplay formats a label map as "key=value, key=value" sorted alphabetically. | ||
| // Returns "(none)" when empty. | ||
| func labelsToDisplay(labels map[string]string) string { |
Owner
Author
There was a problem hiding this comment.
Fixed in f7db0bb — removed the duplicate nodePoolLabelsToDisplay; both edit views now call the shared labelsToDisplay.
- handleGKEClusterEditOpen guards nil details (was nil-deref in buildForm)
if the user pressed `e` before the cluster fetch returned.
- Cluster edit's logging/monitoring baseline now matches the dropdown
default ("none") when the cluster's value isn't in the curated list,
preventing a false-positive diff on every submit. Mirrors the pool-edit
unknown-strategy fix; regression test added.
- handleGKENodePoolEditOpen now resolves the pool by msg.PoolName instead
of FocusedPool(), so the message contract is honored even if focus
shifts (or a future emitter doesn't piggyback on the table cursor).
- Help text for pools without explicit upgrade settings now matches
behavior ("change a value to set them") since the no-op submit guard
rejects unchanged submissions.
- Removed duplicate nodePoolLabelsToDisplay; both edit views now use the
shared labelsToDisplay helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
eon Overview tab, Standard + Autopilot): logging service, monitoring service, daily maintenance window. Resource-labels editor deferred (read-only display for now).eon Node Pools tab, Standard only): auto-upgrade/auto-repair toggles + upgrade strategy/max-surge/max-unavailable. K8s labels and taints are read-only for MVP (key/value editor deferred).instance_editor.go. No-op submits are rejected at the form layer.gke-op:edit-cluster:<name>footer task; status updates as each step lands.Key bindings (Cluster Details)
e→ cluster edit (works on Autopilot too)e→ pool edit for focused pool (Standard only; no-op on Autopilot)Test plan
go test ./...clean — includes 6 new projection tests + 4 cluster-edit tests + 5 pool-edit tests + 4 detail-view regression testsgo test ./internal/ui/... -race -timeout 60sclean (race-free closure capture inrunGKEEditSteprecursion)make lintclean (0 issues)eon Overview works;eon Node Pools is a no-opArchitecture
New data layer (
internal/gcp/):gke_edit.go— pointer-based edit projections (ClusterEdit,MaintenanceWindow,NodePoolEdit,NodePoolManagement) + pure request builders + 4ContainerClientmethods (UpdateClusterBasic,SetClusterMaintenancePolicy,UpdateNodePoolFields,SetNodePoolManagement). Resource labels go throughClusters.SetResourceLabels(separate endpoint, requiresLabelFingerprint).Extended projections (
gke.go):ClusterDetailsgainedResourceLabels,ResourceLabelsFingerprint,LoggingService,MonitoringService,MaintenanceDailyNodePoolgainedLabels,Taints,Tags,UpgradeSettingsNew UI (
internal/ui/views/):gke_cluster_edit.go,gke_node_pool_edit.go— 3-state form/diff/saving views (do NOT useCreateViewBase— different lifecycle)gke_edit_messages.go— Open / Request / Canceled / Result for both flowsApp layer (
app_navigation.go):runGKEEditSequence/runGKEEditStep— sequential operation polling: each step kicks API call → pollsGKEOperationPollMsg→ on DONE recursively schedules next step → finalonComplete(nil)emits the result msgGKEOperationPollMsg,pollGKEOperationOnce,borrowContainerClient,finishTask)Notable bugs caught and fixed during two-stage review
buildSetResourceLabelsRequestnil-deref on zero-value edit (guarded + regression test)SHORT_LIVED) — baseline forced to dropdown default + placeholder; regression testUpdateClusterBasicsentinel errors were doubly-wrapped (function name appeared twice); consolidated to top-of-file block returning sentinels directlyDesign / plan docs
doc/2026-05-19-gke-phase2c-edit/Design.md(with the Description-not-editable correction baked in)doc/2026-05-19-gke-phase2c-edit/TODO.md