Add multi-cluster union view with --union-context and --union-set#172
Open
zapman449 wants to merge 22 commits into
Open
Add multi-cluster union view with --union-context and --union-set#172zapman449 wants to merge 22 commits into
zapman449 wants to merge 22 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements a multi-cluster "union" view: new CLI flags and config (union_sets), model/state and sentinel ( Suggested reviewers
|
Author
|
Some large enterprises divide their workloads across multiple clusters. This provides blast-radius protection for cluster level changes. This PR amends lfk to support this pattern. Existing users should see no changes. However there's a new top-level flag That's a bit verbose, so we also support a config driven union-set: The config file entry looks like: The Major use cases are overwhelmingly read-only (changes should be in argo/infra-as-code), but deleting pods and restarting deployments are supported. This change went deeper into the dispatch plumbing than I was happy with, but it does work. Happy to discuss alternative implementation paths or suggestions. The CLI flags used/chosen were picked to avoid existing workflows/patterns and be "net additive". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/events_group.go (1)
64-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing
GroupedRefswhen regrouping Event rows.Line 68 rebuilds
GroupedRefsfrom just the current row, and Line 98 appends onlysrc’s top-level identity. IfgroupEventsever receives already-grouped rows — which the bulk-expansion path explicitly supports for older cached grouped rows — you'll silently drop underlying Event refs and bulk delete will miss part of the group.Proposed fix
clone := cloneEventItem(it) // Seed GroupedRefs with the first item in the group so bulk // operations can expand the grouped row back into individual // delete calls covering every underlying Event object. - clone.GroupedRefs = []model.GroupedRef{{Name: it.Name, Namespace: it.Namespace, ClusterName: it.ClusterName}} + if len(it.GroupedRefs) > 0 { + clone.GroupedRefs = append([]model.GroupedRef(nil), it.GroupedRefs...) + } else { + clone.GroupedRefs = []model.GroupedRef{{Name: it.Name, Namespace: it.Namespace, ClusterName: it.ClusterName}} + } @@ func mergeEventInto(dst *model.Item, src model.Item) { addEventCount(dst, readEventCount(src)) mergeFirstSeen(dst, src) mergeLastSeen(dst, src) - dst.GroupedRefs = append(dst.GroupedRefs, model.GroupedRef{Name: src.Name, Namespace: src.Namespace, ClusterName: src.ClusterName}) + if len(src.GroupedRefs) > 0 { + dst.GroupedRefs = append(dst.GroupedRefs, src.GroupedRefs...) + } else { + dst.GroupedRefs = append(dst.GroupedRefs, model.GroupedRef{Name: src.Name, Namespace: src.Namespace, ClusterName: src.ClusterName}) + } }Also applies to: 94-99
🤖 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/app/events_group.go` around lines 64 - 68, When regrouping Event rows in groupEvents, the code currently replaces clone.GroupedRefs with only the current row identity (in clone := cloneEventItem(it) then clone.GroupedRefs = ...), and later appends only src's top-level identity — which discards any pre-existing grouped refs. Fix by preserving and merging existing GroupedRefs: after clone := cloneEventItem(it) initialize clone.GroupedRefs as a copy of it.GroupedRefs (if non-nil), then ensure you append the current identity {Name: it.Name, Namespace: it.Namespace, ClusterName: it.ClusterName} only if not already present; likewise, in the code that appends src, merge src.GroupedRefs (or src's identity) into dst.GroupedRefs instead of overwriting so all underlying Event refs are retained for bulk expansion.
🧹 Nitpick comments (2)
UNION-CONTEXT.md (2)
99-111: 💤 Low valueAdd language specifier to data flow code block.
This pseudocode/diagram block should have a language identifier for consistency with markdownlint rules.
Proposed fix
-``` +```text watch tick / user navigates to Pods → loadResources(false) → union branch: nav.Context == "__union__" && unionMode🤖 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 `@UNION-CONTEXT.md` around lines 99 - 111, The markdown code block in UNION-CONTEXT.md lacks a language specifier; update the opening fence to include a language (e.g., change ``` to ```text) for the pseudocode block that starts with "watch tick / user navigates to Pods" so that the diagram that includes symbols like nav.Context == "__union__" && unionMode, GetResourcesUnion, GetResources, ClusterName, and itemCache["__union__/pods"] is fenced with a language identifier.
7-9: 💤 Low valueAdd language specifier to fenced code block.
The code block is missing a language identifier, which helps syntax highlighting and accessibility tools.
Proposed fix
-``` +```shell lfk --union-context blue --union-context green --union-context yellow --namespace cloud-cd -``` +```🤖 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 `@UNION-CONTEXT.md` around lines 7 - 9, The fenced code block containing the CLI example (the block with "lfk --union-context blue --union-context green --union-context yellow --namespace cloud-cd") lacks a language specifier; update that fenced block to include a language identifier (e.g., "shell" or "bash") immediately after the opening triple backticks so the block reads as a shell snippet to enable proper syntax highlighting and accessibility tools.
🤖 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 `@docs/usage.md`:
- Line 34: Replace the ambiguous heading "Mutex with --context and
--union-context." in the docs (the heading string "Mutex with --context and
--union-context.") with clearer CLI wording such as "Mutually exclusive with
--context and --union-context." to avoid misreading; update the heading text in
docs/usage.md accordingly so examples and surrounding text remain consistent
with the new phrasing.
In `@internal/app/app_helpers.go`:
- Around line 55-57: In activeContext(), guard the isUnionSentinel() branch so
it never falls through to return the sentinel nav.Context when unionContexts is
empty: inside the existing check of m.isUnionSentinel() use the length of
m.unionContexts and if >0 return m.unionContexts[0], otherwise return a safe
fallback (e.g. empty string) instead of m.nav.Context; update references in
activeContext(), isUnionSentinel(), m.unionContexts and m.nav.Context
accordingly so callers do not receive the invalid "__union__" context.
In `@internal/app/tabs.go`:
- Around line 75-77: Update the sentinel detection in Model.isUnionSentinel to
only treat the union sentinel when the navigator is at the resources level: add
a check that m.nav.Level == model.LevelResources in addition to m.unionMode and
m.nav.Context == UnionContextSentinel so a real kube context named "__union__"
isn't misclassified; adjust the logic in isUnionSentinel (and any callers like
effectiveContext if they rely on this) to use the tightened condition.
In `@internal/k8s/client.go`:
- Around line 329-334: The sort comparator used in the sort.Slice call over the
merged slice only compares Name and ClusterName, so items with identical Name
and ClusterName but different Namespace are treated as equal and may be
reordered; update the comparator used where sort.Slice(merged, func(i, j int)
bool { ... }) is defined to include Namespace as a third key (i.e., compare
Name, then ClusterName, then Namespace) to guarantee stable ordering, and add a
regression test that constructs two entries with the same Name and ClusterName
but different Namespace and asserts their deterministic order after the
merge/refresh operation.
---
Outside diff comments:
In `@internal/app/events_group.go`:
- Around line 64-68: When regrouping Event rows in groupEvents, the code
currently replaces clone.GroupedRefs with only the current row identity (in
clone := cloneEventItem(it) then clone.GroupedRefs = ...), and later appends
only src's top-level identity — which discards any pre-existing grouped refs.
Fix by preserving and merging existing GroupedRefs: after clone :=
cloneEventItem(it) initialize clone.GroupedRefs as a copy of it.GroupedRefs (if
non-nil), then ensure you append the current identity {Name: it.Name, Namespace:
it.Namespace, ClusterName: it.ClusterName} only if not already present;
likewise, in the code that appends src, merge src.GroupedRefs (or src's
identity) into dst.GroupedRefs instead of overwriting so all underlying Event
refs are retained for bulk expansion.
---
Nitpick comments:
In `@UNION-CONTEXT.md`:
- Around line 99-111: The markdown code block in UNION-CONTEXT.md lacks a
language specifier; update the opening fence to include a language (e.g., change
``` to ```text) for the pseudocode block that starts with "watch tick / user
navigates to Pods" so that the diagram that includes symbols like nav.Context ==
"__union__" && unionMode, GetResourcesUnion, GetResources, ClusterName, and
itemCache["__union__/pods"] is fenced with a language identifier.
- Around line 7-9: The fenced code block containing the CLI example (the block
with "lfk --union-context blue --union-context green --union-context yellow
--namespace cloud-cd") lacks a language specifier; update that fenced block to
include a language identifier (e.g., "shell" or "bash") immediately after the
opening triple backticks so the block reads as a shell snippet to enable proper
syntax highlighting and accessibility tools.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 52a5eea5-adf3-47a9-82dd-f9469faf1a69
📒 Files selected for processing (48)
UNION-CONTEXT.mddocs/config-reference.mddocs/usage.mdinternal/app/app.gointernal/app/app_helpers.gointernal/app/app_init.gointernal/app/app_init_cmd.gointernal/app/app_types.gointernal/app/commandbar_execute.gointernal/app/commands_bulk.gointernal/app/commands_dashboard.gointernal/app/commands_load.gointernal/app/commands_load_metrics.gointernal/app/commands_load_preview.gointernal/app/commands_logs.gointernal/app/commands_yaml.gointernal/app/cursor.gointernal/app/discovery_cache.gointernal/app/discovery_cache_test.gointernal/app/events_group.gointernal/app/events_group_test.gointernal/app/options.gointernal/app/readonly.gointernal/app/session.gointernal/app/tabs.gointernal/app/union_test.gointernal/app/update.gointernal/app/update_actions.gointernal/app/update_actions_misc.gointernal/app/update_bookmarks.gointernal/app/update_bookmarks_session.gointernal/app/update_keys.gointernal/app/update_keys_actions.gointernal/app/update_navigation.gointernal/app/update_overlays_confirm.gointernal/app/update_resources_loaded.gointernal/app/view_status.gointernal/k8s/client.gointernal/k8s/client_union_test.gointernal/model/types.gointernal/ui/cluster_color.gointernal/ui/cluster_color_tile_test.gointernal/ui/config.gointernal/ui/config_apply.gointernal/ui/config_load.gointernal/ui/config_union_sets_test.gointernal/ui/explorer_table.gomain.go
zapman449
changed the title
zapman449
force-pushed
the
zapman449/support-multi-cluster-patterns
branch
from
a53d114 to
9922476
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@UNION-CONTEXT.md`:
- Around line 13-19: The CLI docs need additions: add a row documenting the
--union-set flag (usage: --union-set <name>, selects a named set from config),
note the 8-context upper bound validation for --union-context, and add a new
"Config" section describing the union_sets schema (top-level union_sets key,
named entries mapping to contexts which can be strings or objects with name and
optional color). Update UNION-CONTEXT.md to include the new table row for
--union-set, a note about the 8-context cap for --union-context, and a YAML
example showing union_sets with contexts as plain strings or objects including
the optional color field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5707b85-46fc-45cc-859d-219b2989ee69
📒 Files selected for processing (8)
UNION-CONTEXT.mddocs/usage.mdinternal/app/app_helpers.gointernal/app/events_group.gointernal/app/events_group_test.gointernal/app/union_test.gointernal/k8s/client.gointernal/k8s/client_union_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/usage.md
- internal/k8s/client.go
- internal/app/events_group.go
- internal/app/union_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/explorer_table.go (1)
234-234:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
collectExtraColumnsmissingtileWin the used-width budget.The third argument to
collectExtraColumnsis the width already consumed by fixed columns.tileWis excluded here, so in union mode the function treats 1 extra character as available for extra columns.nameW(line 289) does subtracttileW, so the overall row width stays correct—but the Name column is effectively 1 character narrower than intended when extra columns are present, because the extra columns absorbed the byte that belongs to the tile cell.Proposed fix
- extraCols = collectExtraColumns(items, width, nsW+readyW+restartsW+ageW+statusW+markerColW, tableKind) + extraCols = collectExtraColumns(items, width, nsW+readyW+restartsW+ageW+statusW+markerColW+tileW, tableKind)🤖 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/ui/explorer_table.go` at line 234, The call to collectExtraColumns is missing tileW in the "used width" budget, so add tileW to the sum passed as the third argument; specifically update the call that currently reads collectExtraColumns(items, width, nsW+readyW+restartsW+ageW+statusW+markerColW, tableKind) to include +tileW so the used-width accurately accounts for the tile cell (this aligns with the subtraction done in nameW and prevents extra columns from stealing one character from the Name column in union mode).
🧹 Nitpick comments (1)
internal/app/options.go (1)
29-35: ⚡ Quick winTreat
--union-setas a CLI override too.Right now this only flips after
ResolveUnionSethas populatedUnionContexts. AddingUnionSethere would make the method match its own comment and avoid coupling override detection to resolution order.Possible fix
func (o StartupOptions) HasCLIOverrides() bool { - return o.Context != "" || len(o.Namespaces) > 0 || len(o.UnionContexts) > 0 + return o.Context != "" || o.UnionSet != "" || len(o.Namespaces) > 0 || len(o.UnionContexts) > 0 }🤖 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/app/options.go` around lines 29 - 35, HasCLIOverrides currently ignores the raw --union-set flag and only detects overrides after ResolveUnionSet populates UnionContexts; update StartupOptions.HasCLIOverrides to also consider the union-set field (check o.UnionSet for a non-empty value) so the method returns true when the user passed --union-set, avoiding coupling override detection to ResolveUnionSet and matching the method comment (refer to StartupOptions.HasCLIOverrides, StartupOptions.UnionSet and StartupOptions.UnionContexts).
🤖 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 `@internal/app/update_actions.go`:
- Around line 55-62: The overlay filtering currently uses the global m.readOnly
when skipping mutating actions (in the loop over actions building items,
referencing variables/actions like items, actions, m.readOnly, and
isMutatingAction), which allows mutating actions to be shown for per-context
read-only clusters; update the checks to use the effective read-only state
instead: call readOnlyForContext(m.actionCtx.context) when building
single-resource menus and bulkReadOnlyContext() for bulk menus, replacing
m.readOnly in the mutating-action guard so the visible action list matches what
can actually run; also apply the same change to the other similar loop (the
actions handling around the isUnionSentinel/isUnionAllowedAction logic
referenced in the diff).
In `@internal/app/update_bookmarks_session.go`:
- Around line 21-29: The union-mode restore only sets UnionContextSentinel on
the returned restoredModel.nav.Context, leaving other saved tabs in m.tabs with
real cluster contexts; update the union-mode branch in restoreMultiTabSession
(or the function containing the shown block) to iterate m.tabs (the slice/map
populated by restoreMultiTabSession) and set each tab's nav.Context to
UnionContextSentinel when m.unionMode is true, so that background tabs also pass
isUnionSentinel() and loadResources fans out correctly; keep the existing check
for restoredModel.(Model) and still return the active tab model after fixing all
tabs.
In `@internal/app/update_resources_loaded.go`:
- Around line 279-288: When hydrating cached resource types at
LevelResourceTypes the code currently uses m.nav.Context (the union sentinel) to
look up m.discoveredResources, which misses entries stored under the real
discovery context; change the lookup to use the actual discovery context (e.g.
if m.unionContexts slice is non-empty use m.unionContexts[0], otherwise fall
back to m.nav.Context) when building merged := model.BuildSidebarItems(entries),
updating m.itemCache[...] and calling m.setMiddleItems(...), so union-mode
preloads correctly replace the seeded sidebar.
In `@UNION-CONTEXT.md`:
- Line 93: The table row currently points to internal/app/update.go but the
implementations of updateAPIResourceDiscovery and updateResourcesLoadedMain are
in internal/app/update_resources_loaded.go; update that table entry to reference
internal/app/update_resources_loaded.go so readers can find the implementations
of the functions updateAPIResourceDiscovery and updateResourcesLoadedMain.
---
Outside diff comments:
In `@internal/ui/explorer_table.go`:
- Line 234: The call to collectExtraColumns is missing tileW in the "used width"
budget, so add tileW to the sum passed as the third argument; specifically
update the call that currently reads collectExtraColumns(items, width,
nsW+readyW+restartsW+ageW+statusW+markerColW, tableKind) to include +tileW so
the used-width accurately accounts for the tile cell (this aligns with the
subtraction done in nameW and prevents extra columns from stealing one character
from the Name column in union mode).
---
Nitpick comments:
In `@internal/app/options.go`:
- Around line 29-35: HasCLIOverrides currently ignores the raw --union-set flag
and only detects overrides after ResolveUnionSet populates UnionContexts; update
StartupOptions.HasCLIOverrides to also consider the union-set field (check
o.UnionSet for a non-empty value) so the method returns true when the user
passed --union-set, avoiding coupling override detection to ResolveUnionSet and
matching the method comment (refer to StartupOptions.HasCLIOverrides,
StartupOptions.UnionSet and StartupOptions.UnionContexts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 552f4c08-56c9-4480-8b20-eeb52ec9ead6
📒 Files selected for processing (48)
UNION-CONTEXT.mddocs/config-reference.mddocs/usage.mdinternal/app/app.gointernal/app/app_helpers.gointernal/app/app_init.gointernal/app/app_init_cmd.gointernal/app/app_types.gointernal/app/commandbar_execute.gointernal/app/commands_bulk.gointernal/app/commands_dashboard.gointernal/app/commands_load.gointernal/app/commands_load_metrics.gointernal/app/commands_load_preview.gointernal/app/commands_logs.gointernal/app/commands_yaml.gointernal/app/cursor.gointernal/app/discovery_cache.gointernal/app/discovery_cache_test.gointernal/app/events_group.gointernal/app/events_group_test.gointernal/app/options.gointernal/app/readonly.gointernal/app/session.gointernal/app/tabs.gointernal/app/union_test.gointernal/app/update.gointernal/app/update_actions.gointernal/app/update_actions_misc.gointernal/app/update_bookmarks.gointernal/app/update_bookmarks_session.gointernal/app/update_keys.gointernal/app/update_keys_actions.gointernal/app/update_navigation.gointernal/app/update_overlays_confirm.gointernal/app/update_resources_loaded.gointernal/app/view_status.gointernal/k8s/client.gointernal/k8s/client_union_test.gointernal/model/types.gointernal/ui/cluster_color.gointernal/ui/cluster_color_tile_test.gointernal/ui/config.gointernal/ui/config_apply.gointernal/ui/config_load.gointernal/ui/config_union_sets_test.gointernal/ui/explorer_table.gomain.go
✅ Files skipped from review due to trivial changes (5)
- internal/app/app_types.go
- internal/app/commands_dashboard.go
- docs/usage.md
- internal/app/readonly.go
- internal/app/commands_load.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/app/update_keys_actions.go
- internal/app/update_bookmarks.go
- internal/ui/cluster_color.go
- internal/app/app_init_cmd.go
- internal/app/cursor.go
- internal/app/discovery_cache_test.go
- internal/app/app_init.go
- internal/app/tabs.go
- internal/ui/config_union_sets_test.go
- internal/app/commands_logs.go
- internal/app/discovery_cache.go
- internal/app/update_actions_misc.go
- internal/app/union_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
UNION-CONTEXT.md (1)
18-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe union-set docs still don't match the public contract.
The table still documents
--contextas only conflicting with--union-context, and the YAML example modelsunion_setsas a sequence with an inlinenamefield. This feature is described as--union-set <name>selecting a named set, so users can copy this example and end up with a config that the lookup never resolves. Please document the--context/--union-setmutual exclusion and show the actual named-set shape.Example adjustment
-| `--context` | mutually exclusive with `--union-context` | Returns an error if both are supplied. | +| `--context` | mutually exclusive with `--union-context` and `--union-set` | Returns an error if both are supplied. | union_sets: - - name: staging-west - namespace: cloud-cd - contexts: - - blue - - green - - context: yellow - color: yellow + staging-west: + namespace: cloud-cd + contexts: + - blue + - green + - context: yellow + color: yellowAlso applies to: 24-37
🤖 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 `@UNION-CONTEXT.md` around lines 18 - 20, Update the docs to reflect the actual public contract: mark --context and --union-set (not just --union-context) as mutually exclusive and require --namespace when using --union-set unless the named union_sets entry supplies a default; change the union_sets example from an array of inline name fields to the real named-set structure (e.g., a mapping of union_set names to their contexts and optional default namespace) so that the --union-set <name> lookup will resolve correctly; ensure the table rows for `--union-set`, `--namespace`/`-n`, and `--context` clearly state these constraints and update corresponding YAML example to use the named key shape used by the code.
🤖 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 `@internal/app/update_actions.go`:
- Around line 57-63: The union-mode checks currently only inspect action.Label
(see uses of isUnionAllowedAction(a.Label) and m.isUnionSentinel()), which is
too coarse; update the allowlist logic to be kind-aware and conservative by
default: replace calls to isUnionAllowedAction(a.Label) with a new helper like
isUnionAllowed(kind, actionLabel) that takes the resource kind and action label
(use the same helper in executeAction() and executeBulkAction()), have
m.isUnionSentinel() gate on !isUnionAllowed(kind, label) to block unknown/custom
actions by default, and ensure mutating filtering still uses (kind, action)
where applicable so union mode cannot perform writes on unrelated kinds.
In `@internal/app/update_bookmarks_session.go`:
- Around line 24-29: Restored union-mode tabs are being marked with
UnionContextSentinel (tm.nav.Context = UnionContextSentinel) but loadTab() still
uses m.discoveredResources[m.nav.Context], so background tabs at
LevelResourceTypes fall back to SeedResources(); update loadTab() so when the
tab's nav.Context (or m.nav.Context during reload) equals UnionContextSentinel
it looks up discovered resources from the appropriate union context (e.g.
unionContexts[0]) instead of using the sentinel key—use that discoveredResources
entry for rebuilding the sidebar and avoid calling SeedResources() for
union-restored tabs.
---
Duplicate comments:
In `@UNION-CONTEXT.md`:
- Around line 18-20: Update the docs to reflect the actual public contract: mark
--context and --union-set (not just --union-context) as mutually exclusive and
require --namespace when using --union-set unless the named union_sets entry
supplies a default; change the union_sets example from an array of inline name
fields to the real named-set structure (e.g., a mapping of union_set names to
their contexts and optional default namespace) so that the --union-set <name>
lookup will resolve correctly; ensure the table rows for `--union-set`,
`--namespace`/`-n`, and `--context` clearly state these constraints and update
corresponding YAML example to use the named key shape used by the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33d9860c-95cd-4b08-aa6a-a514c43d51d6
📒 Files selected for processing (8)
UNION-CONTEXT.mdinternal/app/options.gointernal/app/tabs.gointernal/app/union_test.gointernal/app/update_actions.gointernal/app/update_bookmarks_session.gointernal/app/update_resources_loaded.gointernal/ui/explorer_table.go
✅ Files skipped from review due to trivial changes (1)
- internal/app/update_resources_loaded.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/app/readonly.go`:
- Around line 152-171: bulkReadOnlyContext currently falls back to
m.actionCtx.context when item.ClusterName is empty which lets the union sentinel
("__union__") bypass readOnlyForContext; fix bulkReadOnlyContext so the loop
does NOT use m.actionCtx.context when item.ClusterName is empty and
m.actionCtx.context == "__union__": instead attempt to derive the cluster from
the item's metadata (e.g., a metadata cluster field on the item) and call
readOnlyForContext with that derived cluster, or if no cluster can be derived,
treat the item as read-only (return the union-safe context and true) or continue
to the next item; update the loop in Model.bulkReadOnlyContext to prefer
item.ClusterName, then derived metadata cluster, and only use
m.actionCtx.context when it is a real cluster (not "__union__"), calling
readOnlyForContext accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69f5d7d5-3cd5-4d80-a4dd-0b98dd19d6c0
📒 Files selected for processing (4)
internal/app/readonly.gointernal/app/tabs.gointernal/app/union_test.gointernal/app/update_actions.go
✅ Files skipped from review due to trivial changes (1)
- internal/app/update_actions.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/app/union_test.go
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
zapman449
force-pushed
the
zapman449/support-multi-cluster-patterns
branch
from
f06b346 to
179c5fa
Compare
Owner
|
Hi @zapman449 , Thanks for bringing this up. I really like this feature idea, but it'll need a lot of work to be coherent and straightforward. Just to mention a few features that needs additional polish to support:
And a few UX decisions I'd prefer to have:
|
zapman449
force-pushed
the
zapman449/support-multi-cluster-patterns
branch
from
179c5fa to
9590b02
Compare
Author
|
Ack. I'll try to get those sorted into something reasonable. Please let me know if you have input on what any of these should look like. The first parts of it are pushed (along with a rebase off latest), a few more things coming to address specific pieces. |
zapman449
force-pushed
the
zapman449/support-multi-cluster-patterns
branch
2 times, most recently
from
7e78d84 to
8b90530
Compare
Author
Features needing attention:
UX decisions:
Major Notes:
|
zapman449
force-pushed
the
zapman449/support-multi-cluster-patterns
branch
2 times, most recently
from
8d9d152 to
3075fdf
Compare
…union-set
- New --union-context flag (repeatable, capped at 8 clusters) and
--union-set <name> for named sets in config; mutually exclusive with
--context.
- Per-cluster color tiles via union_sets[].contexts[].color, rendered as
a 1-cell leading swatch in the merged table.
- Allow Pod delete / force-delete / force-finalize and workload Restart
at the merged level; everything else mutating stays blocked there.
- Cluster-aware cursor restoration, multi-select keys, and bulk dispatch
route per-row by ClusterName so mixed-cluster selections target the
correct apiserver per item.
- Async discovery-cache preload so kubeconfigs with thousands of
contexts (block-sre-operator-style) don't block startup.
Some large enterprises divide their workloads across multiple clusters.
This provides blast-radius protection for cluster level changes.
This PR amends lfk to support this pattern. Existing users should see
no changes. However there's a new top-level flag `--union-context`.
Users would specify this multiple times, once per cluster. Example:
```
lfk \
--union-context foo-staging-green-us-west-2 \
--union-context foo-staging-yellow-us-west-2 \
--union-context foo-staging-blue-us-west-2 \
--namespace baz
```
That's a bit verbose, so we also support a config driven union-set:
```
lfk --union-set staging-west --namespace foo
```
The config file entry looks like:
```
union_sets:
- name: foo-staging-west
contexts:
- context: operator/biz-sre-operator/foo-staging-green-us-west-2
color: green
- context: operator/biz-sre-operator/foo-staging-yellow-us-west-2
color: yellow
- context: operator/biz-sre-operator/foo-staging-blue-us-west-2
color: blue
- name: bar-staging-west
contexts:
- context: operator/biz-sre-operator/bar-staging-cell-01-us-west-2
color: blue
- context: operator/biz-sre-operator/bar-staging-cell-02-us-west-2
color: green
```
The `color: X` bit places a 1 character background color space to indicate which cluster owns which resource (useful for pods or replicasets, etc). It is optional. You can simplify the config for `contexts:` being a list of strings.
Major use cases are overwhelmingly read-only (changes should be in argo/infra-as-code), but deleting pods and restarting deployments are supported.
This change went deeper into the dispatch plumbing than I was happy with, but it does work. Happy to discuss alternative implementation paths or suggestions. The CLI flags used/choosen were picked to avoid existing workflows/patterns and be "net additive".
…se does not make sense, so enumerate all in current union set, and allow drill into each to get these views
…ed unionsets in picker
"can-i" view has a new item: yellow `?` which indicates all the contexts in the unionset disagree as to whether this action is allowed. I left reverse Who-Can blocked in union mode for now, because “who can do X across clusters” needs separate semantics and output layout.
…lit oversized files Rebasing onto main pulled in three changes that needed local adaptation: * k8s.NewClient gained a kubeconfigDirs argument; updated completion.go callers to pass it through. * commandbar_execute.go grew past the 800-line limit; extracted the kubectl argument helpers (commandAffectsNamespaces, injectKubectlDefaults, commandSupportsNamespaceFlags, kubectlGlobalFlagsWithValue, firstNonFlagToken) to commandbar_execute_kubectl_args.go. * update_overlays_selectors.go grew past the 800-line limit; extracted the colorscheme overlay handlers to update_overlays_colorscheme.go. No behavior change.
…luster bulkScaleResources and batchPatchLabels both used the loop-invariant actionCtx for every iteration, unlike bulkDeleteResources/bulkRestartResources which already routed per item.ClusterName. This path is currently unreachable from the UI — isUnionAllowedActionForKind excludes Scale and Labels/Annotations from the union allow-list. The fix is defensive: it preserves the documented per-row routing contract in model.Item.ClusterName so a future allow-list relaxation does not silently misroute scale or label/annotation mutations across mixed-cluster selections. Also adds an info log to bulk patch operations matching the pattern of the other bulk paths so the per-row context is observable in logs. Tests: assert per-row routing via captured slog output.
Client.secretLazyLoading and Client.kubesharkNamespaceOverride were written by SetSecretLazyLoading/SetKubesharkNamespace at startup and read from tea.Cmd goroutines (GetResources, kubesharkNamespace) without synchronization. `go test -race` flagged the kubeshark write/read pair as a genuine data race once concurrent readers ran while the setter fired — a path the new async discovery cache preload now opens at Init() time. Convert both fields to atomic primitives (atomic.Bool, atomic.Pointer[string]) so the read/write pairs are always race-free, including any future runtime-config-reload path. Adds a -race test covering both setters under concurrent reads.
…dialogs Two related signalling gaps: 1. Bulk Restart fired immediately on the action shortcut, unlike Delete and Force Delete which already prompt for confirmation. In union mode a single keystroke could issue `kubectl rollout restart` across every cluster touched by the selection. Route through overlayConfirm and dispatch via pendingAction == "Restart" in handleConfirmOverlayKey. 2. Bulk confirm prompts (Delete, Force Delete, Restart) named only the resource count, not the source clusters. A user who believes they are operating on one cluster could mistake a 5-resource bulk delete that spans 3 clusters for a single-cluster operation. Append a sorted, deduplicated cluster list to the confirm prompt in union mode; single-cluster prompts are unchanged. Updates the existing coverage-style TestCovExecuteBulkActionRestart to reflect the new confirm-first contract.
ExpandUnionSetConfig silently kept the first non-empty per-member namespace and discarded every subsequent one. A set with members declaring different namespaces (e.g. dev with dev-ns plus prod with payments) would resolve to the first one read, with no indication that the others were ignored. Multi-cluster sets with divergent namespaces would silently use the wrong namespace for every context after the first. Adds an error return on the function and rejects sets where members declare conflicting non-empty namespaces with a message naming the offending values. Sets with no per-member namespaces, or sets where every member agrees on the same namespace, are unaffected. Updates all seven call sites (CLI lookup in main, picker dispatch in union_sets.go, bookmark resolution in update_bookmarks.go, picker withUnionSetRows, navigateChildUnionSet, activateUnionSet, and the table-driven test in union_test.go).
loadAllDiscoveryCaches walked every kubeconfig context to completion regardless of session state. A user quitting during preload of a 1200-context kubeconfig had to wait for the full walk before the tea program exited. Plumb m.reqCtx into discoveryCachePreloadCmd and check it at the top of every iteration. The function returns whatever it has loaded so far when cancelled, so the message handler still merges a partial cache rather than discarding work.
isUnionAllowedActionForKind is the gate that decides which mutating actions remain reachable from the union sentinel. The function has a default: return false branch, so any new action label silently slips into the blocked set if the author forgets to consider it — which is safe behaviour but invisible to the author. The new test enumerates every label in mutatingActions and asserts the union-mode policy expected for it across a representative kind matrix. A new mutating action without a corresponding row fails the test with the offending label, forcing the author to make an explicit decision. The same check runs in reverse: a stale row without a backing mutatingActions entry also fails.
Three places swallowed errors that masked real failures: * commands_bulk.go: bulkForceDeleteResources ran a kubectl patch to clear finalizers, then ignored the patch's exit status with //nolint:errcheck. A patch failure (RBAC denial, resource gone) silently fell through to the --force delete, which would then appear to hang on the user side. Now logs a Warn including the resource, namespace, and context so the trail survives in lfk.log. * update_bookmarks.go and update_resources_loaded.go: contexts, _ := m.client.GetContexts() discarded the error and built navigation state from a nil slice on failure. Logs a Warn instead so a kubeconfig reload race is observable. Extract a tiny contextsOrEmpty helper (placed in app_helpers.go to keep update_resources_loaded.go under the file-length limit). * internal/k8s/client.go: GetResourcesUnion returned only the first per-context error and silently dropped the rest. Two of eight contexts failing showed only one failure in the status bar. Aggregate with errors.Join so callers can render the full set.
…tinel, document Model state invariant Three hygiene items from the review: * navigateToBookmark ballooned to ~187 lines after the bookmark union work. Extract three helpers (applyBookmarkContextSwitch, applyBookmarkNamespace, rebuildLeftHistoryForBookmark) into update_bookmarks_navigate.go so the main function reads as a flat sequence of phases and stays under the gocyclo/length caps. * processCanIRules (the single-context RBAC path) indexed discoveredResources by m.nav.Context, which is the union sentinel in union mode. The normal flow dispatches to processCanIRulesUnion before reaching here, but a future caller forgetting to do so would silently return an empty rules table. Add a sentinel guard that logs the misdispatch and bails out. * Document the inline invariant on Model's per-context maps so future maintainers don't retrofit an unnecessary RWMutex.
janosmiko
force-pushed
the
zapman449/support-multi-cluster-patterns
branch
from
3075fdf to
605aea7
Compare
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
2 participants
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
Support showing resources from several clusters/contexts at once.
Type of change
Related issue
Changes
--union-set for named sets in config; mutually exclusive with
--context.
a 1-cell leading swatch in the merged table.
at the merged level; everything else mutating stays blocked there.
route per-row by ClusterName so mixed-cluster selections target the
correct apiserver per item.
contexts (block-sre-operator-style) don't block startup.
Testing
go build ./...succeedsgo test ./...passesgolangci-lint runpassesChecklist
feat:,fix:,refactor:,docs:,test:,chore:,perf:,ci:)docs/updated when user-visible behavior or config changeddocs/keybindings.mdand the in-app help screen updated when keybindings changedmain)Screenshots / recordings