Skip to content

[chore] advance architecture refactor#864

Merged
ldmonster merged 6 commits intomainfrom
chore/advance-architecture-refactor
Apr 10, 2026
Merged

[chore] advance architecture refactor#864
ldmonster merged 6 commits intomainfrom
chore/advance-architecture-refactor

Conversation

@ldmonster
Copy link
Copy Markdown
Collaborator

@ldmonster ldmonster commented Apr 8, 2026

Overview

Extract interfaces and named types to decouple ShellOperator's core components, replace an untyped task-type switch with a registry, and remove dead code. All changes are backward-compatible; no public API is broken.

What this PR does / why we need it

HookManager interface (pkg/hook/hook_manager.go)

  • Defined a HookManager interface capturing the 10 methods used by ShellOperator.
  • Changed ShellOperator.HookManager from *hook.Manager (concrete) to hook.HookManager (interface).
  • Added var _ HookManager = (*Manager)(nil) compile-time assertion.
  • Effect: ShellOperator no longer imports the concrete Manager; tests can inject a stub with zero real dependencies.

HookDiscovery interface (pkg/hook/hook_manager.go)

  • Extracted HookDiscovery interface with a single Discover(workingDir string) ([]string, error) method.
  • Added FileSystemHookDiscovery as the default implementation (wraps the existing utils_file.RecursiveGetExecutablePaths call).
  • Manager.Init() now calls hm.hookDiscovery.Discover(...) instead of the utility function directly.
  • ManagerConfig.HookDiscovery field added; nil defaults to FileSystemHookDiscovery{}.
  • Effect: filesystem coupling removed from Manager.Init; tests can provide a deterministic stub without touching the real filesystem.

Named webhook event handler types (pkg/shell-operator/webhook_handlers.go, new file)

  • Introduced TaskRunner type alias (func(ctx, task) TaskResult).
  • Extracted AdmissionEventHandler struct with constructor NewAdmissionEventHandler(hm, runner, logger) and Handle method matching the WithAdmissionEventHandler signature.
  • Extracted ConversionEventHandler struct with constructor NewConversionEventHandler(hm, runner, logger) and Handle method matching EventHandlerFn.
  • Replaced the ~45-line inline closure in initValidatingWebhookManager with NewAdmissionEventHandler(...).Handle.
  • Replaced the dead conversionEventHandler method (which also had a return res undefined-variable compile error from the merge) and the inline-style call in initConversionWebhookManager with NewConversionEventHandler(...).Handle.
  • Effect: event-handling logic is now independently testable, named, and dependency-injected.

TaskHandlerRegistry (pkg/shell-operator/task_handler_registry.go, new file)

  • Defined TaskHandlerFunc and TaskHandlerRegistry (map from task.TaskType to handler).
  • NewTaskHandlerRegistry(), Register(taskType, handler), Handle(ctx, task) (TaskResult, bool).
  • Added taskHandlerRegistry *TaskHandlerRegistry field to ShellOperator.
  • Added taskHandler(ctx, task) TaskResult dispatcher method (thin wrapper over the registry).
  • Added RegisterBuiltinTaskHandlers() method registering HookRun, EnableKubernetesBindings, and EnableScheduleBindings; called from assembleShellOperator in bootstrap.go.
  • Extracted taskHandleEnableScheduleBindings from the switch as a proper method alongside the existing taskHandleHookRun and taskHandleEnableKubernetesBindings.
  • Effect: adding a new task type no longer requires editing a central switch; addon-operator can call Register() to extend without forking.

Dead code removal

  • CommonHook interface (pkg/hook/hook.go): was never referenced outside its own declaration — removed.
  • FlagInfo.Define bool field (pkg/app/app.go): all callers always passed true; the if flag.Define { } branches were unconditional in practice — field and branches removed, flag registration is now always unconditional.
  • conversionEventHandler method on ShellOperator: superseded by ConversionEventHandler — removed.
  • v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" import in operator.go: became unused after the above removal — removed.

Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
@ldmonster ldmonster self-assigned this Apr 8, 2026
…vance-architecture-refactor

Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
@ldmonster ldmonster merged commit 4f12b98 into main Apr 10, 2026
8 of 9 checks passed
@ldmonster ldmonster deleted the chore/advance-architecture-refactor branch April 10, 2026 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant