refactor: address review feedback on registry and command patterns

- Make Register methods atomic (validate-then-insert) in check, action,
  and dependency registries to avoid partial state on error
- Change CommandOption to return error instead of panicking
- Remove dead IsSet() guard in formatAndOutputResults (lint mode only)
- Collapse duplicate if/else branches in backup command
- Extract newDefaultActionRegistry() helper to DRY up migrate constructors
- Update stale Command Structure docs in architecture.md

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: lburgazzoli

docs/lint/architecture.md

@@ -344,15 +344,27 @@ The lint command uses a `Command` struct (not `Options`) with constructor `NewCo
 
 ```go
 type Command struct {
-    shared        *SharedOptions
-    targetVersion string
+    *SharedOptions
+    TargetVersion version.SemVersion  // implements pflag.Value
+    registry      *check.CheckRegistry
 }
 
-func NewCommand(opts CommandOptions) *Command {
-    return &Command{
-        shared:        opts.Shared,
-        targetVersion: opts.TargetVersion,
+func NewCommand(
+    streams genericiooptions.IOStreams,
+    configFlags *genericclioptions.ConfigFlags,
+    options ...CommandOption,
+) (*Command, error) {
+    registry, err := check.NewRegistry(/* checks... */)
+    if err != nil {
+        return nil, fmt.Errorf("registering checks: %w", err)
+    }
+
+    c := &Command{
+        SharedOptions: NewSharedOptions(streams, configFlags),
+        registry:      registry,
     }
+    // ... apply options ...
+    return c, nil
 }
 ```
 

pkg/backup/command.go

@@ -96,25 +96,18 @@ func (c *Command) Complete() error {
 		}
 	}
 
+	var resolvers []dependencies.Resolver
 	if c.Dependencies {
-		registry, err := dependencies.NewRegistry(
-			notebooks.NewResolver(),
-			dspa.NewResolver(),
-		)
-		if err != nil {
-			return fmt.Errorf("registering dependency resolvers: %w", err)
-		}
-
-		c.depRegistry = registry
-	} else {
-		registry, err := dependencies.NewRegistry()
-		if err != nil {
-			return fmt.Errorf("creating dependency registry: %w", err)
-		}
+		resolvers = append(resolvers, notebooks.NewResolver(), dspa.NewResolver())
+	}
 
-		c.depRegistry = registry
+	registry, err := dependencies.NewRegistry(resolvers...)
+	if err != nil {
+		return fmt.Errorf("creating dependency registry: %w", err)
 	}
 
+	c.depRegistry = registry
+
 	return nil
 }
 

pkg/backup/dependencies/registry.go

@@ -27,16 +27,17 @@ func NewRegistry(resolvers ...Resolver) (*Registry, error) {
 }
 
 // Register adds one or more resolvers to the registry.
+// The operation is atomic: either all resolvers are registered or none are.
 // Returns an error if any resolver is nil.
 func (r *Registry) Register(resolvers ...Resolver) error {
 	for _, resolver := range resolvers {
 		if resolver == nil {
 			return errors.New("cannot register nil resolver")
 		}
-
-		r.resolvers = append(r.resolvers, resolver)
 	}
 
+	r.resolvers = append(r.resolvers, resolvers...)
+
 	return nil
 }
 

pkg/lint/check/registry.go

@@ -26,6 +26,7 @@ func NewRegistry(checks ...Check) (*CheckRegistry, error) {
 }
 
 // Register adds one or more checks to the registry.
+// The operation is atomic: either all checks are registered or none are.
 // Returns an error if a check with the same ID already exists.
 func (r *CheckRegistry) Register(checks ...Check) error {
 	r.mu.Lock()
@@ -35,7 +36,9 @@ func (r *CheckRegistry) Register(checks ...Check) error {
 		if _, exists := r.checks[c.ID()]; exists {
 			return fmt.Errorf("check with ID %s already registered", c.ID())
 		}
+	}
 
+	for _, c := range checks {
 		r.checks[c.ID()] = c
 	}
 

pkg/lint/command.go

@@ -111,7 +111,9 @@ func NewCommand(
 	}
 
 	for _, opt := range options {
-		opt(c)
+		if err := opt(c); err != nil {
+			return nil, fmt.Errorf("applying command option: %w", err)
+		}
 	}
 
 	return c, nil
@@ -405,13 +407,7 @@ func (c *Command) formatAndOutputResults(
 	resultsByGroup map[check.CheckGroup][]check.CheckExecution,
 ) error {
 	clusterVer := &c.currentClusterVersion
-
-	var targetVer *string
-	if c.TargetVersion.IsSet() {
-		s := c.TargetVersion.String()
-		targetVer = &s
-	}
-
+	var targetVer *string // always nil in lint mode
 	ocpVer := c.openShiftVersionPtr()
 
 	// Flatten results to sorted array

pkg/lint/command_options.go

@@ -210,18 +210,19 @@ func ValidateCheckSelector(selector string) error {
 //
 // Example:
 //
-//	cmd := lint.NewCommand(streams, configFlags,
+//	cmd, err := lint.NewCommand(streams, configFlags,
 //	    lint.WithTargetVersion("3.0"),
 //	)
-type CommandOption func(*Command)
+type CommandOption func(*Command) error
 
 // WithTargetVersion returns a CommandOption that sets the target version.
-// Panics if version is not a valid semver string (programmatic API).
 func WithTargetVersion(v string) CommandOption {
-	return func(c *Command) {
+	return func(c *Command) error {
 		if err := c.TargetVersion.Set(v); err != nil {
-			panic(fmt.Sprintf("WithTargetVersion: %v", err))
+			return fmt.Errorf("setting target version: %w", err)
 		}
+
+		return nil
 	}
 }
 

pkg/migrate/action/registry.go

@@ -24,17 +24,21 @@ func NewActionRegistry(actions ...Action) (*ActionRegistry, error) {
 	return r, nil
 }
 
+// Register adds one or more actions to the registry.
+// The operation is atomic: either all actions are registered or none are.
+// Returns an error if an action with the same ID already exists.
 func (r *ActionRegistry) Register(actions ...Action) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
 	for _, a := range actions {
-		id := a.ID()
-		if _, exists := r.actions[id]; exists {
-			return fmt.Errorf("action with ID %q already registered", id)
+		if _, exists := r.actions[a.ID()]; exists {
+			return fmt.Errorf("action with ID %q already registered", a.ID())
 		}
+	}
 
-		r.actions[id] = a
+	for _, a := range actions {
+		r.actions[a.ID()] = a
 	}
 
 	return nil

pkg/migrate/command_list.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/opendatahub-io/odh-cli/pkg/cmd"
 	"github.com/opendatahub-io/odh-cli/pkg/migrate/action"
-	"github.com/opendatahub-io/odh-cli/pkg/migrate/actions/kueue/rhbok"
 	"github.com/opendatahub-io/odh-cli/pkg/printer/table"
 	"github.com/opendatahub-io/odh-cli/pkg/util/iostreams"
 	"github.com/opendatahub-io/odh-cli/pkg/util/version"
@@ -43,17 +42,13 @@ type ListCommand struct {
 }
 
 func NewListCommand(streams genericiooptions.IOStreams) (*ListCommand, error) {
-	shared := NewSharedOptions(streams)
-
-	registry, err := action.NewActionRegistry(
-		&rhbok.RHBOKMigrationAction{},
-	)
+	registry, err := newDefaultActionRegistry()
 	if err != nil {
 		return nil, fmt.Errorf("registering actions: %w", err)
 	}
 
 	return &ListCommand{
-		SharedOptions: shared,
+		SharedOptions: NewSharedOptions(streams),
 		registry:      registry,
 	}, nil
 }

pkg/migrate/command_options.go

@@ -8,6 +8,8 @@ import (
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/cli-runtime/pkg/genericiooptions"
 
+	"github.com/opendatahub-io/odh-cli/pkg/migrate/action"
+	"github.com/opendatahub-io/odh-cli/pkg/migrate/actions/kueue/rhbok"
 	"github.com/opendatahub-io/odh-cli/pkg/util/client"
 	"github.com/opendatahub-io/odh-cli/pkg/util/iostreams"
 )
@@ -84,3 +86,11 @@ func (o *SharedOptions) Validate() error {
 
 	return nil
 }
+
+// newDefaultActionRegistry creates an action registry with all known migration actions.
+// Centralizes the action list so additions are made in one place.
+func newDefaultActionRegistry() (*action.ActionRegistry, error) {
+	return action.NewActionRegistry(
+		&rhbok.RHBOKMigrationAction{},
+	)
+}

pkg/migrate/command_prepare.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/opendatahub-io/odh-cli/pkg/cmd"
 	"github.com/opendatahub-io/odh-cli/pkg/migrate/action"
-	"github.com/opendatahub-io/odh-cli/pkg/migrate/actions/kueue/rhbok"
 	"github.com/opendatahub-io/odh-cli/pkg/util/version"
 )
 
@@ -37,17 +36,13 @@ type PrepareCommand struct {
 }
 
 func NewPrepareCommand(streams genericiooptions.IOStreams) (*PrepareCommand, error) {
-	shared := NewSharedOptions(streams)
-
-	registry, err := action.NewActionRegistry(
-		&rhbok.RHBOKMigrationAction{},
-	)
+	registry, err := newDefaultActionRegistry()
 	if err != nil {
 		return nil, fmt.Errorf("registering actions: %w", err)
 	}
 
 	return &PrepareCommand{
-		SharedOptions: shared,
+		SharedOptions: NewSharedOptions(streams),
 		registry:      registry,
 	}, nil
 }