[chore] advance architecture refactor

pkg/app/app.go

@@ -28,108 +28,86 @@ var (
 var PrometheusMetricsPrefix = "shell_operator_"
 
 type FlagInfo struct {
-	Name   string
-	Help   string
-	Envar  string
-	Define bool
+	Name  string
+	Help  string
+	Envar string
 }
 
 var CommonFlagsInfo = map[string]FlagInfo{
 	"hooks-dir": {
 		"hooks-dir",
 		"A path to a hooks file structure. Can be set with $SHELL_OPERATOR_HOOKS_DIR.",
 		"SHELL_OPERATOR_HOOKS_DIR",
-		true,
 	},
 	"tmp-dir": {
 		"tmp-dir",
 		"A path to store temporary files with data for hooks. Can be set with $SHELL_OPERATOR_TMP_DIR.",
 		"SHELL_OPERATOR_TMP_DIR",
-		true,
 	},
 	"listen-address": {
 		"listen-address",
 		"Address to use to serve metrics to Prometheus. Can be set with $SHELL_OPERATOR_LISTEN_ADDRESS.",
 		"SHELL_OPERATOR_LISTEN_ADDRESS",
-		true,
 	},
 	"listen-port": {
 		"listen-port",
 		"Port to use to serve metrics to Prometheus. Can be set with $SHELL_OPERATOR_LISTEN_PORT.",
 		"SHELL_OPERATOR_LISTEN_PORT",
-		true,
 	},
 	"prometheus-metrics-prefix": {
 		"prometheus-metrics-prefix",
 		"Prefix for Prometheus metrics. Can be set with $SHELL_OPERATOR_PROMETHEUS_METRICS_PREFIX.",
 		"SHELL_OPERATOR_PROMETHEUS_METRICS_PREFIX",
-		true,
 	},
 	"hook-metrics-listen-port": {
 		"hook-metrics-listen-port",
 		"Port to use to serve hooks’ custom metrics to Prometheus. Can be set with $SHELL_OPERATOR_HOOK_METRICS_LISTEN_PORT. Equal to listen-port if empty.",
 		"SHELL_OPERATOR_HOOK_METRICS_LISTEN_PORT",
-		true,
 	},
 	"namespace": {
 		"namespace",
 		"A namespace of a shell-operator. Used to setup validating webhooks. Can be set with $SHELL_OPERATOR_NAMESPACE.",
 		"SHELL_OPERATOR_NAMESPACE",
-		true,
 	},
 }
 
 // DefineStartCommandFlags set shell-operator flags for cmd
 func DefineStartCommandFlags(kpApp *kingpin.Application, cmd *kingpin.CmdClause) {
-	var flag FlagInfo
-
-	flag = CommonFlagsInfo["hooks-dir"]
-	if flag.Define {
-		cmd.Flag(flag.Name, flag.Help).
-			Envar(flag.Envar).
-			Default(HooksDir).
-			StringVar(&HooksDir)
-	}
+	flag := CommonFlagsInfo["hooks-dir"]
+	cmd.Flag(flag.Name, flag.Help).
+		Envar(flag.Envar).
+		Default(HooksDir).
+		StringVar(&HooksDir)
 
 	flag = CommonFlagsInfo["tmp-dir"]
-	if flag.Define {
-		cmd.Flag(flag.Name, flag.Help).
-			Envar(flag.Envar).
-			Default(TempDir).
-			StringVar(&TempDir)
-	}
+	cmd.Flag(flag.Name, flag.Help).
+		Envar(flag.Envar).
+		Default(TempDir).
+		StringVar(&TempDir)
 
 	flag = CommonFlagsInfo["listen-address"]
-	if flag.Define {
-		cmd.Flag(flag.Name, flag.Help).
-			Envar(flag.Envar).
-			Default(ListenAddress).
-			StringVar(&ListenAddress)
-	}
+	cmd.Flag(flag.Name, flag.Help).
+		Envar(flag.Envar).
+		Default(ListenAddress).
+		StringVar(&ListenAddress)
 
 	flag = CommonFlagsInfo["listen-port"]
-	if flag.Define {
-		cmd.Flag(flag.Name, flag.Help).
-			Envar(flag.Envar).
-			Default(ListenPort).
-			StringVar(&ListenPort)
-	}
+	cmd.Flag(flag.Name, flag.Help).
+		Envar(flag.Envar).
+		Default(ListenPort).
+		StringVar(&ListenPort)
 
 	flag = CommonFlagsInfo["prometheus-metrics-prefix"]
-	if flag.Define {
-		cmd.Flag(flag.Name, flag.Help).
-			Envar(flag.Envar).
-			Default(PrometheusMetricsPrefix).
-			StringVar(&PrometheusMetricsPrefix)
-	}
+	cmd.Flag(flag.Name, flag.Help).
+		Envar(flag.Envar).
+		Default(PrometheusMetricsPrefix).
+		StringVar(&PrometheusMetricsPrefix)
 
 	flag = CommonFlagsInfo["namespace"]
-	if flag.Define {
-		cmd.Flag(flag.Name, flag.Help).
-			Envar(flag.Envar).
-			Default(Namespace).
-			StringVar(&Namespace)
-	}
+	cmd.Flag(flag.Name, flag.Help).
+		Envar(flag.Envar).
+		Default(Namespace).
+		StringVar(&Namespace)
 
 	DefineKubeClientFlags(cmd)
 	DefineValidatingWebhookFlags(cmd)

pkg/hook/hook.go

@@ -29,10 +29,6 @@ const (
 	serviceName = "hook"
 )
 
-type CommonHook interface {
-	Name() string
-}
-
 type Result struct {
 	Usage                *executor.CmdUsage
 	Metrics              []operation.MetricOperation

pkg/hook/hook_discovery_test.go

@@ -0,0 +1,143 @@
+package hook
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestFileSystemHookDiscovery_Discover_emptyDir returns no paths for an empty directory.
+func TestFileSystemHookDiscovery_Discover_emptyDir(t *testing.T) {
+	dir := t.TempDir()
+	d := FileSystemHookDiscovery{}
+	paths, err := d.Discover(dir)
+	require.NoError(t, err)
+	assert.Empty(t, paths)
+}
+
+// TestFileSystemHookDiscovery_Discover_findsExecutables returns executable files.
+func TestFileSystemHookDiscovery_Discover_findsExecutables(t *testing.T) {
+	dir := t.TempDir()
+
+	// Create an executable file.
+	execPath := filepath.Join(dir, "my-hook.sh")
+	err := os.WriteFile(execPath, []byte("#!/bin/sh\necho ok\n"), 0o755)
+	require.NoError(t, err)
+
+	d := FileSystemHookDiscovery{}
+	paths, err := d.Discover(dir)
+	require.NoError(t, err)
+	assert.Contains(t, paths, execPath)
+}
+
+// TestFileSystemHookDiscovery_Discover_ignoresNonExecutables skips regular files.
+func TestFileSystemHookDiscovery_Discover_ignoresNonExecutables(t *testing.T) {
+	dir := t.TempDir()
+
+	// Non-executable file.
+	nonExecPath := filepath.Join(dir, "README.md")
+	err := os.WriteFile(nonExecPath, []byte("docs"), 0o644)
+	require.NoError(t, err)
+
+	d := FileSystemHookDiscovery{}
+	paths, err := d.Discover(dir)
+	require.NoError(t, err)
+	assert.NotContains(t, paths, nonExecPath)
+}
+
+// TestFileSystemHookDiscovery_Discover_recursive finds executables in subdirectories.
+func TestFileSystemHookDiscovery_Discover_recursive(t *testing.T) {
+	dir := t.TempDir()
+	subdir := filepath.Join(dir, "subdir")
+	require.NoError(t, os.MkdirAll(subdir, 0o755))
+
+	hookPath := filepath.Join(subdir, "hook.sh")
+	require.NoError(t, os.WriteFile(hookPath, []byte("#!/bin/sh\n"), 0o755))
+
+	d := FileSystemHookDiscovery{}
+	paths, err := d.Discover(dir)
+	require.NoError(t, err)
+	assert.Contains(t, paths, hookPath)
+}
+
+// TestNewHookManager_defaultDiscovery verifies that a nil HookDiscovery in
+// ManagerConfig defaults to FileSystemHookDiscovery inside the Manager.
+func TestNewHookManager_defaultDiscovery(t *testing.T) {
+	hm := newHookManager(t, t.TempDir())
+	hm.hookDiscovery = nil // reset to exercise the nil-default path via NewHookManager
+	cfg := &ManagerConfig{
+		WorkingDir:               t.TempDir(),
+		TempDir:                  t.TempDir(),
+		HookDiscovery:            nil, // explicitly nil → should default
+		AdmissionWebhookManager:  hm.admissionWebhookManager,
+		ConversionWebhookManager: hm.conversionWebhookManager,
+		Logger:                   hm.logger,
+	}
+	hm2 := NewHookManager(cfg)
+	require.NotNil(t, hm2)
+	assert.IsType(t, FileSystemHookDiscovery{}, hm2.hookDiscovery)
+}
+
+// TestNewHookManager_injectedDiscovery verifies that a custom HookDiscovery is
+// stored as-is in the Manager.
+func TestNewHookManager_injectedDiscovery(t *testing.T) {
+	stub := &stubDiscovery{}
+	hm := newHookManager(t, t.TempDir())
+	cfg := &ManagerConfig{
+		WorkingDir:               t.TempDir(),
+		TempDir:                  t.TempDir(),
+		HookDiscovery:            stub,
+		AdmissionWebhookManager:  hm.admissionWebhookManager,
+		ConversionWebhookManager: hm.conversionWebhookManager,
+		Logger:                   hm.logger,
+	}
+	hm2 := NewHookManager(cfg)
+	assert.Equal(t, stub, hm2.hookDiscovery)
+}
+
+// TestManager_Init_usesInjectedDiscovery verifies that Manager.Init calls
+// HookDiscovery.Discover rather than the filesystem when a custom discovery is
+// injected.  An empty result means Init succeeds with zero hooks loaded.
+func TestManager_Init_usesInjectedDiscovery(t *testing.T) {
+	stub := &stubDiscovery{paths: []string{}} // returns nothing
+	hm := newHookManager(t, t.TempDir())
+	hm.hookDiscovery = stub
+
+	err := hm.Init()
+	require.NoError(t, err)
+	assert.True(t, stub.called, "Discover should have been called")
+	assert.Equal(t, []string{}, hm.GetHookNames())
+}
+
+// TestManager_Init_discoveryError propagates discovery errors.
+func TestManager_Init_discoveryError(t *testing.T) {
+	stub := &stubDiscovery{err: errStubDiscovery}
+	hm := newHookManager(t, t.TempDir())
+	hm.hookDiscovery = stub
+
+	err := hm.Init()
+	require.Error(t, err)
+	assert.ErrorIs(t, err, errStubDiscovery)
+}
+
+// ---- helpers ----
+
+var errStubDiscovery = &testError{"stub discovery error"}
+
+type testError struct{ msg string }
+
+func (e *testError) Error() string { return e.msg }
+
+type stubDiscovery struct {
+	paths  []string
+	err    error
+	called bool
+}
+
+func (s *stubDiscovery) Discover(_ string) ([]string, error) {
+	s.called = true
+	return s.paths, s.err
+}

pkg/hook/hook_manager.go

@@ -36,6 +36,10 @@ type Manager struct {
 	conversionWebhookManager *conversion.WebhookManager
 	admissionWebhookManager  *admission.WebhookManager
 
+	// hookDiscovery resolves the set of hook executables to load.
+	// Defaults to FileSystemHookDiscovery; tests may inject a stub.
+	hookDiscovery HookDiscovery
+
 	// hook execution options
 	keepTemporaryHookFiles bool
 	logProxyHookJSON       bool
@@ -64,6 +68,10 @@ type ManagerConfig struct {
 	AdmissionWebhookManager  *admission.WebhookManager
 	ConversionWebhookManager *conversion.WebhookManager
 
+	// HookDiscovery overrides the default filesystem-based hook discovery.
+	// When nil, FileSystemHookDiscovery is used.
+	HookDiscovery HookDiscovery
+
 	KeepTemporaryHookFiles bool
 	LogProxyHookJSON       bool
 	LogProxyHookJSONKey    string
@@ -72,6 +80,10 @@ type ManagerConfig struct {
 }
 
 func NewHookManager(config *ManagerConfig) *Manager {
+	disc := config.HookDiscovery
+	if disc == nil {
+		disc = FileSystemHookDiscovery{}
+	}
 	return &Manager{
 		hooksByName:      make(map[string]*Hook),
 		hookNamesInOrder: make([]string, 0),
@@ -84,6 +96,7 @@ func NewHookManager(config *ManagerConfig) *Manager {
 		scheduleManager:          config.ScheduleManager,
 		admissionWebhookManager:  config.AdmissionWebhookManager,
 		conversionWebhookManager: config.ConversionWebhookManager,
+		hookDiscovery:            disc,
 
 		keepTemporaryHookFiles: config.KeepTemporaryHookFiles,
 		logProxyHookJSON:       config.LogProxyHookJSON,
@@ -114,16 +127,16 @@ func (hm *Manager) Init() error {
 			log.Err(err))
 	}
 
-	hooksRelativePaths, err := utils_file.RecursiveGetExecutablePaths(hm.workingDir)
+	hookPaths, err := hm.hookDiscovery.Discover(hm.workingDir)
 	if err != nil {
 		return err
 	}
 
 	// sort hooks by path
-	sort.Strings(hooksRelativePaths)
-	hm.logger.Debug("Search hooks in paths", slog.Any(pkg.LogKeyPaths, hooksRelativePaths))
+	sort.Strings(hookPaths)
+	hm.logger.Debug("Search hooks in paths", slog.Any(pkg.LogKeyPaths, hookPaths))
 
-	for _, hookPath := range hooksRelativePaths {
+	for _, hookPath := range hookPaths {
 		hook, err := hm.loadHook(hookPath)
 		if err != nil {
 			return err
@@ -449,3 +462,37 @@ func (hm *Manager) UpdateConversionChains() error {
 func (hm *Manager) FindConversionChain(crdName string, rule conversion.Rule) []conversion.Rule {
 	return hm.conversionChains.FindConversionChain(crdName, rule)
 }
+
+// HookManager is the interface for the hook manager used by the operator.
+// It allows substituting test doubles in unit tests.
+type HookManager interface {
+	Init() error
+	GetHook(name string) *Hook
+	GetHookNames() []string
+	GetHooksInOrder(bindingType htypes.BindingType) ([]string, error)
+	CreateTasksFromKubeEvent(kubeEvent kemtypes.KubeEvent, createTaskFn func(*Hook, controller.BindingExecutionInfo) task.Task) []task.Task
+	HandleCreateTasksFromScheduleEvent(crontab string, createTaskFn func(*Hook, controller.BindingExecutionInfo) task.Task) []task.Task
+	HandleAdmissionEvent(ctx context.Context, event admission.Event, createTaskFn func(*Hook, controller.BindingExecutionInfo))
+	DetectAdmissionEventType(event admission.Event) htypes.BindingType
+	HandleConversionEvent(ctx context.Context, crdName string, request *v1.ConversionRequest, rule conversion.Rule, createTaskFn func(*Hook, controller.BindingExecutionInfo))
+	FindConversionChain(crdName string, rule conversion.Rule) []conversion.Rule
+}
+
+// HookDiscovery discovers hook executables to be loaded by the Manager.
+// The default implementation scans the filesystem; tests and alternative
+// runtimes can supply their own.
+type HookDiscovery interface {
+	// Discover returns a sorted list of absolute paths to hook executables.
+	Discover(workingDir string) ([]string, error)
+}
+
+// FileSystemHookDiscovery discovers hooks by recursively scanning workingDir
+// for executable files.
+type FileSystemHookDiscovery struct{}
+
+func (FileSystemHookDiscovery) Discover(workingDir string) ([]string, error) {
+	return utils_file.RecursiveGetExecutablePaths(workingDir)
+}
+
+// compile-time assertion
+var _ HookManager = (*Manager)(nil)