Open
Conversation
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
aaronschweig
approved these changes
Jan 26, 2026
Contributor
There was a problem hiding this comment.
LGTM 👍
@nexus49 is this something we want to merge this now and include it in the 0.2 release?
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
Contributor
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds explicit initializer controllers for org and account LogicalCluster resources and refactors existing reconcilers and subroutines to use these initializers. Predicates (including a HasInitializerPredicate) route clusters to initializer vs reconciler flows and to account vs org types. Removes direct multicluster provider/client dependencies from command entry points and many subroutines; cluster-scoped clients are now obtained from the manager via context. Identity provider and workspace initializer logic simplified (consolidated CreateOrUpdate and fewer per-client patches). Several types, constructors, and SetupWithManager signatures were renamed or adjusted to accept initializer names. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/subroutine/account_tuples.go (1)
116-130: 🛠️ Refactor suggestion | 🟠 MajorInconsistent client retrieval pattern between Initialize and Terminate.
The
Initializemethod (lines 58-61) usess.mgr.ClusterFromContext(ctx)for client retrieval, butTerminatestill uses the oldiclient.NewForLogicalClusterpattern. This inconsistency should be addressed for maintainability and alignment with the PR's refactoring goals.♻️ Proposed fix to use consistent ClusterFromContext pattern
// Remove finalizer from AccountInfo. if updated := controllerutil.RemoveFinalizer(&ai, accountTuplesTerminatorFinalizer); updated { - lcID, ok := mccontext.ClusterFrom(ctx) - if !ok { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("cluster name not found in context"), true, true) - } - - lcClient, err := iclient.NewForLogicalCluster(s.mgr.GetLocalManager().GetConfig(), s.mgr.GetLocalManager().GetScheme(), logicalcluster.Name(lcID)) + cl, err := s.mgr.ClusterFromContext(ctx) if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("getting client: %w", err), true, true) + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get cluster from context: %w", err), true, true) } - if err := lcClient.Update(ctx, &ai); err != nil { + if err := cl.GetClient().Update(ctx, &ai); err != nil { return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("updating AccountInfo to remove finalizer: %w", err), true, true) } }This would also allow removing the
iclient,mccontext, andlogicalclusterimports if no longer needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/account_tuples.go` around lines 116 - 130, The Terminate block currently creates a logical-cluster client via iclient.NewForLogicalCluster (inside the controllerutil.RemoveFinalizer branch); replace that with the same pattern used in Initialize by calling s.mgr.ClusterFromContext(ctx) (or s.mgr.ClusterFromContext to match the existing method) to obtain the logical-cluster client and cluster ID, handle the missing-cluster case the same way Initialize does, then call lcClient.Update(ctx, &ai) to persist the finalizer removal; after changing the retrieval, remove the now-unused iclient, mccontext, and logicalcluster imports if they become unused.
🧹 Nitpick comments (6)
internal/subroutine/workspace_initializer.go (1)
155-158: DuplicateClusterFromContextcall.The cluster is already retrieved on line 87 and stored in
cl. This second call on line 155 is redundant. Reuse the existingclvariable.♻️ Proposed fix
- cluster, err := w.mgr.ClusterFromContext(ctx) - if err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("unable to get cluster from context: %w", err), true, false) - } - accountInfo := accountsv1alpha1.AccountInfo{ ObjectMeta: metav1.ObjectMeta{Name: "account"}, } - _, err = controllerutil.CreateOrUpdate(ctx, cluster.GetClient(), &accountInfo, func() error { + _, err = controllerutil.CreateOrUpdate(ctx, cl.GetClient(), &accountInfo, func() error { accountInfo.Spec.FGA.Store.Id = store.Status.StoreID return nil })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/workspace_initializer.go` around lines 155 - 158, Remove the redundant call to w.mgr.ClusterFromContext(ctx) and the local variable cluster; reuse the previously obtained cl variable (from the earlier ClusterFromContext call) wherever cluster is used in this block. Replace references to cluster with cl and remove the duplicated error handling for ClusterFromContext so there is only the original retrieval/err check that populated cl.internal/subroutine/idp.go (1)
106-139: Redundant Get after CreateOrUpdate.The
controllerutil.CreateOrUpdatefunction updates the passedidpobject in place with the server's response, including the currentresourceVersion. The subsequentGetcall on line 137 is unnecessary and adds latency.♻️ Proposed fix to remove redundant Get
if err != nil { return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to create idp resource %w", err), true, true) } log.Info().Str("workspace", workspaceName).Msg("idp configuration resource is created") - if err := cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp); err != nil { - return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("failed to get idp resource %w", err), true, true) - } - if !meta.IsStatusConditionTrue(idp.GetConditions(), "Ready") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/idp.go` around lines 106 - 139, The CreateOrUpdate call on the idp object (controllerutil.CreateOrUpdate with idp) already returns the updated object in-place (including resourceVersion), so remove the redundant cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp) call and its error handling; rely on the idp instance populated by CreateOrUpdate and continue using that variable (and its metadata) for any subsequent logic or logging to avoid the extra API call and latency.internal/controller/accountlogicalcluster_controller.go (1)
23-35: Constructor name does not match return type.The constructor
NewAccountLogicalClusterReconcilerreturns*AccountTypeLogicalClusterReconciler. This naming inconsistency is confusing. Consider renaming toNewAccountTypeLogicalClusterReconcilerfor clarity and consistency.♻️ Proposed fix
-func NewAccountLogicalClusterReconciler(log *logger.Logger, cfg config.Config, fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager) *AccountTypeLogicalClusterReconciler { +func NewAccountTypeLogicalClusterReconciler(log *logger.Logger, cfg config.Config, fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager) *AccountTypeLogicalClusterReconciler {Note: This would require updating call sites in
cmd/operator.go(line 207) and potentially other files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/accountlogicalcluster_controller.go` around lines 23 - 35, The constructor function name NewAccountLogicalClusterReconciler does not match the returned type AccountTypeLogicalClusterReconciler; rename the constructor to NewAccountTypeLogicalClusterReconciler and update all call sites to use the new name, ensuring any references to NewAccountLogicalClusterReconciler are replaced; keep the function signature and returned struct (AccountTypeLogicalClusterReconciler, fields log and mclifecycle built via builder.NewBuilder(...).BuildMultiCluster(mgr)) unchanged so only the constructor identifier is updated for consistency.internal/subroutine/account_tuples.go (1)
175-175: Extraneous blank line.Line 175 contains a trailing whitespace/blank line that should be removed for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/account_tuples.go` at line 175, Remove the extraneous trailing blank line at line 175 in internal/subroutine/account_tuples.go to eliminate the unnecessary whitespace; open the file, locate the blank/whitespace-only line near the surrounding function or type declaration in account_tuples.go (e.g., within the account tuple-related functions) and delete that empty line so the file has no trailing blank line at that location.internal/controller/orglogicalcluster_initializer_controller.go (1)
47-47: Use initializer-specific builder name for consistent observability.Line 47 passes
"OrgLogicalClusterReconciler"even though this controller is an initializer. Rename it to keep logs and metrics aligned with the actual controller role.Proposed fix
- mclifecycle: builder.NewBuilder("logicalcluster", "OrgLogicalClusterReconciler", subroutines, log). + mclifecycle: builder.NewBuilder("logicalcluster", "OrgLogicalClusterInitializer", subroutines, log).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/orglogicalcluster_initializer_controller.go` at line 47, The builder invocation using builder.NewBuilder currently uses the reconciler name "OrgLogicalClusterReconciler" which is incorrect for this initializer; update the string argument to an initializer-specific name (e.g. "OrgLogicalClusterInitializer" or "OrgLogicalClusterInitializerController") so logs/metrics align with the controller's role where mclifecycle is constructed via builder.NewBuilder("logicalcluster", "...", subroutines, log).internal/controller/accountlogicalcluster_initializer_controller.go (1)
23-34: Align controller naming to initializer semantics.Line 23 and Line 33 still use
Reconcilernaming although this type is an initializer. This makes logs/metrics/controller identity harder to trace.Proposed fix
-// AccountLogicalClusterReconciler acts as an initializer for account workspaces. +// AccountLogicalClusterInitializer acts as an initializer for account workspaces. @@ - mclifecycle: builder.NewBuilder("security", "AccountLogicalClusterReconciler", []lifecyclesubroutine.Subroutine{ + mclifecycle: builder.NewBuilder("security", "AccountLogicalClusterInitializer", []lifecyclesubroutine.Subroutine{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/accountlogicalcluster_initializer_controller.go` around lines 23 - 34, The controller name still uses "Reconciler" which conflicts with the initializer semantics; update the identifier passed into builder.NewBuilder to use "AccountLogicalClusterInitializer" (and any other occurrences of "AccountLogicalClusterReconciler") so logs/metrics and controller identity reflect the initializer. Specifically, edit the NewAccountLogicalClusterInitializer function and change the name argument in builder.NewBuilder from "AccountLogicalClusterReconciler" to "AccountLogicalClusterInitializer" and replace any other literal or symbol usages of "Reconciler" related to AccountLogicalCluster with "Initializer".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/accountlogicalcluster_controller.go`:
- Around line 3-21: Reorder the import block in
internal/controller/accountlogicalcluster_controller.go to satisfy the project's
gci rules: run the project's import formatter (or gci) to group and sort imports
(standard library first like context, then external modules such as
kcpcorev1alpha1 and openfgav1, then github.com/platform-mesh/... and
local/internal packages), ensuring symbols like kcpcorev1alpha1, openfgav1,
platformeshconfig, builder, multicluster, lifecyclesubroutine, logger, config,
predicates, subroutine, ctrl, predicate, mccontext, mcmanager, and mcreconcile
remain imported but correctly ordered.
In `@internal/controller/orglogicalcluster_controller.go`:
- Around line 60-63: The SetupWithManager method constructs a
HasInitializerPredicate from initializerName without validating it, which can
silently drop events if initializerName is empty; add a guard at the start of
LogicalClusterReconciler.SetupWithManager to return an error when
initializerName == "" (use a clear fmt.Errorf or errors.New message) so the
manager setup fails fast, and keep the rest of the function unchanged (still
build allPredicates and call r.mclifecycle.SetupWithManager).
In `@internal/predicates/initializer.go`:
- Around line 14-28: Replace the unchecked type assertions in the predicate
handlers (CreateFunc, UpdateFunc, DeleteFunc, GenericFunc) with safe type checks
before calling shouldReconcile: use the comma-ok form to assert that e.Object
(and e.ObjectNew for UpdateFunc) is a *kcpcorev1alpha1.LogicalCluster, return
false if the cast fails, and only then call shouldReconcile with the typed
value; ensure UpdateFunc handles e.ObjectNew safely and DeleteFunc/GenericFunc
also guard against nil or unexpected types to avoid panics.
- Around line 3-9: The import block in initializer.go is mis-grouped causing gci
lint failures; reorder the imports into the configured groups (standard library
first: "slices", then third-party modules like
"sigs.k8s.io/controller-runtime/pkg/event" and
"sigs.k8s.io/controller-runtime/pkg/predicate", then project/other prefixed
imports such as kcpcorev1alpha1 "github.com/kcp-dev/sdk/apis/core/v1alpha1") or
run your gci formatter (e.g. gci write -s standard -s default -s
"prefix(github.com/platform-mesh)" internal/predicates/initializer.go) to
automatically fix the grouping.
In `@internal/subroutine/account_tuples.go`:
- Around line 171-173: The error returned from mgr.ClusterFromContext(ctx) in
accountAndInfoForLogicalCluster is being discarded; update the error handling to
wrap the original err when constructing the OperatorError so the underlying
cause is preserved (i.e., pass the original err into fmt.Errorf or into
errors.NewOperatorError rather than creating a new generic error). Locate the
accountAndInfoForLogicalCluster function and the call to
mgr.ClusterFromContext(ctx) and modify the return to include the original error
(err) inside the OperatorError so callers can inspect the root cause.
In `@internal/subroutine/idp.go`:
- Line 112: The RedirectURIs assignment is appending directly to the config
slice i.additionalRedirectURLs which can mutate/shared state; instead make a
fresh slice copy of i.additionalRedirectURLs before adding the formatted URL so
the original config isn't modified (e.g., create a new slice from
i.additionalRedirectURLs and then append fmt.Sprintf("https://%s.%s/*",
workspaceName, i.baseDomain) to that copy) and assign that new slice to
RedirectURIs; update the code around the RedirectURIs field where
i.additionalRedirectURLs is referenced to use this non-mutating copy.
In `@internal/subroutine/workspace_initializer.go`:
- Around line 87-90: The returned error discards the original diagnostic
context; update the error construction where you call
w.mgr.ClusterFromContext(ctx) so the original err is wrapped/propagated into
errors.NewOperatorError (i.e., include the original err when creating the
OperatorError returned from that block). Locate the call to
w.mgr.ClusterFromContext(ctx) and change the errors.NewOperatorError invocation
to pass a wrapped error (or fmt.Errorf with %w) that includes the original err
so the underlying cause is preserved.
---
Outside diff comments:
In `@internal/subroutine/account_tuples.go`:
- Around line 116-130: The Terminate block currently creates a logical-cluster
client via iclient.NewForLogicalCluster (inside the
controllerutil.RemoveFinalizer branch); replace that with the same pattern used
in Initialize by calling s.mgr.ClusterFromContext(ctx) (or
s.mgr.ClusterFromContext to match the existing method) to obtain the
logical-cluster client and cluster ID, handle the missing-cluster case the same
way Initialize does, then call lcClient.Update(ctx, &ai) to persist the
finalizer removal; after changing the retrieval, remove the now-unused iclient,
mccontext, and logicalcluster imports if they become unused.
---
Nitpick comments:
In `@internal/controller/accountlogicalcluster_controller.go`:
- Around line 23-35: The constructor function name
NewAccountLogicalClusterReconciler does not match the returned type
AccountTypeLogicalClusterReconciler; rename the constructor to
NewAccountTypeLogicalClusterReconciler and update all call sites to use the new
name, ensuring any references to NewAccountLogicalClusterReconciler are
replaced; keep the function signature and returned struct
(AccountTypeLogicalClusterReconciler, fields log and mclifecycle built via
builder.NewBuilder(...).BuildMultiCluster(mgr)) unchanged so only the
constructor identifier is updated for consistency.
In `@internal/controller/accountlogicalcluster_initializer_controller.go`:
- Around line 23-34: The controller name still uses "Reconciler" which conflicts
with the initializer semantics; update the identifier passed into
builder.NewBuilder to use "AccountLogicalClusterInitializer" (and any other
occurrences of "AccountLogicalClusterReconciler") so logs/metrics and controller
identity reflect the initializer. Specifically, edit the
NewAccountLogicalClusterInitializer function and change the name argument in
builder.NewBuilder from "AccountLogicalClusterReconciler" to
"AccountLogicalClusterInitializer" and replace any other literal or symbol
usages of "Reconciler" related to AccountLogicalCluster with "Initializer".
In `@internal/controller/orglogicalcluster_initializer_controller.go`:
- Line 47: The builder invocation using builder.NewBuilder currently uses the
reconciler name "OrgLogicalClusterReconciler" which is incorrect for this
initializer; update the string argument to an initializer-specific name (e.g.
"OrgLogicalClusterInitializer" or "OrgLogicalClusterInitializerController") so
logs/metrics align with the controller's role where mclifecycle is constructed
via builder.NewBuilder("logicalcluster", "...", subroutines, log).
In `@internal/subroutine/account_tuples.go`:
- Line 175: Remove the extraneous trailing blank line at line 175 in
internal/subroutine/account_tuples.go to eliminate the unnecessary whitespace;
open the file, locate the blank/whitespace-only line near the surrounding
function or type declaration in account_tuples.go (e.g., within the account
tuple-related functions) and delete that empty line so the file has no trailing
blank line at that location.
In `@internal/subroutine/idp.go`:
- Around line 106-139: The CreateOrUpdate call on the idp object
(controllerutil.CreateOrUpdate with idp) already returns the updated object
in-place (including resourceVersion), so remove the redundant
cl.GetClient().Get(ctx, types.NamespacedName{Name: workspaceName}, idp) call and
its error handling; rely on the idp instance populated by CreateOrUpdate and
continue using that variable (and its metadata) for any subsequent logic or
logging to avoid the extra API call and latency.
In `@internal/subroutine/workspace_initializer.go`:
- Around line 155-158: Remove the redundant call to
w.mgr.ClusterFromContext(ctx) and the local variable cluster; reuse the
previously obtained cl variable (from the earlier ClusterFromContext call)
wherever cluster is used in this block. Replace references to cluster with cl
and remove the duplicated error handling for ClusterFromContext so there is only
the original retrieval/err check that populated cl.
ℹ️ Review info
Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/initializer.gocmd/operator.gocmd/terminator.gointernal/controller/accountlogicalcluster_controller.gointernal/controller/accountlogicalcluster_initializer_controller.gointernal/controller/orglogicalcluster_controller.gointernal/controller/orglogicalcluster_initializer_controller.gointernal/predicates/initializer.gointernal/subroutine/account_tuples.gointernal/subroutine/idp.gointernal/subroutine/workspace_initializer.go
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/controller/accountlogicalcluster_controller.go (1)
29-30: Align constructor name with returned type.The constructor name and returned type now diverge, which makes the API surface less consistent.
Proposed rename
-func NewAccountLogicalClusterReconciler(log *logger.Logger, cfg config.Config, fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager) *AccountTypeLogicalClusterReconciler { +func NewAccountTypeLogicalClusterReconciler(log *logger.Logger, cfg config.Config, fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager) *AccountTypeLogicalClusterReconciler {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/accountlogicalcluster_controller.go` around lines 29 - 30, The constructor function NewAccountLogicalClusterReconciler does not match the returned type *AccountTypeLogicalClusterReconciler; rename the constructor to NewAccountTypeLogicalClusterReconciler (or alternatively change the returned type to AccountLogicalClusterReconciler) so the function name and the concrete type align—update the function declaration and any callers to use NewAccountTypeLogicalClusterReconciler to keep the API surface consistent.internal/controller/orglogicalcluster_initializer_controller.go (1)
47-47: Use an initializer-specific lifecycle identifier.The builder name still uses a reconciler label in initializer wiring; this can blur logs/metrics attribution.
Proposed tweak
- mclifecycle: builder.NewBuilder("logicalcluster", "OrgLogicalClusterReconciler", subroutines, log). + mclifecycle: builder.NewBuilder("logicalcluster", "OrgLogicalClusterInitializer", subroutines, log).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/orglogicalcluster_initializer_controller.go` at line 47, Replace the reconciler-centric lifecycle name used in the initializer wiring so logs/metrics reflect the initializer; update the builder.NewBuilder call (currently using "logicalcluster", "OrgLogicalClusterReconciler") to use an initializer-specific identifier (e.g., "logicalcluster", "OrgLogicalClusterInitializer" or similar) where mclifecycle is created, keeping the same subroutines and log variables so only the name changes.internal/subroutine/account_tuples.go (1)
52-63: Avoid resolving cluster context twice in Initialize.
accountAndInfoForLogicalClusteralready resolves cluster from context. Resolving it again beforeUpdateadds another failure point; consider returning the resolved cluster/client from the helper and reusing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/subroutine/account_tuples.go` around lines 52 - 63, The helper accountAndInfoForLogicalCluster currently resolves the cluster but the caller re-resolves it again before updating; change accountAndInfoForLogicalCluster to return the resolved cluster or client along with acc and ai (e.g., return values acc, ai, cl, opErr) and then reuse that returned cluster/client for the Update call instead of calling s.mgr.ClusterFromContext(ctx) again so the Update uses the already-resolved cluster (replace the s.mgr.ClusterFromContext call and cl variable with the returned cluster/client).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/accountlogicalcluster_initializer_controller.go`:
- Around line 23-28: The top-of-file comment incorrectly references
"AccountLogicalClusterReconciler"; update that comment to use the actual type
name "AccountLogicalClusterInitializer" so it reads e.g.
"AccountLogicalClusterInitializer acts as an initializer for account
workspaces." and keep the rest of the comment unchanged to maintain clarity
around the AccountLogicalClusterInitializer type and its fields (log and
mclifecycle).
In `@internal/predicates/initializer.go`:
- Around line 17-19: The panic message inside HasInitializerPredicate
incorrectly references "LogicalClusterIsAccountTypeOrg"; update the error text
to accurately name HasInitializerPredicate (or a generic
"HasInitializerPredicate received non-LogicalCluster resource") so the panic
reflects the correct predicate; locate the panic in function
HasInitializerPredicate and change the fmt.Errorf string accordingly to match
the predicate name.
In `@internal/subroutine/idp.go`:
- Around line 119-127: The kubectl OAuth client block is setting SecretRef even
though ClientType is IdentityProviderClientTypePublic (which uses
TokenEndpointAuthMethodNone); remove the SecretRef field from the kubectl client
literal (or wrap it so SecretRef is only added when ClientType is confidential)
so that the public client object (the one using i.kubectlClientRedirectURLs and
kubectlClientName) does not include SecretRef; update the literal that builds
the client (the struct with ClientName/ClientType/RedirectURIs) accordingly and
ensure only the confidential client retains its SecretRef.
---
Nitpick comments:
In `@internal/controller/accountlogicalcluster_controller.go`:
- Around line 29-30: The constructor function NewAccountLogicalClusterReconciler
does not match the returned type *AccountTypeLogicalClusterReconciler; rename
the constructor to NewAccountTypeLogicalClusterReconciler (or alternatively
change the returned type to AccountLogicalClusterReconciler) so the function
name and the concrete type align—update the function declaration and any callers
to use NewAccountTypeLogicalClusterReconciler to keep the API surface
consistent.
In `@internal/controller/orglogicalcluster_initializer_controller.go`:
- Line 47: Replace the reconciler-centric lifecycle name used in the initializer
wiring so logs/metrics reflect the initializer; update the builder.NewBuilder
call (currently using "logicalcluster", "OrgLogicalClusterReconciler") to use an
initializer-specific identifier (e.g., "logicalcluster",
"OrgLogicalClusterInitializer" or similar) where mclifecycle is created, keeping
the same subroutines and log variables so only the name changes.
In `@internal/subroutine/account_tuples.go`:
- Around line 52-63: The helper accountAndInfoForLogicalCluster currently
resolves the cluster but the caller re-resolves it again before updating; change
accountAndInfoForLogicalCluster to return the resolved cluster or client along
with acc and ai (e.g., return values acc, ai, cl, opErr) and then reuse that
returned cluster/client for the Update call instead of calling
s.mgr.ClusterFromContext(ctx) again so the Update uses the already-resolved
cluster (replace the s.mgr.ClusterFromContext call and cl variable with the
returned cluster/client).
ℹ️ Review info
Configuration used: Repository: platform-mesh/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Taskfile.yamlcmd/initializer.gocmd/operator.gocmd/terminator.gointernal/controller/accountlogicalcluster_controller.gointernal/controller/accountlogicalcluster_initializer_controller.gointernal/controller/apibinding_controller.gointernal/controller/orglogicalcluster_controller.gointernal/controller/orglogicalcluster_initializer_controller.gointernal/predicates/initializer.gointernal/subroutine/account_tuples.gointernal/subroutine/idp.gointernal/subroutine/workspace_initializer.go
💤 Files with no reviewable changes (1)
- internal/controller/apibinding_controller.go
internal/controller/accountlogicalcluster_initializer_controller.go
Outdated
Show resolved
Hide resolved
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
On-behalf-of: SAP aleh.yarshou@sap.com
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
3 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.
On-behalf-of: SAP aleh.yarshou@sap.com
This pr is related to #175.
Changes: