Enable workspace resolver to find nested workspaces#54
Open
Perseus985 wants to merge 3 commits intomainfrom
Open
Enable workspace resolver to find nested workspaces#54Perseus985 wants to merge 3 commits intomainfrom
Perseus985 wants to merge 3 commits intomainfrom
Conversation
Signed-off-by: Kevin Stellmacher <dev@stellmacher.xyz>
📝 WalkthroughWalkthroughThe workspace resolver mechanism is refactored to use Reconciler.BaseConfig directly, store a front-proxy baseCfg, and resolve workspace paths by deriving parent logicalcluster paths, creating per-parent REST clients, listing Workspace objects under each parent, validating ws.Spec.Cluster, and caching parentPath:workspaceName → cluster mappings. ChangesWorkspace Resolver Parent-Based Discovery
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/controller/dependencyrule_controller.go (2)
193-196: ⚡ Quick winRemove commented-out dead code.
Lines 193–196 still contain the old client-construction snippet. Dead commented-out code should not be included in a merged PR.
🧹 Proposed cleanup
- // c, err := client.New(w.rootCfg, client.Options{Scheme: w.scheme}) - // if err != nil { - // return fmt.Errorf("creating root workspace client: %w", err) - // } for _, path := range missing {🤖 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/controller/dependencyrule_controller.go` around lines 193 - 196, Remove the dead commented-out client construction snippet (the block containing client.New, client.Options, w.rootCfg and w.scheme) from dependencyrule_controller.go; locate the commented lines that begin with "// c, err := client.New(...)" and delete that entire commented block so no commented-out client creation code remains in the PR.
197-228: ⚡ Quick winMultiple
ListAPI calls issued for the same parent when several sibling paths are missing.The outer loop iterates every entry in
missingindividually. Ifroot:org:ws1,root:org:ws2, androot:org:ws3are all absent from the cache, three separate clients are built and threeListcalls are sent to the sameroot:orgparent endpoint. The inner cache check on line 198 only short-circuits if an exact path match was already cached (which it never is for a freshly listed sibling), so it does not prevent the redundant calls.Group
missingpaths byparentPathfirst, then issue oneListper distinct parent:♻️ Proposed refactor
- for _, path := range missing { - if _, ok := w.cache[path]; ok { - continue - } - - var parentPath string - if i := strings.LastIndex(path, ":"); i >= 0 { - parentPath = path[:i] - } - - parentCfg := rest.CopyConfig(w.baseCfg) - parentCfg.Host += logicalcluster.NewPath(parentPath).RequestPath() - - c, err := client.New(parentCfg, client.Options{Scheme: w.scheme}) - if err != nil { - return fmt.Errorf("creating %s workspace client: %w", parentPath, err) - } - - var wsList tenancyv1alpha1.WorkspaceList - if err := c.List(ctx, &wsList); err != nil { - return fmt.Errorf("list workspaces in %s: %w", parentPath, err) - } - - for _, ws := range wsList.Items { - if ws.Spec.Cluster == "" { - return fmt.Errorf("workspace %s has no logical cluster name", ws.Name) - } - - wsPath := parentPath + ":" + ws.Name - w.cache[wsPath] = ws.Spec.Cluster - log.FromContext(ctx).Info("resolved workspace path", "path", wsPath, "cluster", ws.Spec.Cluster) - } - } + // Group missing paths by their parent to avoid redundant List calls. + byParent := make(map[string]struct{}) + for _, path := range missing { + if _, ok := w.cache[path]; ok { + continue + } + i := strings.LastIndex(path, ":") + if i < 0 { + return fmt.Errorf("workspace path %q has no parent segment", path) + } + byParent[path[:i]] = struct{}{} + } + + for parentPath := range byParent { + parentCfg := rest.CopyConfig(w.baseCfg) + parentCfg.Host += logicalcluster.NewPath(parentPath).RequestPath() + + c, err := client.New(parentCfg, client.Options{Scheme: w.scheme}) + if err != nil { + return fmt.Errorf("creating %s workspace client: %w", parentPath, err) + } + + var wsList tenancyv1alpha1.WorkspaceList + if err := c.List(ctx, &wsList); err != nil { + return fmt.Errorf("list workspaces in %s: %w", parentPath, err) + } + + for _, ws := range wsList.Items { + if ws.Spec.Cluster == "" { + log.FromContext(ctx).Info("skipping workspace with no cluster (still provisioning?)", "workspace", ws.Name) + continue + } + wsPath := parentPath + ":" + ws.Name + w.cache[wsPath] = ws.Spec.Cluster + log.FromContext(ctx).Info("resolved workspace path", "path", wsPath, "cluster", ws.Spec.Cluster) + } + }🤖 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/controller/dependencyrule_controller.go` around lines 197 - 228, The loop over missing issues redundant List calls for the same parentPath; change the logic to first group missing paths by their parentPath (compute parentPath via strings.LastIndex like current code), then iterate each distinct parentPath once: build parentCfg (rest.CopyConfig + logicalcluster.NewPath(parentPath).RequestPath()), create client.New(parentCfg, ...), call c.List(ctx, &wsList) once per parent, and populate w.cache for each child wsPath (parentPath + ":" + ws.Name) from wsList.Items; keep the existing checks/logging (w.cache lookup, ws.Spec.Cluster emptiness, and log.FromContext) but remove per-missing-path client creation and List calls so only one List happens per unique parentPath.
🤖 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/controller/dependencyrule_controller.go`:
- Around line 221-223: The early return in ensureResolved when ws.Spec.Cluster
== "" should be changed to skip the not-yet-provisioned workspace instead of
failing the whole reconciliation: replace the hard error return in the block
that checks ws.Spec.Cluster (inside ensureResolved / the workspace iteration)
with a processLogger or logger.Warnf message noting the workspace is not ready
and continue to the next workspace, ensuring the function does not abort
resolution for other paths; keep the check but make it non-fatal so other
caching/resolution work proceeds.
- Around line 202-205: The code builds parentPath using strings.LastIndex(path,
":") which yields -1 for single-segment workspace paths (e.g., "root"), leaving
parentPath empty and causing cache keys like ":child". Update the parentPath
logic in the block that sets parentPath (the variable using strings.LastIndex)
so that when LastIndex returns -1 you assign parentPath = path (instead of
leaving it empty); this ensures cache entries use "root:child" and resolve(path)
can find single-segment workspace entries correctly.
---
Nitpick comments:
In `@internal/controller/dependencyrule_controller.go`:
- Around line 193-196: Remove the dead commented-out client construction snippet
(the block containing client.New, client.Options, w.rootCfg and w.scheme) from
dependencyrule_controller.go; locate the commented lines that begin with "// c,
err := client.New(...)" and delete that entire commented block so no
commented-out client creation code remains in the PR.
- Around line 197-228: The loop over missing issues redundant List calls for the
same parentPath; change the logic to first group missing paths by their
parentPath (compute parentPath via strings.LastIndex like current code), then
iterate each distinct parentPath once: build parentCfg (rest.CopyConfig +
logicalcluster.NewPath(parentPath).RequestPath()), create client.New(parentCfg,
...), call c.List(ctx, &wsList) once per parent, and populate w.cache for each
child wsPath (parentPath + ":" + ws.Name) from wsList.Items; keep the existing
checks/logging (w.cache lookup, ws.Spec.Cluster emptiness, and log.FromContext)
but remove per-missing-path client creation and List calls so only one List
happens per unique parentPath.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a531908-919b-4987-ba22-9d0c386c46d3
📒 Files selected for processing (1)
internal/controller/dependencyrule_controller.go
…nit testing Signed-off-by: Cyrill Berg <cyrill.berg@opendefense.cloud>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/controller/dependencyrule_controller_test.go (2)
159-168: ⚡ Quick winConsider also covering an empty (non-nil) slice —
niland[]string{}may exercise different code paths.The production code likely has
if len(paths) == 0 { return nil }, which handles both. But if the guard isif paths == nil, an empty slice would still trigger client creation. A one-liner addition eliminates the ambiguity:💡 Suggested addition
if err := r.ensureResolved(context.Background(), nil); err != nil { t.Fatalf("ensureResolved: %v", err) } +if err := r.ensureResolved(context.Background(), []string{}); err != nil { + t.Fatalf("ensureResolved with empty slice: %v", err) +}🤖 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/controller/dependencyrule_controller_test.go` around lines 159 - 168, Add a second assertion to TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop that calls r.ensureResolved with an empty non-nil slice (e.g., []string{}) to ensure the code treats []string{} the same as nil; use the same newTestResolver callback that fails the test if a client is created and assert that ensureResolved returns nil (no error) for the empty slice as well, referencing TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop, newTestResolver, and ensureResolved.
154-156: ⚡ Quick win
len(calls)only checks distinct host count, not per-host call count — double-listing would go unnoticed.If
ensureResolvedmistakenly callsnewClienttwice for the same parent (e.g., once per child path),calls["root:org-a"]would be 2 butlen(calls)would still be 2, passing the assertion. Consider asserting the per-host count:💡 Suggested assertion addition
if len(calls) != 2 { t.Errorf("expected exactly 2 distinct hosts called, got %v", calls) } +for host, n := range calls { + if n != 1 { + t.Errorf("host %q called %d times, want 1 (parent should be listed once)", host, n) + } +}🤖 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/controller/dependencyrule_controller_test.go` around lines 154 - 156, The test currently only asserts len(calls) == 2 which misses duplicate calls to the same host; update the test around ensureResolved/newClient to also assert each host was called exactly once by checking calls["root:org-a"] == 1 and calls["root:org-b"] == 1 (with clear error messages), so the test fails if ensureResolved invokes newClient multiple times for the same parent.
🤖 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/controller/dependencyrule_controller_test.go`:
- Line 15: The test is using scheme.Scheme which lacks KCP tenancy types
(tenancyv1alpha1.Workspace/WorkspaceList), causing
fake.NewClientBuilder().WithScheme(...).WithLists(...) to fail; create a
dedicated test scheme (e.g., call runtime.NewScheme()), register core Kubernetes
types and KCP tenancy types by calling corev1.AddToScheme or
clientgoscheme.AddToScheme and tenancyv1alpha1.AddToScheme (reference
tenancyv1alpha1.Workspace and WorkspaceList), then replace scheme.Scheme in
fake.NewClientBuilder().WithScheme(...) calls with this new scheme and import
the tenancyv1alpha1 package and runtime/client-go scheme helpers as needed so
TestWorkspaceResolver_EnsureResolved_ResolvesPath and the related tests can
build and run.
---
Nitpick comments:
In `@internal/controller/dependencyrule_controller_test.go`:
- Around line 159-168: Add a second assertion to
TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop that calls
r.ensureResolved with an empty non-nil slice (e.g., []string{}) to ensure the
code treats []string{} the same as nil; use the same newTestResolver callback
that fails the test if a client is created and assert that ensureResolved
returns nil (no error) for the empty slice as well, referencing
TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop, newTestResolver, and
ensureResolved.
- Around line 154-156: The test currently only asserts len(calls) == 2 which
misses duplicate calls to the same host; update the test around
ensureResolved/newClient to also assert each host was called exactly once by
checking calls["root:org-a"] == 1 and calls["root:org-b"] == 1 (with clear error
messages), so the test fails if ensureResolved invokes newClient multiple times
for the same parent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d67154e-c535-4dbe-8ad8-00cff31bb2d9
📒 Files selected for processing (5)
.golangci.yaml.yamllint.yamlinternal/controller/dependencyrule_controller.gointernal/controller/dependencyrule_controller_test.gotest/e2e/suite_test.go
💤 Files with no reviewable changes (1)
- .golangci.yaml
✅ Files skipped from review due to trivial changes (1)
- .yamllint.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/dependencyrule_controller.go
… fail hard on unready workspaces Signed-off-by: Cyrill Berg <cyrill.berg@opendefense.cloud>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/dependencyrule_controller_test.go (2)
163-165: ⚡ Quick winAssert per-host call counts, not only distinct hosts.
Line 163 currently checks only
len(calls) == 2, which still passes if each host is called multiple times. Add exact count assertions per expected host to lock in the intended one-list-per-parent behavior.Suggested test hardening
- if len(calls) != 2 { - t.Errorf("expected exactly 2 distinct hosts called, got %v", calls) - } + if len(calls) != 2 { + t.Errorf("expected exactly 2 distinct hosts called, got %v", calls) + } + if calls[baseHost+logicalcluster.NewPath("root:org-a").RequestPath()] != 1 { + t.Errorf("expected exactly 1 call for root:org-a host, got %d", calls[baseHost+logicalcluster.NewPath("root:org-a").RequestPath()]) + } + if calls[baseHost+logicalcluster.NewPath("root:org-b").RequestPath()] != 1 { + t.Errorf("expected exactly 1 call for root:org-b host, got %d", calls[baseHost+logicalcluster.NewPath("root:org-b").RequestPath()]) + }🤖 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/controller/dependencyrule_controller_test.go` around lines 163 - 165, The current test only asserts len(calls) == 2 which allows multiple hits per host; update the assertion to verify per-host call counts by building a frequency map from the calls slice and asserting that each expected host (the specific host strings used in this test) has count == 1; modify the assertion near the len(calls) check in dependencyrule_controller_test.go to create a map[string]int of counts for the calls variable and assert exact counts for each host instead of only checking distinct host count.
168-176: ⚡ Quick winCover empty slice explicitly in no-op test.
Line 174 validates
nil, but not[]string{}. Since behavior is documented as nil/empty no-op, add the empty-slice assertion too.Suggested test extension
if err := r.ensureResolved(context.Background(), nil); err != nil { t.Fatalf("ensureResolved: %v", err) } + if err := r.ensureResolved(context.Background(), []string{}); err != nil { + t.Fatalf("ensureResolved(empty): %v", err) + }🤖 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/controller/dependencyrule_controller_test.go` around lines 168 - 176, Update the TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop test to also assert that an empty slice is a no-op: add a second call to r.ensureResolved(context.Background(), []string{}) and check it returns no error (just like the existing nil case) while still ensuring newTestResolver's client factory is not invoked; modify only the TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop test (referencing r.ensureResolved, newTestResolver) to include this extra assertion.
🤖 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.
Nitpick comments:
In `@internal/controller/dependencyrule_controller_test.go`:
- Around line 163-165: The current test only asserts len(calls) == 2 which
allows multiple hits per host; update the assertion to verify per-host call
counts by building a frequency map from the calls slice and asserting that each
expected host (the specific host strings used in this test) has count == 1;
modify the assertion near the len(calls) check in
dependencyrule_controller_test.go to create a map[string]int of counts for the
calls variable and assert exact counts for each host instead of only checking
distinct host count.
- Around line 168-176: Update the
TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop test to also assert that
an empty slice is a no-op: add a second call to
r.ensureResolved(context.Background(), []string{}) and check it returns no error
(just like the existing nil case) while still ensuring newTestResolver's client
factory is not invoked; modify only the
TestWorkspaceResolver_EnsureResolved_EmptyInputIsNoop test (referencing
r.ensureResolved, newTestResolver) to include this extra assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a43a3ba-035f-40c9-9989-8bd9c695ec72
📒 Files selected for processing (2)
internal/controller/dependencyrule_controller.gointernal/controller/dependencyrule_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/dependencyrule_controller.go
BergCyrill
approved these changes
May 7, 2026
Contributor
BergCyrill
left a comment
BergCyrill
left a comment
There was a problem hiding this comment.
The logic itself seems legit to me and I have added some unit tests to the new workspace resolver additionally to the already existent and working e2e tests. The controller needs to get more testing in the future to prevent regression on functionality to be automatically discovered during CI.
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
Motivation
The controller was failing to resolve workspace paths for anything not at depth 1 under root. This change makes the resolver work for any depth of the kcp workspace tree.
Changes
Closes #26
Summary by CodeRabbit
Refactor
Style
Tests