feat: migrate to multicluster controller runtime #43
Conversation
WalkthroughRefactors operator to a multicluster runtime with an APIExport provider, makes controllers and subroutines cluster-aware (ClusterClientGetter, mcmanager/mclifecycle/mcreconcile), updates constructors/signatures and lazy orgs-client logic, adds/rewrites extensive tests, and updates module dependencies, mocks, and tooling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (4)pkg/subroutines/workspace_type.go (1)
pkg/subroutines/workspace_type_test.go (3)
pkg/subroutines/workspace.go (6)
internal/controller/account_controller.go (6)
🔇 Additional comments (18)
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 |
…rt multicluster operations
…tance) signature and tests
…, accountinfo, and common helpers
…anch flagged by staticcheck SA9003
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/subroutines/fga.go (1)
110-115: ServiceAccount validation bug + logging misuse.
- validateCreator checks “system.serviceaccount” (dot) instead of “system:serviceaccount:” (colon), allowing reserved SAs through.
- Logging uses an unrelated err variable; drop it.
Apply these diffs:
- if valid := validateCreator(*account.Spec.Creator); !valid { - log.Error().Err(err).Str("creator", *account.Spec.Creator).Msg("creator string is in the protected service account prefix range") - return ctrl.Result{}, errors.NewOperatorError(err, false, false) - } + if valid := validateCreator(*account.Spec.Creator); !valid { + log.Error().Str("creator", *account.Spec.Creator).Msg("creator string is in the protected service account prefix range") + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("creator in protected service account range"), false, false) + }-func validateCreator(creator string) bool { - return !strings.HasPrefix(creator, "system.serviceaccount") -} +func validateCreator(creator string) bool { + return !strings.HasPrefix(creator, "system:serviceaccount:") +}Also applies to: 253-255
🧹 Nitpick comments (10)
pkg/subroutines/workspace_type_test.go (1)
133-139: Consider removing duplicate test.This test appears to duplicate
TestFinalizeSkipsAccounts(lines 112-122), both verifying that finalization skips for non-org accounts. Consider consolidating these tests to reduce redundancy.pkg/subroutines/accountinfo_subroutine_test.go (2)
33-57: LGTM: Test validates organization AccountInfo creation.The test correctly verifies that processing an org account creates an AccountInfo with the expected fields.
Note: This test uses direct struct construction (
&AccountInfoSubroutine{...}) whileaccountinfo_test.gousesNewAccountInfoSubroutine(). While both are valid, consider using the constructor consistently for better maintainability.
59-86: LGTM: Test validates parent information inheritance.The test correctly verifies account-type accounts inherit parent organization details.
Note: This test appears to cover similar ground as
TestProcessAccountInheritsParentinaccountinfo_test.go(lines 92-118). Consider consolidating these tests to avoid redundancy unless they're exercising different code paths.pkg/subroutines/workspace.go (1)
136-144: Avoid silently skipping WorkspaceType readiness when orgs client is unavailable.checkWorkspaceTypeReady returns ready=true if orgsClient is nil, while getOrgsClient returns (nil, nil) when baseConfig is nil. This can create workspaces before types exist and differs from workspace_type.go which errors on missing baseConfig. Align behavior (e.g., return not-ready or an explicit error) or gate the skip behind a test-only constructor flag.
Also applies to: 155-164
pkg/subroutines/accountinfo.go (2)
173-176: Use the existing constant for AccountInfo name.Avoid string literal drift.
- err := cl.Get(ctx, client.ObjectKey{Name: "account"}, accountInfo) + err := cl.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName}, accountInfo)
104-106: Remove redundant wsCtx variable.wsCtx := ctx is a no-op here. Use ctx directly to reduce noise.
pkg/subroutines/workspace_subroutine_test.go (4)
14-15: Remove unused logger setup to reduce dependencies/noise.The suite-level logger is created but never used. Drop the import, field, and setup lines.
import ( "context" "errors" "fmt" "testing" "time" - "github.com/platform-mesh/golang-commons/logger" "github.com/stretchr/testify/suite" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" mccontext "sigs.k8s.io/multicluster-runtime/pkg/context" ) type WorkspaceSubroutineTestSuite struct { suite.Suite scheme *runtime.Scheme ctx context.Context - log *logger.Logger } func (s *WorkspaceSubroutineTestSuite) SetupSuite() { s.scheme = runtime.NewScheme() s.Require().NoError(corev1alpha1.AddToScheme(s.scheme)) s.Require().NoError(kcpcorev1alpha.AddToScheme(s.scheme)) s.Require().NoError(kcptenancyv1alpha.AddToScheme(s.scheme)) - var err error - s.log, err = logger.New(logger.DefaultConfig()) - s.Require().NoError(err) s.ctx = context.Background() }Also applies to: 28-33, 45-48
75-81: Stabilize requeue assertions by using a static rate limiter in tests.Using 1ms exponential limiters can be timing-sensitive. Prefer a deterministic limiter that always returns a fixed duration to avoid flakes.
Add a tiny static limiter for tests:
// in this test file type staticLimiter[T any] struct{ d time.Duration } func (s staticLimiter[T]) When(T) time.Duration { return s.d } func (s staticLimiter[T]) Forget(T) {} func (s staticLimiter[T]) NumRequeues(T) int { return 1 }Then replace occurrences like:
lim := workqueue.NewTypedItemExponentialFailureRateLimiter[ClusteredName](1time.Millisecond, 1time.Millisecond)with:
lim := staticLimiter[ClusteredName]{d: 50 * time.Millisecond}Also applies to: 86-91, 97-102, 116-120, 140-145, 172-177, 204-213, 244-248, 254-259, 266-271
55-69: Prefer mccontext scoping over annotations for consistency with the new multicluster runtime.Several tests set kcp.io/cluster annotations while others use mccontext.WithCluster. Normalize to mccontext in creation/operations to align with the migration and future-proof the tests.
Based on PR objectives
Also applies to: 179-193
250-259: Add a success-path test for Account type.You cover missing AccountInfo and empty org name. Consider a positive case with AccountInfo present and Organization.Name set to ensure no requeue and expected behavior.
Based on learnings
Also applies to: 261-271
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
.gitignore(1 hunks)cmd/operator.go(6 hunks)go.mod(6 hunks)internal/controller/account_controller.go(1 hunks)internal/controller/account_controller_reconcile_unit_test.go(1 hunks)internal/controller/account_controller_setup_unit_test.go(1 hunks)internal/controller/account_controller_test.go(5 hunks)internal/controller/account_controller_unit_test.go(1 hunks)pkg/subroutines/accountinfo.go(6 hunks)pkg/subroutines/accountinfo_extra_test.go(1 hunks)pkg/subroutines/accountinfo_subroutine_test.go(1 hunks)pkg/subroutines/accountinfo_test.go(1 hunks)pkg/subroutines/cluster_client.go(1 hunks)pkg/subroutines/cluster_client_test.go(1 hunks)pkg/subroutines/clusteredname.go(2 hunks)pkg/subroutines/clusteredname_test.go(1 hunks)pkg/subroutines/common.go(2 hunks)pkg/subroutines/common_test.go(4 hunks)pkg/subroutines/fga.go(6 hunks)pkg/subroutines/fga_helpers_test.go(1 hunks)pkg/subroutines/fga_subroutine_test.go(1 hunks)pkg/subroutines/fga_test.go(0 hunks)pkg/subroutines/mocks/mock_K8Service.go(2 hunks)pkg/subroutines/subroutines_getters_test.go(1 hunks)pkg/subroutines/test_helpers_test.go(1 hunks)pkg/subroutines/workspace.go(5 hunks)pkg/subroutines/workspace_subroutine_test.go(1 hunks)pkg/subroutines/workspace_test.go(0 hunks)pkg/subroutines/workspace_type.go(4 hunks)pkg/subroutines/workspace_type_error_test.go(1 hunks)pkg/subroutines/workspace_type_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/subroutines/fga_test.go
- pkg/subroutines/workspace_test.go
🧰 Additional context used
🧬 Code graph analysis (20)
pkg/subroutines/clusteredname_test.go (2)
api/v1alpha1/account_types.go (1)
Account(81-87)pkg/subroutines/clusteredname.go (1)
GetClusteredName(17-29)
internal/controller/account_controller_test.go (4)
pkg/testing/kcpenvtest/server.go (2)
Environment(40-62)NewEnvironment(64-80)internal/controller/account_controller.go (1)
NewAccountReconciler(59-81)api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeAccount(28-28)AccountTypeOrg(27-27)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
pkg/subroutines/accountinfo_extra_test.go (2)
pkg/subroutines/accountinfo.go (2)
AccountInfoSubroutine(32-37)AccountInfoSubroutineName(28-28)api/v1alpha1/account_types.go (1)
Account(81-87)
internal/controller/account_controller.go (6)
internal/config/config.go (1)
OperatorConfig(4-34)pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/workspace_type.go (1)
NewWorkspaceTypeSubroutine(125-127)pkg/subroutines/workspace.go (1)
NewWorkspaceSubroutine(41-44)pkg/subroutines/accountinfo.go (1)
NewAccountInfoSubroutine(39-42)pkg/subroutines/fga.go (1)
NewFGASubroutine(34-45)
pkg/subroutines/fga_subroutine_test.go (6)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)api/v1alpha1/account_info_types.go (2)
AccountInfo(65-71)AccountLocation(36-46)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)pkg/subroutines/mocks/mock_OpenFGAServiceClient.go (1)
NewOpenFGAServiceClient(17-27)pkg/subroutines/fga.go (2)
NewFGASubroutine(34-45)FGASubroutine(24-32)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
pkg/subroutines/fga.go (4)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
pkg/subroutines/cluster_client_test.go (1)
pkg/subroutines/workspace.go (1)
NewWorkspaceSubroutine(41-44)
cmd/operator.go (1)
api/v1alpha1/account_webhook.go (1)
SetupAccountWebhookWithManager(15-21)
pkg/subroutines/workspace_type_error_test.go (2)
pkg/subroutines/workspace_type.go (1)
NewWorkspaceTypeSubroutineWithClient(129-131)api/v1alpha1/account_types.go (3)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)
pkg/subroutines/workspace_type.go (1)
api/v1alpha1/account_types.go (2)
Account(81-87)AccountTypeOrg(27-27)
pkg/subroutines/workspace.go (4)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-34)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
pkg/subroutines/accountinfo.go (2)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-34)
internal/controller/account_controller_unit_test.go (4)
internal/config/config.go (1)
OperatorConfig(4-34)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)api/v1alpha1/account_types.go (1)
Account(81-87)
pkg/subroutines/accountinfo_subroutine_test.go (5)
api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/accountinfo.go (2)
AccountInfoSubroutine(32-37)DefaultAccountInfoName(29-29)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (3)
AccountInfo(65-71)ClusterInfo(32-34)AccountLocation(36-46)
internal/controller/account_controller_setup_unit_test.go (3)
api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)internal/controller/account_controller.go (1)
NewAccountReconciler(59-81)internal/config/config.go (1)
OperatorConfig(4-34)
pkg/subroutines/workspace_type_test.go (3)
internal/config/config.go (1)
OperatorConfig(4-34)pkg/subroutines/workspace_type.go (3)
WorkspaceTypeSubroutine(36-42)NewWorkspaceTypeSubroutineWithClient(129-131)NewWorkspaceTypeSubroutine(125-127)api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)
pkg/subroutines/subroutines_getters_test.go (6)
pkg/subroutines/workspace_type.go (2)
NewWorkspaceTypeSubroutine(125-127)NewWorkspaceTypeSubroutineWithClient(129-131)api/v1alpha1/account_types.go (2)
Account(81-87)AccountTypeOrg(27-27)pkg/subroutines/workspace.go (1)
NewWorkspaceSubroutineForTesting(47-49)pkg/subroutines/accountinfo.go (2)
NewAccountInfoSubroutine(39-42)AccountInfoSubroutine(32-37)pkg/subroutines/fga.go (1)
NewFGASubroutine(34-45)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
internal/controller/account_controller_reconcile_unit_test.go (2)
internal/config/config.go (1)
OperatorConfig(4-34)internal/controller/account_controller.go (1)
NewAccountReconciler(59-81)
pkg/subroutines/accountinfo_test.go (4)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/accountinfo.go (3)
NewAccountInfoSubroutine(39-42)DefaultAccountInfoName(29-29)AccountInfoSubroutine(32-37)api/v1alpha1/account_info_types.go (3)
AccountInfo(65-71)ClusterInfo(32-34)AccountLocation(36-46)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
pkg/subroutines/workspace_subroutine_test.go (5)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/workspace.go (3)
NewWorkspaceSubroutine(41-44)WorkspaceSubroutine(30-39)WorkspaceSubroutineFinalizer(27-27)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
🔇 Additional comments (51)
go.mod (1)
3-3: Verify Go toolchain availability
go 1.24.5is ahead of the latest published toolchain in my knowledge window. Please confirm that CI/build environments actually provide this version; otherwise the module will fail to build and should stay on the latest stable release available to you.pkg/subroutines/cluster_client.go (1)
17-31: Solid cluster client fallback handlingClean hand-off between cluster-aware and local clients; error path is explicit. Nice addition.
internal/controller/account_controller_reconcile_unit_test.go (1)
69-100: Great coverage of provider error pathsNice to see both not-found and generic provider errors surfaced through reconcile; this will guard against silent drops.
pkg/subroutines/cluster_client_test.go (1)
39-92: Thorough exercise of cluster-aware client selectionThese tests hit both the cluster-context and fallback branches, which should keep regressions in check.
pkg/subroutines/fga_subroutine_test.go (1)
61-308: Impressive coverage for FGA flowsThe suite captures happy paths, retries, cluster lookups, and edge cases—excellent safety net for the new multicluster wiring.
pkg/subroutines/workspace_type_test.go (11)
37-49: LGTM: Test suite setup is well-structured.The SetupSuite correctly initializes the scheme with all necessary API groups (core, tenancy) and establishes a logger and context for the test suite. The scheme registration ensures that fake clients can handle the relevant Kubernetes and KCP API types.
51-54: LGTM: Helper function provides clean test fixture creation.The helper correctly constructs a fake client with the test scheme and returns both the subroutine and client, enabling tests to verify side effects.
56-70: LGTM: Test correctly verifies workspace type creation for organizations.The test validates that processing an org-type account creates both the
organdaccworkspace types, and asserts successful completion without requeue.
72-82: LGTM: Test validates account type skipping logic.The test correctly verifies that non-org accounts do not create workspace types, checking that the expected resource is NotFound.
84-91: LGTM: Test validates error handling for missing client.The test ensures that when the subroutine lacks a client (constructed with nil config/scheme), it returns a retryable error, which is the correct failure mode.
93-110: LGTM: Test validates finalization cleanup.The test correctly seeds workspace types and verifies they are deleted during finalization, ensuring proper resource cleanup.
112-122: LGTM: Test validates finalization skip logic for account types.The test ensures that finalization for non-org accounts completes without error and does not attempt to delete workspace types.
124-131: LGTM: Test validates error handling for missing client during finalization.The test ensures that Finalize returns a retryable error when the client is unavailable, consistent with the Process error handling.
141-147: LGTM: Test validates error handling for invalid base configuration.The test ensures that
getOrgsClientreturns an error when the base config contains an invalid host URL, validating the error path.
149-160: LGTM: Test validates client caching behavior.The test ensures that
getOrgsClientreturns the same cached client on subsequent calls, avoiding unnecessary re-initialization.
162-167: LGTM: Test validates successful client creation with valid configuration.The test ensures that
getOrgsClientsuccessfully creates a client when provided with a valid base configuration.pkg/subroutines/accountinfo_test.go (12)
34-44: LGTM: Test suite setup is appropriate for accountinfo tests.The SetupSuite correctly initializes the scheme with core and KCP API groups needed for AccountInfo operations. Using
context.Background()is simpler and appropriate for unit tests.
46-59: LGTM: Helper functions provide clean test fixture creation.The helpers enable concise test data construction.
newWorkspacecorrectly populates both spec and status fields for workspace objects.
62-78: LGTM: Test validates organization AccountInfo creation.The test correctly verifies that processing an org account creates an AccountInfo resource with the expected account details and CA certificate.
81-89: LGTM: Test validates retry behavior for unready workspaces.The test ensures that when a workspace is not in Ready phase, processing yields a positive requeue duration for retry.
92-118: LGTM: Test validates parent information inheritance for accounts.The test correctly verifies that account-type accounts inherit parent organization details and FGA store information during AccountInfo creation.
121-130: LGTM: Test validates error handling for missing parent AccountInfo.The test ensures that processing an account when the parent AccountInfo is missing returns an error, which is the correct behavior.
132-142: LGTM: Test validates cluster getter integration.The test ensures that the subroutine correctly uses the cluster getter to obtain the client when processing within a multicluster context.
144-151: LGTM: Test validates cluster getter error handling.The test ensures that when the cluster getter returns an error, the subroutine propagates it correctly.
154-172: LGTM: Test validates finalizer coordination logic.The test ensures that AccountInfo finalization waits until it's the last finalizer before completing, preventing premature resource deletion.
174-178: LGTM: Test validates nil input handling.The test ensures that Finalize correctly handles nil input and returns an error.
180-186: LGTM: Test validates default requeue behavior without limiter.The test ensures that when no limiter is configured, the subroutine uses a default 1-second requeue duration.
189-208: LGTM: Tests validate workspace path extraction logic.The tests correctly verify error handling for malformed URLs and successful path extraction from valid workspace URLs.
pkg/subroutines/accountinfo_subroutine_test.go (3)
18-31: LGTM: Helper function provides clean scheme initialization.The helper correctly uses
t.Helper()and fails fast witht.Fatal()on scheme registration errors, following Go testing best practices.
88-104: LGTM: Test validates requeue behavior without limiter.The test ensures that when no limiter is configured and the workspace is not ready, processing returns a positive requeue duration.
106-119: LGTM: Test validates origin cluster annotation requirement.The test ensures that processing an account without the required cluster annotation returns a retryable error, which is the correct behavior.
internal/controller/account_controller_test.go (9)
78-96: LGTM: Extended timeouts accommodate KCP startup requirements.The 2-minute context timeout and control plane start timeout are appropriate for KCP environment initialization. The skip-on-failure pattern ensures tests don't fail in environments where KCP cannot be started.
105-126: LGTM: Multicluster runtime integration is correctly implemented.The setup properly initializes the APIExport provider, configures the multicluster manager with appropriate options, and wires the reconciler to use the multicluster manager. This aligns with the PR's migration objective.
131-142: LGTM: Test client correctly targets the orgs workspace.The client configuration for test assertions correctly targets the
orgs:root-orgworkspace, and the readiness check ensures the test environment is stable before proceeding.
145-157: LGTM: Teardown logic is safe and handles partial setup failures.The conditional checks ensure teardown doesn't panic if setup failed, and best-effort cleanup with ignored errors is appropriate for test cleanup.
159-164: LGTM: Controller startup correctly runs provider alongside manager.The async provider startup followed by blocking manager start is the correct pattern for multicluster controller runtime.
166-188: LGTM: Finalizer test updated for multicluster architecture.The test correctly verifies that three finalizers are added and matches the expected finalizer names for the new architecture.
190-215: LGTM: Workspace creation test correctly validates multicluster behavior.The test verifies workspace creation and readiness, using the multicluster-aware client for assertions.
217-239: LGTM: AccountInfo creation test validates organization flow.The test correctly verifies that AccountInfo is created for org-type accounts with the expected type field.
241-259: LGTM: Workspace finalizer test validates cleanup behavior.The test correctly verifies that deleting an account results in the workspace being removed, using
kerrors.IsNotFoundfor the check.pkg/subroutines/workspace_type_error_test.go (9)
21-30: LGTM: Error injection client is well-designed.The
failingClientwrapper correctly targets WorkspaceType objects for error injection, enabling focused error path testing.
93-112: LGTM: Conditional delete client enables granular error testing.The client implementations (
conditionalDeleteClient,failingDeleteClient) allow testing different delete scenarios, improving error path coverage.
123-137: LGTM: Selective fail client enables multi-call error testing.The
selectiveFailGetClientwith call counting allows testing error paths that occur on subsequent operations, providing comprehensive error coverage.
51-59: LGTM: Test validates Process error handling.The test correctly verifies that Get errors during Process result in a retryable error.
61-68: LGTM: Test validates NotFound is treated as success.The test ensures that NotFound errors during Finalize are not treated as failures, which is the correct behavior.
70-79: LGTM: Test validates delete error retry logic.The test correctly verifies that delete errors during Finalize result in a retryable error.
81-91: LGTM: Test validates create and update paths.The test ensures that both the initial create and subsequent update operations work correctly when processing the same account twice.
114-121: LGTM: Test validates multiple delete error handling.The test verifies that when the second WorkspaceType deletion fails, the error is properly returned as retryable.
139-147: LGTM: Test validates second Get error path.The test ensures that errors on the second WorkspaceType retrieval (account type) are properly handled and returned as retryable.
pkg/subroutines/workspace_type.go (1)
125-131: Constructors and lazy orgs client look good.Clear separation for prod (baseConfig+scheme) and tests (WithClient) with thread-safe lazy init. LGTM.
internal/controller/account_controller.go (1)
59-81: Wiring to multicluster lifecycle and subroutines looks sound.Nice: copy local config, pass scheme/CA, build subs once, enable condition management.
Please confirm mcmanager.Manager satisfies subroutines.ClusterClientGetter (GetCluster(ctx, name) signature) so passing mgr into buildAccountSubroutines matches the expected interface.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/subroutines/accountinfo_test.go (6)
23-28: Remove unused logger from the suite.
logis initialized but never used. Drop the field and setup to simplify.type AccountInfoSubroutineTestSuite struct { suite.Suite scheme *runtime.Scheme ctx context.Context - log *logger.Logger } func (s *AccountInfoSubroutineTestSuite) SetupSuite() { s.scheme = runtime.NewScheme() s.Require().NoError(corev1alpha1.AddToScheme(s.scheme)) s.Require().NoError(kcpcorev1alpha.AddToScheme(s.scheme)) s.Require().NoError(kcptenancyv1alpha.AddToScheme(s.scheme)) - - var err error - s.log, err = logger.New(logger.DefaultConfig()) - s.Require().NoError(err) s.ctx = mccontext.WithCluster(context.Background(), "cluster-test") }Also remove the logger import.
- "github.com/platform-mesh/golang-commons/logger"Also applies to: 40-44
51-59: Strongly type workspace phase to avoid string casts.Accepting a string then casting invites bugs. Take
kcpcorev1alpha.LogicalClusterPhaseTypedirectly.-func newWorkspace(name, phase, cluster, url string) *kcptenancyv1alpha.Workspace { +func newWorkspace(name string, phase kcpcorev1alpha.LogicalClusterPhaseType, cluster, url string) *kcptenancyv1alpha.Workspace { ws := &kcptenancyv1alpha.Workspace{ObjectMeta: metav1.ObjectMeta{Name: name}} ws.Spec.Cluster = cluster ws.Spec.URL = url - if phase != "" { - ws.Status.Phase = kcpcorev1alpha.LogicalClusterPhaseType(phase) - } + ws.Status.Phase = phase return ws }Update call sites, e.g.:
ws := newWorkspace("org-a", kcpcorev1alpha.LogicalClusterPhaseReady, "cluster-org-a", "https://host/root:orgs/org-a")
63-66: De-duplicate the cluster annotation literal.
"kcp.io/cluster"is repeated. Centralize to a helper/const to prevent typos and ease updates if the key changes.Example:
const clusterAnno = "kcp.io/cluster" func newAccount(name string, t corev1alpha1.AccountType, cluster string) *corev1alpha1.Account { return &corev1alpha1.Account{ ObjectMeta: metav1.ObjectMeta{ Name: name, Annotations: map[string]string{clusterAnno: cluster}, }, Spec: corev1alpha1.AccountSpec{Type: t}, } }If a canonical constant exists in your codebase or KCP APIs, prefer importing it over redefining.
Also applies to: 82-86, 94-107, 121-128, 132-139, 145-148, 155-161
91-119: Strengthen assertions for inherited fields.Add checks for derived path/URL on the created AccountInfo to validate full propagation, not just parent name/FGA.
Example:
- Assert
created.Spec.Account.URL == "https://host/root:orgs/org-c/acc-x"- Assert
created.Spec.Account.Path(and/orParentAccount.Path) as expected.
153-173: Avoid hard-coded finalizer string in tests.Use an exported finalizer const (if available) to reduce brittleness.
Search for a constant in the codebase (e.g.,
AccountInfoFinalizer) and reference it here; otherwise define a test-localconst.
180-186: Avoid magic number for default requeue.If the implementation exposes/uses a constant for the default requeue without a limiter, reference that constant in the assertion.
Confirm the implementation’s default is stable at 1s; align the test with a shared const to prevent future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/subroutines/accountinfo_extra_test.go(1 hunks)pkg/subroutines/accountinfo_subroutine_test.go(1 hunks)pkg/subroutines/accountinfo_test.go(1 hunks)pkg/subroutines/clusteredname.go(2 hunks)pkg/subroutines/fga_subroutine_test.go(1 hunks)pkg/subroutines/subroutines_getters_test.go(1 hunks)pkg/subroutines/workspace_subroutine_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/subroutines/clusteredname.go
- pkg/subroutines/workspace_subroutine_test.go
- pkg/subroutines/subroutines_getters_test.go
- pkg/subroutines/accountinfo_extra_test.go
- pkg/subroutines/accountinfo_subroutine_test.go
- pkg/subroutines/fga_subroutine_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/subroutines/accountinfo_test.go (4)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/accountinfo.go (3)
NewAccountInfoSubroutine(39-42)DefaultAccountInfoName(29-29)AccountInfoSubroutine(32-37)api/v1alpha1/account_info_types.go (3)
AccountInfo(65-71)ClusterInfo(32-34)AccountLocation(36-46)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
🔇 Additional comments (5)
pkg/subroutines/accountinfo_test.go (5)
46-48: LGTM: fake client helper.Scheme is wired; objects seeded correctly. This keeps tests concise.
61-79: LGTM: org AccountInfo creation path.Covers happy path; verifies key fields and no requeue.
80-90: LGTM: workspace-not-ready retry.Asserts requeue without error; behavior matches limiter semantics.
188-199: LGTM: error paths for workspace URL parsing.Validates empty URL and trailing-slash cases.
200-208: LGTM: success path for workspace URL parsing.Checks both last segment and full path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/subroutines/fga.go (1)
70-71: Update or remove misleading comment.The comment "Prepare context to work in workspace" is now inaccurate since the code simply assigns
ctxtowsCtxwithout modification. Previously, this line usedkontext.WithClusterto add cluster information, but that's no longer needed.Apply this diff to clarify the code:
- // Prepare context to work in workspace - wsCtx := ctx + // Use the cluster-scoped context + wsCtx := ctxOr simply remove the intermediate variable if it's not needed elsewhere:
- // Prepare context to work in workspace - wsCtx := ctx - - accountInfo, err := e.getAccountInfo(wsCtx, clusterClient) + accountInfo, err := e.getAccountInfo(ctx, clusterClient)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/subroutines/fga.go(8 hunks)pkg/subroutines/fga_helpers_test.go(1 hunks)pkg/subroutines/fga_subroutine_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/subroutines/fga_helpers_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/subroutines/fga_subroutine_test.go (6)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)api/v1alpha1/account_info_types.go (2)
AccountInfo(65-71)AccountLocation(36-46)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)pkg/subroutines/mocks/mock_OpenFGAServiceClient.go (1)
NewOpenFGAServiceClient(17-27)pkg/subroutines/fga.go (2)
NewFGASubroutine(34-45)FGASubroutine(24-32)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
pkg/subroutines/fga.go (3)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
🔇 Additional comments (10)
pkg/subroutines/fga.go (5)
27-27: LGTM: Cluster-aware field addition.The
clusterGetterfield and updated constructor properly integrate cluster-scoped client retrieval into FGASubroutine, aligning with the multicluster migration objectives.Also applies to: 34-44
54-59: LGTM: Cluster-scoped client retrieval.The code correctly obtains a cluster-scoped client via
clientForContextand applies it to workspace retrieval. Error handling is appropriate.
159-164: LGTM: Consistent cluster client handling in finalization.The finalization path correctly mirrors the Process method's cluster-scoped client retrieval pattern with appropriate error handling.
226-228: LGTM: Explicit client parameter improves testability.The updated signature makes dependencies explicit and supports the cluster-aware client pattern.
237-239: LGTM: Interface alignment.The updated
Finalizerssignature aligns with interface requirements. The unused parameter is appropriately marked with an underscore prefix.pkg/subroutines/fga_subroutine_test.go (5)
26-60: LGTM: Well-structured test suite setup.The test suite properly initializes the runtime scheme with all necessary types, creates a clustered context to prevent panics in
MustGetClusteredName, and provides clean helper functions for creating test fixtures.
62-182: LGTM: Comprehensive Process method test coverage.The test cases thoroughly cover:
- Tuple writing for organizations and accounts with creators
- Workspace readiness retry behavior
- Already-written tuple detection
- Creator validation (rejecting service accounts)
- Error handling (missing store, write failures)
Mock expectations are precise and all assertions are appropriate.
198-211: LGTM: Finalization skip test and helper validation.The org-type finalization skip test is safe despite using an uninitialized
FGASubroutinebecause the early return based on account type prevents access to uninitialized fields. Helper function tests appropriately validate both creator validation and user formatting logic.
251-309: LGTM: Thorough error case coverage.The remaining test cases comprehensively validate error handling for:
- Missing store IDs during finalization
- Missing account and parent cluster IDs during processing
- Parent relation writes for account types
The
strPtrhelper is a standard utility pattern. All mock expectations and assertions are appropriate.
213-249: fakeClusterGetter and fakeCluster types are already defined in pkg/subroutines/test_helpers_test.go; no changes required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/subroutines/workspace.go (1)
128-132: Remove the fallback to clusterClient.Scheme() - it will fail at runtime.The fallback to
clusterClient.Scheme()won't work becauseclient.Clientdoesn't expose aScheme()method. Sincer.schemeshould always be set byNewWorkspaceSubroutinein production code, remove the fallback entirely and user.schemedirectly.Apply this diff to use the scheme directly:
- scheme := r.scheme - if scheme == nil { - scheme = clusterClient.Scheme() - } - return controllerutil.SetOwnerReference(instance, createdWorkspace, scheme) + return controllerutil.SetOwnerReference(instance, createdWorkspace, r.scheme)Then update the test constructor to accept and set the scheme parameter (see previous comment).
🧹 Nitpick comments (1)
pkg/subroutines/workspace.go (1)
47-50: Update the test constructor to accept a scheme parameter.The test constructor leaves
schemeas nil, which will cause issues whenSetOwnerReferenceis called (line 132) since a scheme is required. Consider updating the constructor signature to include a scheme parameter.Apply this diff:
-func NewWorkspaceSubroutineForTesting(client client.Client, limiter workqueue.TypedRateLimiter[ClusteredName]) *WorkspaceSubroutine { - return &WorkspaceSubroutine{client: client, limiter: limiter} +func NewWorkspaceSubroutineForTesting(client client.Client, limiter workqueue.TypedRateLimiter[ClusteredName], scheme *runtime.Scheme) *WorkspaceSubroutine { + return &WorkspaceSubroutine{client: client, limiter: limiter, scheme: scheme} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/subroutines/workspace.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/subroutines/workspace.go (5)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-36)api/v1alpha1/account_types.go (2)
Account(81-87)AccountTypeAccount(28-28)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
🔇 Additional comments (5)
pkg/subroutines/workspace.go (5)
5-6: LGTM!The new imports support cluster-aware operations and lazy initialization patterns.
Also applies to: 16-17
31-40: LGTM!The struct properly supports cluster-aware operations with appropriate synchronization for the cached orgs client.
56-84: LGTM!The finalization logic correctly uses cluster-aware clients and properly handles workspace deletion with appropriate error handling.
86-88: LGTM!Finalizers signature correctly updated to match the new interface.
157-185: LGTM!The lazy initialization of the orgs client is correctly implemented with proper thread safety using mutex and appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Taskfile.yml (1)
52-64: Go 1.24.5 released; mockery v3.5.5 lacks native import-alias
- Go 1.24.5 is available (released July 8, 2025).
- mockery v3.5.5 does not support a dedicated import-alias; keep the Perl post-processing or optionally use
.mockery.yaml’sreplace-type/resolve-type-aliasremapping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
Taskfile.yml(1 hunks)pkg/subroutines/doc.go(1 hunks)pkg/subroutines/mocks/mock_K8Service.go(1 hunks)pkg/subroutines/mocks/mock_OpenFGAServiceClient.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T11:39:49.264Z
Learnt from: kekcleader
PR: platform-mesh/account-operator#43
File: pkg/subroutines/mocks/mock_K8Service.go:533-605
Timestamp: 2025-10-07T11:39:49.264Z
Learning: In controller-runtime v0.22.x, the Writer.Apply method signature uses runtime.ApplyConfiguration as the second parameter, not client.Object: Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error. This is the correct signature for Server-Side Apply support.
Applied to files:
pkg/subroutines/mocks/mock_K8Service.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / lint / lint
🔇 Additional comments (2)
pkg/subroutines/mocks/mock_K8Service.go (1)
530-600: LGTM! Apply mock correctly implements controller-runtime v0.22.x signature.The Apply method mock uses
runtime.ApplyConfigurationas the second parameter, which is the correct signature for controller-runtime v0.22.x Server-Side Apply support. The implementation follows the same pattern as other mock methods in this file, with proper variadic options handling, type assertions, and Return/RunAndReturn wiring.Based on learnings.
pkg/subroutines/mocks/mock_OpenFGAServiceClient.go (1)
10-10: LGTM!The import alias
openfgav1is correctly added and consistently used throughout the auto-generated mock file. All type references have been updated to use theopenfgav1.*prefix.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/subroutines/doc.go (2)
3-3: Add error handling and consider simplifying the fallback logic.The conditional approach (config file or explicit flags) is pragmatic, but several concerns remain:
- No error handling: If
mockeryfails,go generatesucceeds silently. Addset -eor check exit codes.- Maintainability: Running
mockerytwice with long flag lists is harder to maintain than a single.mockery.yaml. Consider always requiring the config file and documenting its setup in the README or a setup script.- Redundancy: If
.mockery.yamlshould exist (per past review feedback), the fallback branch may never execute in practice.Example of adding error handling:
-//go:generate bash -c "cd ../.. && if [ -f .mockery.yaml ]; then go run -mod=mod github.com/vektra/mockery/[email protected]; else go run -mod=mod github.com/vektra/mockery/[email protected] --srcpkg=github.com/openfga/api/proto/openfga/v1 --name=OpenFGAServiceClient --case=underscore --with-expecter --dir=pkg/subroutines/mocks --pkgname=mocks --formatter=goimports && go run -mod=mod github.com/vektra/mockery/[email protected] --srcpkg=sigs.k8s.io/controller-runtime/pkg/client --name=Client --case=underscore --with-expecter --dir=pkg/subroutines/mocks --pkgname=mocks --formatter=goimports; fi" +//go:generate bash -c "set -e; cd ../.. && if [ -f .mockery.yaml ]; then go run -mod=mod github.com/vektra/mockery/[email protected]; else go run -mod=mod github.com/vektra/mockery/[email protected] --srcpkg=github.com/openfga/api/proto/openfga/v1 --name=OpenFGAServiceClient --case=underscore --with-expecter --dir=pkg/subroutines/mocks --pkgname=mocks --formatter=goimports && go run -mod=mod github.com/vektra/mockery/[email protected] --srcpkg=sigs.k8s.io/controller-runtime/pkg/client --name=Client --case=underscore --with-expecter --dir=pkg/subroutines/mocks --pkgname=mocks --formatter=goimports; fi"
4-4: Use mockery v3 replace-type to alias imports
Remove the Perl-based post-processing in thego:generateline and configure the alias natively in.mockery.yamlor via the CLI flag:--replace-type=github.com/openfga/api/proto/openfga/v1.OpenFGAServiceClient=openfgav1:github.com/openfga/api/proto/openfga/v1.OpenFGAServiceClientThis eliminates the external dependency and brittle regex hack.
pkg/subroutines/accountinfo.go (1)
49-52: Consider removing defensive nil checks.The nil checks for
ro(line 50) andaccountWorkspace(line 84) are defensive guards that might hide bugs in the lifecycle framework or inretrieveWorkspace. If the framework guarantees non-nil runtime objects andretrieveWorkspaceproperly returns errors for missing workspaces, these checks add noise without value.Also applies to: 84-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/subroutines/accountinfo.go(6 hunks)pkg/subroutines/doc.go(1 hunks)pkg/subroutines/fga.go(7 hunks)pkg/subroutines/workspace.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/subroutines/workspace.go (3)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-36)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
pkg/subroutines/accountinfo.go (3)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-36)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)
pkg/subroutines/fga.go (4)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(13-15)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(29-29)
🔇 Additional comments (11)
pkg/subroutines/fga.go (4)
27-27: LGTM: Cluster-aware dependency injection.The
clusterGetterfield and constructor parameter properly enable cluster-scoped client retrieval throughout the subroutine.Also applies to: 34-34, 38-38
54-57: LGTM: Correct cluster client retrieval and usage.Properly obtains the cluster-scoped client via
clientForContextand passes it to workspace and account info retrieval, ensuring operations target the correct cluster context.Also applies to: 59-69
155-160: LGTM: Finalize correctly uses cluster-scoped client.Properly retrieves the cluster client and uses it for account info lookup during finalization.
250-250: LGTM: Validation prefix corrected.The check for
"system:serviceaccount:"with a colon now correctly validates the service account prefix format.pkg/subroutines/workspace.go (4)
32-40: LGTM: Dependencies for cluster-aware operation.The new fields (
clusterGetter,baseConfig,scheme,mu,orgsClient) and updated constructor properly support cluster-scoped client retrieval and lazy orgs client initialization.Also applies to: 42-44
47-50: LGTM: Test constructor simplifies unit testing.Provides a clean way to construct the subroutine for tests without full cluster dependencies.
126-130: LGTM: Scheme handling correctly addresses previous review.Properly uses
r.schemewhen available and falls back toclusterClient.Scheme()only when necessary. This resolves the previous concern about callingScheme()on the client interface.Based on learnings
156-184: LGTM: Thread-safe lazy initialization.The
getOrgsClientmethod correctly uses a mutex to ensure thread-safe lazy initialization of the orgs client, with proper nil checks forbaseConfigand optional scheme support.pkg/subroutines/accountinfo.go (3)
33-36: LGTM: Cluster-aware dependency injection.The
clusterGetterfield and constructor parameter enable cluster-scoped client retrieval for account info operations.Also applies to: 39-41
75-78: LGTM: Correct cluster client retrieval.Properly obtains the cluster-scoped client and uses it for workspace and account info operations.
Also applies to: 80-86
155-157: LGTM: Signature updated for cluster-scoped client.The
retrieveAccountInfomethod now correctly accepts aclient.Clientparameter, enabling callers to pass the cluster-scoped client.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-3: Invalidgodirective versionThe
godirective only accepts major.minor values;go 1.24.5will be rejected by the Go toolchain (go: invalid go version '1.24.5'). Drop the patch component so the module builds.-go 1.24.5 +go 1.24
🧹 Nitpick comments (1)
pkg/subroutines/limiters_test.go (1)
1-12: LGTM! Clean test helper implementation.The
staticLimiterprovides a deterministic rate limiter for tests, with a fixed delay and no-op behavior forForget. The implementation is correct and serves its intended purpose well.Optionally, consider adding a constructor function for better discoverability:
func newStaticLimiter(delay time.Duration) *staticLimiter { return &staticLimiter{delay: delay} }This would make the test helper easier to discover and use, though the current direct struct initialization approach is perfectly valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.mod(6 hunks)pkg/subroutines/accountinfo.go(7 hunks)pkg/subroutines/accountinfo_extra_test.go(1 hunks)pkg/subroutines/accountinfo_subroutine_test.go(1 hunks)pkg/subroutines/accountinfo_test.go(1 hunks)pkg/subroutines/cluster_client.go(1 hunks)pkg/subroutines/fga.go(7 hunks)pkg/subroutines/fga_subroutine_test.go(1 hunks)pkg/subroutines/limiters_test.go(1 hunks)pkg/subroutines/subroutines_getters_test.go(1 hunks)pkg/subroutines/workspace.go(5 hunks)pkg/subroutines/workspace_subroutine_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/subroutines/accountinfo_test.go
- pkg/subroutines/accountinfo_subroutine_test.go
- pkg/subroutines/subroutines_getters_test.go
- pkg/subroutines/fga_subroutine_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/subroutines/limiters_test.go (1)
pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
pkg/subroutines/workspace_subroutine_test.go (5)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/workspace.go (3)
NewWorkspaceSubroutine(43-46)WorkspaceSubroutine(32-41)WorkspaceSubroutineFinalizer(29-29)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(30-30)
pkg/subroutines/workspace.go (3)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(10-12)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-36)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(30-30)
pkg/subroutines/accountinfo.go (2)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(10-12)pkg/subroutines/clusteredname.go (2)
ClusteredName(12-15)MustGetClusteredName(31-36)
pkg/subroutines/accountinfo_extra_test.go (2)
pkg/subroutines/accountinfo.go (2)
AccountInfoSubroutine(33-38)AccountInfoSubroutineName(29-29)api/v1alpha1/account_types.go (1)
Account(81-87)
pkg/subroutines/fga.go (3)
pkg/subroutines/cluster_client.go (1)
ClusterClientGetter(10-12)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / lint / lint
- GitHub Check: pipe / testSource / test
🔇 Additional comments (3)
pkg/subroutines/workspace_subroutine_test.go (3)
1-26: LGTM!Imports are appropriate for the multicluster test suite, including KCP SDK types, testify suite, fake clients, and multicluster-runtime context.
28-53: LGTM!Suite setup correctly registers all necessary schemes (Account, KCP core, tenancy), initializes a logger, and sets up a cluster-aware context. The
newClienthelper is clean and reusable.
55-315: Comprehensive test coverage.The test suite thoroughly exercises:
- Happy paths for org and account workspace creation (Lines 55-72, 207-223)
- Retry behavior for unready/missing workspace types (Lines 74-104)
- Finalization with deletion in progress and NotFound cases (Lines 153-168, 272-284)
- Error handling for cluster getter, client operations, and invalid configurations (Lines 170-243)
- Caching behavior for orgsClient (Lines 257-270)
- Account-specific flows with AccountInfo dependencies (Lines 286-315)
The use of custom error clients (
deleteErrorClient,failingGetClient) and configurable rate limiters enables precise testing of edge cases.
| func (s *WorkspaceSubroutineTestSuite) TestFinalizeDeletesWorkspace() { | ||
| acc := &corev1alpha1.Account{ObjectMeta: metav1.ObjectMeta{Name: "org-del", Finalizers: []string{WorkspaceSubroutineFinalizer}, Annotations: map[string]string{"kcp.io/cluster": "root"}}, Spec: corev1alpha1.AccountSpec{Type: corev1alpha1.AccountTypeOrg}} | ||
| ws := &kcptenancyv1alpha.Workspace{ObjectMeta: metav1.ObjectMeta{Name: "org-del"}} | ||
| cl := s.newClient(acc, ws) | ||
| lim := workqueue.NewTypedItemExponentialFailureRateLimiter[ClusteredName](1*time.Millisecond, 1*time.Millisecond) | ||
| sub := &WorkspaceSubroutine{ | ||
| client: cl, | ||
| clusterGetter: fakeClusterGetter{cluster: &fakeCluster{client: cl}}, | ||
| limiter: lim, | ||
| } | ||
| res, opErr := sub.Finalize(s.ctx, acc) | ||
| s.Nil(opErr) | ||
| s.True(res.RequeueAfter > 0) | ||
| } |
There was a problem hiding this comment.
Clarify test name to match actual behavior.
The test name TestFinalizeDeletesWorkspace suggests verification of deletion, but the test only checks that Finalize requeues (Line 118: s.True(res.RequeueAfter > 0)). Consider renaming to TestFinalizeRequeuesForWorkspaceDeletion or adding an assertion to verify the workspace has a deletion timestamp set:
res, opErr := sub.Finalize(s.ctx, acc)
s.Nil(opErr)
s.True(res.RequeueAfter > 0)
+
+// Verify deletion was triggered
+wsAfter := &kcptenancyv1alpha.Workspace{}
+s.Require().NoError(cl.Get(s.ctx, client.ObjectKey{Name: "org-del"}, wsAfter))
+s.NotNil(wsAfter.DeletionTimestamp)📝 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.
| func (s *WorkspaceSubroutineTestSuite) TestFinalizeDeletesWorkspace() { | |
| acc := &corev1alpha1.Account{ObjectMeta: metav1.ObjectMeta{Name: "org-del", Finalizers: []string{WorkspaceSubroutineFinalizer}, Annotations: map[string]string{"kcp.io/cluster": "root"}}, Spec: corev1alpha1.AccountSpec{Type: corev1alpha1.AccountTypeOrg}} | |
| ws := &kcptenancyv1alpha.Workspace{ObjectMeta: metav1.ObjectMeta{Name: "org-del"}} | |
| cl := s.newClient(acc, ws) | |
| lim := workqueue.NewTypedItemExponentialFailureRateLimiter[ClusteredName](1*time.Millisecond, 1*time.Millisecond) | |
| sub := &WorkspaceSubroutine{ | |
| client: cl, | |
| clusterGetter: fakeClusterGetter{cluster: &fakeCluster{client: cl}}, | |
| limiter: lim, | |
| } | |
| res, opErr := sub.Finalize(s.ctx, acc) | |
| s.Nil(opErr) | |
| s.True(res.RequeueAfter > 0) | |
| } | |
| func (s *WorkspaceSubroutineTestSuite) TestFinalizeDeletesWorkspace() { | |
| acc := &corev1alpha1.Account{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "org-del", | |
| Finalizers: []string{WorkspaceSubroutineFinalizer}, | |
| Annotations: map[string]string{ | |
| "kcp.io/cluster": "root", | |
| }, | |
| }, | |
| Spec: corev1alpha1.AccountSpec{ | |
| Type: corev1alpha1.AccountTypeOrg, | |
| }, | |
| } | |
| ws := &kcptenancyv1alpha.Workspace{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "org-del"}, | |
| } | |
| cl := s.newClient(acc, ws) | |
| lim := workqueue.NewTypedItemExponentialFailureRateLimiter[ClusteredName](1*time.Millisecond, 1*time.Millisecond) | |
| sub := &WorkspaceSubroutine{ | |
| client: cl, | |
| clusterGetter: fakeClusterGetter{cluster: &fakeCluster{client: cl}}, | |
| limiter: lim, | |
| } | |
| res, opErr := sub.Finalize(s.ctx, acc) | |
| s.Nil(opErr) | |
| s.True(res.RequeueAfter > 0) | |
| // Verify deletion was triggered | |
| wsAfter := &kcptenancyv1alpha.Workspace{} | |
| s.Require().NoError(cl.Get(s.ctx, client.ObjectKey{Name: "org-del"}, wsAfter)) | |
| s.NotNil(wsAfter.DeletionTimestamp) | |
| } |
🤖 Prompt for AI Agents
In pkg/subroutines/workspace_subroutine_test.go around lines 106-119, the test
function name TestFinalizeDeletesWorkspace is misleading because the test only
asserts that Finalize requeues (res.RequeueAfter > 0); either rename the test to
reflect that behavior (e.g., TestFinalizeRequeuesForWorkspaceDeletion) or keep
the name and add an assertion that verifies the workspace was marked for
deletion (fetch the Workspace from the fake client and assert it has a non-nil
DeletionTimestamp or equivalent) in addition to the existing requeue assertion.
| func (s *WorkspaceSubroutineTestSuite) TestProcessTreatsMissingOrgsClientAsReady() { | ||
| // With the new behavior, missing orgs client results in an error and no workspace creation. | ||
| acc := &corev1alpha1.Account{ObjectMeta: metav1.ObjectMeta{Name: "org-ws3", Annotations: map[string]string{"kcp.io/cluster": "root"}}, Spec: corev1alpha1.AccountSpec{Type: corev1alpha1.AccountTypeOrg}} | ||
| cl := s.newClient(acc) | ||
| getter := fakeClusterGetter{cluster: &fakeCluster{client: cl}} | ||
| sub := NewWorkspaceSubroutine(getter, nil, nil, nil) | ||
| _, opErr := sub.Process(s.ctx, acc) | ||
| s.NotNil(opErr) | ||
| } |
There was a problem hiding this comment.
Test name contradicts expected behavior.
The test name TestProcessTreatsMissingOrgsClientAsReady implies the missing orgsClient is treated as ready, but Line 150 asserts s.NotNil(opErr), expecting an error. The comment at Line 144 confirms the "new behavior" returns an error.
Rename to match the actual behavior:
-func (s *WorkspaceSubroutineTestSuite) TestProcessTreatsMissingOrgsClientAsReady() {
- // With the new behavior, missing orgs client results in an error and no workspace creation.
+func (s *WorkspaceSubroutineTestSuite) TestProcessErrorsWhenOrgsClientMissing() {
+ // Missing orgs client results in an error and no workspace creation.📝 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.
| func (s *WorkspaceSubroutineTestSuite) TestProcessTreatsMissingOrgsClientAsReady() { | |
| // With the new behavior, missing orgs client results in an error and no workspace creation. | |
| acc := &corev1alpha1.Account{ObjectMeta: metav1.ObjectMeta{Name: "org-ws3", Annotations: map[string]string{"kcp.io/cluster": "root"}}, Spec: corev1alpha1.AccountSpec{Type: corev1alpha1.AccountTypeOrg}} | |
| cl := s.newClient(acc) | |
| getter := fakeClusterGetter{cluster: &fakeCluster{client: cl}} | |
| sub := NewWorkspaceSubroutine(getter, nil, nil, nil) | |
| _, opErr := sub.Process(s.ctx, acc) | |
| s.NotNil(opErr) | |
| } | |
| func (s *WorkspaceSubroutineTestSuite) TestProcessErrorsWhenOrgsClientMissing() { | |
| // Missing orgs client results in an error and no workspace creation. | |
| acc := &corev1alpha1.Account{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "org-ws3", | |
| Annotations: map[string]string{"kcp.io/cluster": "root"}, | |
| }, | |
| Spec: corev1alpha1.AccountSpec{Type: corev1alpha1.AccountTypeOrg}, | |
| } | |
| cl := s.newClient(acc) | |
| getter := fakeClusterGetter{cluster: &fakeCluster{client: cl}} | |
| sub := NewWorkspaceSubroutine(getter, nil, nil, nil) | |
| _, opErr := sub.Process(s.ctx, acc) | |
| s.NotNil(opErr) | |
| } |
🤖 Prompt for AI Agents
In pkg/subroutines/workspace_subroutine_test.go around lines 143 to 151, the
test name TestProcessTreatsMissingOrgsClientAsReady contradicts its behavior (it
expects an error); rename the test to reflect the new behavior (e.g.,
TestProcessReturnsErrorWhenOrgsClientMissing or
TestProcessTreatsMissingOrgsClientAsNotReady) and update any references,
ensuring the name clearly indicates the test expects an error when the orgs
client is missing.
| accountInfo := &corev1alpha1.AccountInfo{} | ||
| err := r.client.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName, Namespace: instance.Namespace}, accountInfo) | ||
| if err != nil { | ||
| if err := clusterClient.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName, Namespace: instance.Namespace}, accountInfo); err != nil { | ||
| if kerrors.IsNotFound(err) { | ||
| // AccountInfo not found, requeue | ||
| return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil | ||
| } | ||
| return ctrl.Result{}, errors.NewOperatorError(err, true, true) | ||
| } | ||
| if accountInfo.Spec.Organization.Name == "" { |
There was a problem hiding this comment.
Cluster-scoped AccountInfo fetched with a namespace
AccountInfo is created cluster-scoped (see accountinfo.go where it’s written without a namespace). Here we issue a GET with Namespace: instance.Namespace, which makes the request namespaced and guarantees a 404 even when the object exists. That leaves account workspaces stuck in perpetual requeue. Drop the namespace from the lookup.
- if err := clusterClient.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName, Namespace: instance.Namespace}, accountInfo); err != nil {
+ if err := clusterClient.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName}, accountInfo); err != nil {📝 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.
| accountInfo := &corev1alpha1.AccountInfo{} | |
| err := r.client.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName, Namespace: instance.Namespace}, accountInfo) | |
| if err != nil { | |
| if err := clusterClient.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName, Namespace: instance.Namespace}, accountInfo); err != nil { | |
| if kerrors.IsNotFound(err) { | |
| // AccountInfo not found, requeue | |
| return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil | |
| } | |
| return ctrl.Result{}, errors.NewOperatorError(err, true, true) | |
| } | |
| if accountInfo.Spec.Organization.Name == "" { | |
| accountInfo := &corev1alpha1.AccountInfo{} | |
| if err := clusterClient.Get(ctx, client.ObjectKey{Name: DefaultAccountInfoName}, accountInfo); err != nil { | |
| if kerrors.IsNotFound(err) { | |
| return ctrl.Result{RequeueAfter: r.limiter.When(cn)}, nil | |
| } | |
| return ctrl.Result{}, errors.NewOperatorError(err, true, true) | |
| } | |
| if accountInfo.Spec.Organization.Name == "" { |
🤖 Prompt for AI Agents
In pkg/subroutines/workspace.go around lines 112 to 119, the code performs a
namespaced GET for the cluster-scoped AccountInfo by using
client.ObjectKey{Name: DefaultAccountInfoName, Namespace: instance.Namespace};
remove the Namespace from the lookup (use client.ObjectKey{Name:
DefaultAccountInfoName} or an empty namespace) so the GET is cluster-scoped and
won't return a spurious 404 when the AccountInfo exists.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/subroutines/workspace_subroutine_test.go (2)
106-119: Test name vs. assertion (deletion).Previous review flagged this; either assert deletion timestamp or rename to reflect requeue behavior.
Example assertion to add:
res, opErr := sub.Finalize(s.ctx, acc) s.Nil(opErr) s.True(res.RequeueAfter > 0) + +wsAfter := &kcptenancyv1alpha.Workspace{} +s.Require().NoError(cl.Get(s.ctx, client.ObjectKey{Name: "org-del"}, wsAfter)) +s.NotNil(wsAfter.DeletionTimestamp)
143-151: Test name contradicts behavior.Already noted previously: rename to indicate it expects an error when orgs client is missing.
-func (s *WorkspaceSubroutineTestSuite) TestProcessTreatsMissingOrgsClientAsReady() { +func (s *WorkspaceSubroutineTestSuite) TestProcessErrorsWhenOrgsClientMissing() {
🧹 Nitpick comments (9)
pkg/subroutines/common_test.go (1)
285-294: Consider verifying the panic message and adding success path coverage.The panic test correctly uses defer/recover. Two optional improvements:
- Verify panic message: Assert the panic message matches the expected text from the implementation.
- Add success path test: Consider adding a test for
MustGetClusteredNamewhen cluster context is properly set.Apply this diff to verify the panic message:
func TestMustGetClusteredNamePanicsWithoutCluster(t *testing.T) { // Use a minimal runtime object implementing the required interface obj := &corev1alpha1.Account{} defer func() { - if r := recover(); r == nil { + r := recover() + if r == nil { t.Fatalf("expected panic when cluster missing in context") } + // Verify panic message matches expected text + if panicMsg, ok := r.(string); ok { + assert.Contains(t, panicMsg, "cluster not found in context") + } }() _ = MustGetClusteredName(context.Background(), obj) }For success path coverage, add:
func TestMustGetClusteredName_Success(t *testing.T) { // Create context with cluster information using mccontext // (implementation depends on how mccontext sets cluster in context) // Then verify MustGetClusteredName returns expected ClusteredName }pkg/subroutines/accountinfo_subroutine_test.go (3)
56-57: Use the same cluster-scoped ctx for reads for consistency.Prefer cl.Get(ctx, ...) instead of context.Background() to match cluster-scoped behavior.
47-47: Prefer explicit durations.Use time.Second (or time.Millisecond) instead of bare 1 for clarity.
- limiter: workqueue.NewTypedItemExponentialFailureRateLimiter[ClusteredName](1, 1), + limiter: workqueue.NewTypedItemExponentialFailureRateLimiter[ClusteredName](1*time.Second, 1*time.Second),Also applies to: 83-83
147-157: Duplicate coverage with TestAccountInfoProcessMissingOriginClusterAnnotation.Two tests assert the same behavior. Consider consolidating to one style (suite or standalone).
pkg/subroutines/workspace_subroutine_test.go (1)
234-244: Avoid panics for missing cluster in context; use typed error instead.Prefer returning an error (e.g., retryable=false) instead of panicking. If keeping panic, simplify the test using suite helpers.
Option A (improve test):
- defer func() { - if r := recover(); r == nil { s.T().Fatalf("expected panic when cluster missing in context") } - }() - _, _ = sub.Process(context.Background(), acc) + s.Panics(func() { _, _ = sub.Process(context.Background(), acc) })Option B (preferred behavior change): make Process validate context and return an operation error; update test to assert error instead of panic.
pkg/subroutines/accountinfo_test.go (4)
34-44: Drop unused logger from the suite (simplify setup).
s.logis never used. Remove the field, the import, and the initialization to speed up tests and reduce noise.@@ type AccountInfoSubroutineTestSuite struct { suite.Suite - scheme *runtime.Scheme - ctx context.Context - log *logger.Logger + scheme *runtime.Scheme + ctx context.Context } @@ -import ( +import ( "context" - "errors" + "errors" "testing" "time" @@ - "github.com/platform-mesh/golang-commons/logger" "github.com/stretchr/testify/suite" @@ func (s *AccountInfoSubroutineTestSuite) SetupSuite() { s.scheme = runtime.NewScheme() @@ - var err error - s.log, err = logger.New(logger.DefaultConfig()) - s.Require().NoError(err) s.ctx = mccontext.WithCluster(context.Background(), "cluster-test") }Also applies to: 23-28, 11-12
50-59: Helper newWorkspace is fine; consider TypeMeta for robustness.Optional: set TypeMeta for clearer object identity in fake client dumps.
func newWorkspace(name, phase, cluster, url string) *kcptenancyv1alpha.Workspace { - ws := &kcptenancyv1alpha.Workspace{ObjectMeta: metav1.ObjectMeta{Name: name}} + ws := &kcptenancyv1alpha.Workspace{ + TypeMeta: metav1.TypeMeta{APIVersion: "tenancy.kcp.io/v1alpha1", Kind: "Workspace"}, + ObjectMeta: metav1.ObjectMeta{Name: name}, + } ws.Spec.Cluster = cluster ws.Spec.URL = url if phase != "" { ws.Status.Phase = kcpcorev1alpha.LogicalClusterPhaseType(phase) } return ws }
93-121: Parent inheritance test is good; tighten cluster realism.Current fake getter returns a single client instance. This can mask bugs in cross-cluster lookups. Consider enhancing fakeClusterGetter to return per-cluster clients (map[clusterID]client.Client) to better simulate parent/child workspaces.
123-134: Assert error kind for missing parent (improve signal).Also check the error type/message (e.g., via
errors.Isor substring) to ensure the failure reason is precise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/subroutines/accountinfo_subroutine_test.go(1 hunks)pkg/subroutines/accountinfo_test.go(1 hunks)pkg/subroutines/common_test.go(4 hunks)pkg/subroutines/fga_subroutine_test.go(1 hunks)pkg/subroutines/workspace_subroutine_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/subroutines/fga_subroutine_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/subroutines/common_test.go (2)
api/v1alpha1/account_types.go (1)
Account(81-87)pkg/subroutines/clusteredname.go (1)
MustGetClusteredName(31-36)
pkg/subroutines/accountinfo_subroutine_test.go (6)
api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/accountinfo.go (3)
AccountInfoSubroutine(33-38)DefaultAccountInfoName(30-30)NewAccountInfoSubroutine(40-43)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (3)
AccountInfo(65-71)ClusterInfo(32-34)AccountLocation(36-46)pkg/subroutines/accountinfo_test.go (1)
AccountInfoSubroutineTestSuite(23-28)
pkg/subroutines/workspace_subroutine_test.go (5)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/workspace.go (3)
NewWorkspaceSubroutine(43-46)WorkspaceSubroutine(32-41)WorkspaceSubroutineFinalizer(29-29)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)api/v1alpha1/account_info_types.go (1)
AccountInfo(65-71)pkg/subroutines/accountinfo.go (1)
DefaultAccountInfoName(30-30)
pkg/subroutines/accountinfo_test.go (4)
api/v1alpha1/account_types.go (4)
Account(81-87)AccountSpec(34-53)AccountTypeOrg(27-27)AccountTypeAccount(28-28)pkg/subroutines/accountinfo.go (3)
NewAccountInfoSubroutine(40-43)DefaultAccountInfoName(30-30)AccountInfoSubroutine(33-38)api/v1alpha1/account_info_types.go (3)
AccountInfo(65-71)ClusterInfo(32-34)AccountLocation(36-46)pkg/subroutines/clusteredname.go (1)
ClusteredName(12-15)
🔇 Additional comments (15)
pkg/subroutines/common_test.go (3)
132-132: LGTM: Mock.Anything is appropriate here.The change from
mock.AnythingOfType("*v1alpha1.Workspace")tomock.Anythingreduces type-string coupling while maintaining type safety through theRun()function's cast to*kcptenancyv1alpha.Workspace. This is a reasonable tradeoff that makes the tests less brittle to internal refactoring.Also applies to: 158-158, 178-178
261-272: retrieveWorkspace correctly handles nil inputs
LGTM—the implementation returns errors with "account is nil" and "client is nil" as expected.
274-283: Nil client defensive check verified
The implementation in pkg/subroutines/common.go returnsnil, fmt.Errorf("client is nil"), satisfying the test’s expectation. No changes required.pkg/subroutines/accountinfo_subroutine_test.go (2)
154-154: Passing nil client into NewAccountInfoSubroutine.Likely fine since error occurs before client use, but please verify Process never dereferences sub.client on this path.
110-115: staticLimiter already defined and implements TypedRateLimiter[ClusteredName].
Definition found in pkg/subroutines/limiters_test.go; no changes required.pkg/subroutines/workspace_subroutine_test.go (1)
315-317: Verify AccountInfo resource scope
We couldn’t detect whetherAccountInfois cluster-scoped or namespaced—please check its+kubebuilder:resourceannotation (e.g. in theaccount_info_types.goAPI file). If it’s cluster-scoped, omit theNamespace: "default"field in the test setup.pkg/subroutines/accountinfo_test.go (9)
46-48: Fake client builder looks good.Scheme wired correctly; using WithObjects keeps tests fast and hermetic.
61-79: Org create path test is clear and effective.Covers immediate success and validates key fields, including CA.
81-91: Retry on not-ready workspace covered well.Asserting positive RequeueAfter is appropriate.
136-146: ClusterGetter usage test looks good.Verifies mccontext-based client selection.
148-155: ClusterGetter error path covered.Good negative test.
157-176: Finalize requeue behavior tested deterministically.Custom limiter avoids flakiness; assertions are appropriate.
178-182: Nil runtime object check is appropriate.Covers defensive path.
184-190: Static limiter test is solid.Exact delay assertion demonstrates limiter plumbed correctly.
192-221: Workspace URL path parsing tests are comprehensive.Covers happy path and key error cases (empty URL, trailing slash, too-few segments).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/download-kcp.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / dockerBuild / docker
| if [[ "$KCP_VERSION" != v* ]]; then | ||
| KCP_VERSION="v${KCP_VERSION}" | ||
| fi |
There was a problem hiding this comment.
Don’t force a v prefix onto arbitrary overrides.
Prefixing every override with v breaks common values like main, latest, or raw commit SHAs. The previous flow allowed those; now they become vmain, etc., and go install fails. Relax the normalization so only plain semver numbers are prefixed.
-if [[ "$KCP_VERSION" != v* ]]; then
- KCP_VERSION="v${KCP_VERSION}"
-fi
+if [[ "$KCP_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-.*)?$ ]] && [[ "$KCP_VERSION" != v* ]]; then
+ KCP_VERSION="v${KCP_VERSION}"
+fi📝 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.
| if [[ "$KCP_VERSION" != v* ]]; then | |
| KCP_VERSION="v${KCP_VERSION}" | |
| fi | |
| if [[ "$KCP_VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-.*)?$ ]] && [[ "$KCP_VERSION" != v* ]]; then | |
| KCP_VERSION="v${KCP_VERSION}" | |
| fi |
🤖 Prompt for AI Agents
In hack/download-kcp.sh around lines 17-19, the script unconditionally prefixes
KCP_VERSION with "v", which corrupts non-semver identifiers like "main",
"latest", or SHAs; change the normalization so only plain numeric semver-ish
values get a "v" prefix by testing KCP_VERSION with a regex that matches numeric
version patterns (e.g. starts with a digit and contains one or more
dot-separated numeric components) and only then set
KCP_VERSION="v${KCP_VERSION}".
…ated its constructor and usages
…by eagerly creating and caching the orgs client in the constructor
#35
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests