Skip to content

Commit 1e1ebdb

Browse files
refactor: return errors from registry constructors and introduce SemVersion pflag type
Registry constructors (check.NewRegistry, dependencies.NewRegistry, action.NewRegistry) now return (*Registry, error) instead of panicking via MustRegister, propagating registration errors to callers. Replace the TargetVersion string + parsedTargetVersion *semver.Version pair in the lint Command with a single version.SemVersion type that implements pflag.Value, validating at flag-parse time. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent b7b4ad5 commit 1e1ebdb

27 files changed

+483
-358
lines changed

cmd/lint/lint.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,18 +74,21 @@ func AddCommand(root *cobra.Command, flags *genericclioptions.ConfigFlags) {
7474
ErrOut: root.ErrOrStderr(),
7575
}
7676

77-
// Create command with ConfigFlags from parent to ensure CLI auth flags are used
78-
command := lintpkg.NewCommand(streams, flags)
77+
command, err := lintpkg.NewCommand(streams, flags)
78+
if err != nil {
79+
_, _ = fmt.Fprintf(root.ErrOrStderr(), "Error initializing lint command: %v\n", err)
80+
81+
return
82+
}
7983

8084
cmd := &cobra.Command{
8185
Use: cmdName,
8286
Short: cmdShort,
8387
Long: cmdLong,
8488
Example: cmdExample,
8589
SilenceUsage: true,
86-
SilenceErrors: true, // We'll handle error output manually based on --quiet flag
90+
SilenceErrors: true,
8791
RunE: func(cmd *cobra.Command, _ []string) error {
88-
// Complete phase
8992
if err := command.Complete(); err != nil {
9093
if command.Verbose {
9194
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Error: %v\n", err)
@@ -94,7 +97,6 @@ func AddCommand(root *cobra.Command, flags *genericclioptions.ConfigFlags) {
9497
return fmt.Errorf("completing command: %w", err)
9598
}
9699

97-
// Validate phase
98100
if err := command.Validate(); err != nil {
99101
if command.Verbose {
100102
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Error: %v\n", err)
@@ -103,7 +105,6 @@ func AddCommand(root *cobra.Command, flags *genericclioptions.ConfigFlags) {
103105
return fmt.Errorf("validating command: %w", err)
104106
}
105107

106-
// Run phase
107108
err := command.Run(cmd.Context())
108109
if err != nil {
109110
if command.Verbose {
@@ -117,7 +118,6 @@ func AddCommand(root *cobra.Command, flags *genericclioptions.ConfigFlags) {
117118
},
118119
}
119120

120-
// Register flags using AddFlags method
121121
command.AddFlags(cmd.Flags())
122122

123123
root.AddCommand(cmd)

cmd/migrate/list/list.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package list
22

33
import (
4+
"fmt"
5+
46
"github.com/spf13/cobra"
57

68
"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -41,7 +43,13 @@ func AddCommand(
4143
flags *genericclioptions.ConfigFlags,
4244
streams genericiooptions.IOStreams,
4345
) {
44-
command := migrate.NewListCommand(streams)
46+
command, err := migrate.NewListCommand(streams)
47+
if err != nil {
48+
_, _ = fmt.Fprintf(parent.ErrOrStderr(), "Error initializing list command: %v\n", err)
49+
50+
return
51+
}
52+
4553
command.ConfigFlags = flags
4654

4755
cmd := &cobra.Command{

cmd/migrate/prepare/prepare.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package prepare
22

33
import (
4+
"fmt"
5+
46
"github.com/spf13/cobra"
57

68
"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -51,7 +53,13 @@ func AddCommand(
5153
flags *genericclioptions.ConfigFlags,
5254
streams genericiooptions.IOStreams,
5355
) {
54-
command := migrate.NewPrepareCommand(streams)
56+
command, err := migrate.NewPrepareCommand(streams)
57+
if err != nil {
58+
_, _ = fmt.Fprintf(parent.ErrOrStderr(), "Error initializing prepare command: %v\n", err)
59+
60+
return
61+
}
62+
5563
command.ConfigFlags = flags
5664

5765
cmd := &cobra.Command{

cmd/migrate/run/run.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package run
22

33
import (
4+
"fmt"
5+
46
"github.com/spf13/cobra"
57

68
"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -48,7 +50,13 @@ func AddCommand(
4850
flags *genericclioptions.ConfigFlags,
4951
streams genericiooptions.IOStreams,
5052
) {
51-
command := migrate.NewRunCommand(streams)
53+
command, err := migrate.NewRunCommand(streams)
54+
if err != nil {
55+
_, _ = fmt.Fprintf(parent.ErrOrStderr(), "Error initializing run command: %v\n", err)
56+
57+
return
58+
}
59+
5260
command.ConfigFlags = flags
5361

5462
cmd := &cobra.Command{

docs/lint/architecture.md

Lines changed: 34 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -82,45 +82,44 @@ func NewCommand(
8282
streams genericiooptions.IOStreams,
8383
configFlags *genericclioptions.ConfigFlags,
8484
options ...CommandOption,
85-
) *Command {
86-
registry := check.NewRegistry()
87-
88-
// Explicitly register all checks (no global state, full test isolation)
89-
// Components (11)
90-
registry.MustRegister(codeflare.NewRemovalCheck())
91-
registry.MustRegister(dashboard.NewAcceleratorProfileMigrationCheck())
92-
registry.MustRegister(dashboard.NewHardwareProfileMigrationCheck())
93-
registry.MustRegister(datasciencepipelines.NewInstructLabRemovalCheck())
94-
registry.MustRegister(datasciencepipelines.NewRenamingCheck())
95-
registry.MustRegister(kserve.NewServerlessRemovalCheck())
96-
registry.MustRegister(kserve.NewInferenceServiceConfigCheck())
97-
registry.MustRegister(kueue.NewManagementStateCheck())
98-
registry.MustRegister(kueue.NewOperatorInstalledCheck())
99-
registry.MustRegister(modelmesh.NewRemovalCheck())
100-
registry.MustRegister(trainingoperator.NewDeprecationCheck())
101-
102-
// Dependencies (3)
103-
registry.MustRegister(certmanager.NewCheck())
104-
registry.MustRegister(openshift.NewCheck())
105-
registry.MustRegister(servicemeshoperator.NewCheck())
106-
107-
// Services (1)
108-
registry.MustRegister(servicemesh.NewRemovalCheck())
109-
110-
// Workloads (8)
111-
registry.MustRegister(guardrails.NewImpactedWorkloadsCheck())
112-
registry.MustRegister(guardrails.NewOtelMigrationCheck())
113-
registry.MustRegister(kserveworkloads.NewAcceleratorMigrationCheck())
114-
registry.MustRegister(kserveworkloads.NewImpactedWorkloadsCheck())
115-
registry.MustRegister(notebook.NewAcceleratorMigrationCheck())
116-
registry.MustRegister(notebook.NewImpactedWorkloadsCheck())
117-
registry.MustRegister(ray.NewImpactedWorkloadsCheck())
118-
registry.MustRegister(trainingoperatorworkloads.NewImpactedWorkloadsCheck())
85+
) (*Command, error) {
86+
registry, err := check.NewRegistry(
87+
// Components (11)
88+
codeflare.NewRemovalCheck(),
89+
dashboard.NewAcceleratorProfileMigrationCheck(),
90+
dashboard.NewHardwareProfileMigrationCheck(),
91+
datasciencepipelines.NewInstructLabRemovalCheck(),
92+
datasciencepipelines.NewRenamingCheck(),
93+
kserve.NewServerlessRemovalCheck(),
94+
kserve.NewInferenceServiceConfigCheck(),
95+
kueue.NewManagementStateCheck(),
96+
kueue.NewOperatorInstalledCheck(),
97+
modelmesh.NewRemovalCheck(),
98+
trainingoperator.NewDeprecationCheck(),
99+
// Dependencies (3)
100+
certmanager.NewCheck(),
101+
openshift.NewCheck(),
102+
servicemeshoperator.NewCheck(),
103+
// Services (1)
104+
servicemesh.NewRemovalCheck(),
105+
// Workloads (8)
106+
guardrails.NewImpactedWorkloadsCheck(),
107+
guardrails.NewOtelMigrationCheck(),
108+
kserveworkloads.NewAcceleratorMigrationCheck(),
109+
kserveworkloads.NewImpactedWorkloadsCheck(),
110+
notebook.NewAcceleratorMigrationCheck(),
111+
notebook.NewImpactedWorkloadsCheck(),
112+
ray.NewImpactedWorkloadsCheck(),
113+
trainingoperatorworkloads.NewImpactedWorkloadsCheck(),
114+
)
115+
if err != nil {
116+
return nil, fmt.Errorf("registering checks: %w", err)
117+
}
119118

120119
return &Command{
121120
SharedOptions: shared,
122121
registry: registry,
123-
}
122+
}, nil
124123
}
125124
```
126125

docs/lint/writing-checks.md

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,14 @@ func (c *Check) Validate(ctx context.Context, target check.Target) (*result.Diag
7979
}
8080
```
8181

82-
**Registration:** Checks are explicitly registered in `pkg/lint/command.go`:
82+
**Registration:** Checks are explicitly registered in `pkg/lint/command.go` via `check.NewRegistry(...)`:
8383

8484
```go
8585
// In NewCommand()
86-
registry.MustRegister(dashboard.NewCheck())
86+
registry, err := check.NewRegistry(
87+
dashboard.NewCheck(),
88+
// ... other checks
89+
)
8790
```
8891

8992
### Using BaseCheck
@@ -129,34 +132,33 @@ func NewCommand(
129132
streams genericiooptions.IOStreams,
130133
configFlags *genericclioptions.ConfigFlags,
131134
options ...CommandOption,
132-
) *Command {
133-
registry := check.NewRegistry()
134-
135-
// Explicitly register all checks
136-
// Components (11)
137-
registry.MustRegister(codeflare.NewRemovalCheck())
138-
registry.MustRegister(dashboard.NewAcceleratorProfileMigrationCheck())
139-
registry.MustRegister(dashboard.NewHardwareProfileMigrationCheck())
140-
registry.MustRegister(datasciencepipelines.NewInstructLabRemovalCheck())
141-
// ... additional component checks
142-
143-
// Dependencies (4)
144-
registry.MustRegister(certmanager.NewCheck())
145-
// ... additional dependency checks
146-
147-
// Services (1)
148-
registry.MustRegister(servicemesh.NewRemovalCheck())
149-
150-
// Workloads (7)
151-
registry.MustRegister(guardrails.NewOtelMigrationCheck())
152-
registry.MustRegister(kserveworkloads.NewAcceleratorMigrationCheck())
153-
registry.MustRegister(notebook.NewImpactedWorkloadsCheck())
154-
// ... additional workload checks
135+
) (*Command, error) {
136+
registry, err := check.NewRegistry(
137+
// Components (11)
138+
codeflare.NewRemovalCheck(),
139+
dashboard.NewAcceleratorProfileMigrationCheck(),
140+
dashboard.NewHardwareProfileMigrationCheck(),
141+
datasciencepipelines.NewInstructLabRemovalCheck(),
142+
// ... additional component checks
143+
// Dependencies (4)
144+
certmanager.NewCheck(),
145+
// ... additional dependency checks
146+
// Services (1)
147+
servicemesh.NewRemovalCheck(),
148+
// Workloads (7)
149+
guardrails.NewOtelMigrationCheck(),
150+
kserveworkloads.NewAcceleratorMigrationCheck(),
151+
notebook.NewImpactedWorkloadsCheck(),
152+
// ... additional workload checks
153+
)
154+
if err != nil {
155+
return nil, fmt.Errorf("registering checks: %w", err)
156+
}
155157

156158
return &Command{
157159
SharedOptions: shared,
158160
registry: registry,
159-
}
161+
}, nil
160162
}
161163
```
162164

@@ -852,8 +854,8 @@ func (c *RemovalCheck) Validate(ctx context.Context, target check.Target) (*resu
852854
Run(ctx, validate.Removal("CodeFlare is enabled (state: %s) but will be removed in RHOAI 3.x"))
853855
}
854856

855-
// Registration is done explicitly in pkg/lint/command.go:
856-
// registry.MustRegister(codeflare.NewRemovalCheck())
857+
// Registration is done explicitly in pkg/lint/command.go via check.NewRegistry(...)
858+
// e.g. codeflare.NewRemovalCheck() is passed to check.NewRegistry()
857859
```
858860

859861
**Note:** For complex checks that don't fit the builder pattern, you can still write checks without builders using `c.NewResult()` and manually fetching resources via `client.GetDataScienceCluster(ctx, target.Client)`.

docs/migrate/implementation-plan.md

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ type ActionRegistry struct {
6767
```
6868
Pattern matching via `ListByPattern()`, similar to CheckRegistry
6969

70-
**File:** `pkg/migrate/action/global.go`
71-
Global registry with `MustRegisterAction()` for auto-registration
70+
**File:** `pkg/migrate/action/registry.go`
71+
Variadic `NewActionRegistry(actions ...Action) (*ActionRegistry, error)` constructor with error-returning `Register(actions ...Action) error`
7272

7373
#### 1.3 Action Executor
7474
**File:** `pkg/migrate/action/executor.go`
@@ -389,16 +389,11 @@ if step1.Status == result.StepFailed {
389389
}
390390
```
391391

392-
### Auto-Registration Pattern
392+
### Explicit Registration Pattern
393393
```go
394-
// In rhbok.go
395-
func init() {
396-
action.MustRegisterAction(&RHBOKMigrationAction{})
397-
}
398-
399-
// In cmd/migrate/migrate.go
400-
import (
401-
_ "github.com/opendatahub-io/odh-cli/pkg/migrate/actions/kueue/rhbok"
394+
// Actions are registered explicitly in each command constructor:
395+
registry, err := action.NewActionRegistry(
396+
&rhbok.RHBOKMigrationAction{},
402397
)
403398
```
404399

pkg/backup/command.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -96,13 +96,23 @@ func (c *Command) Complete() error {
9696
}
9797
}
9898

99-
// Create registry - always needed even if empty
100-
c.depRegistry = dependencies.NewRegistry()
101-
102-
// Only register resolvers if dependency resolution is enabled
10399
if c.Dependencies {
104-
c.depRegistry.MustRegister(notebooks.NewResolver())
105-
c.depRegistry.MustRegister(dspa.NewResolver())
100+
registry, err := dependencies.NewRegistry(
101+
notebooks.NewResolver(),
102+
dspa.NewResolver(),
103+
)
104+
if err != nil {
105+
return fmt.Errorf("registering dependency resolvers: %w", err)
106+
}
107+
108+
c.depRegistry = registry
109+
} else {
110+
registry, err := dependencies.NewRegistry()
111+
if err != nil {
112+
return fmt.Errorf("creating dependency registry: %w", err)
113+
}
114+
115+
c.depRegistry = registry
106116
}
107117

108118
return nil

0 commit comments

Comments
 (0)