refactor(cli): migrate commands to RunE with a single exit funnel by Caesarsage · Pull Request #490 · microcks/microcks-cli

Caesarsage · 2026-07-09T14:13:02Z

depend on #489

Part of the Microcks CLI v2 work (#255). Stacked on PR #489.

All ten commands move from Run + scattered os.Exit/log.Fatal to RunE returning errors.
A single funnel — cmd.Handle, called only by main — prints to stderr and maps Failure Kind → exit code (cmd/exit.go). Nothing else exits.
A non-conforming contract test travels as the silent ErrTestFailed sentinel → exit 1 with no spurious error line (mirrors kubectl diff).

No impact on the microcks binary: commands, flags, args, and success output are unchanged. Only code importing these packages is affected.

Exit codes (new, user-visible)

0 = success / non-zero = failure still holds, and 1 still means "contract test failed". Operational failures now carry distinct codes so CI can branch on why:

Code	Meaning
0	success / contract conformed
1	contract test failed (a clean run)
2	usage — bad arguments or flags
11	connection — couldn't reach Microcks/Keycloak
12	API — server rejected the request / unusable response
13	not-found — remote resource missing
14	environment — local precondition (container runtime, image, readiness)
20	generic — unclassified failure

These codes were previously undocumented and inconsistent (1/2/20 interchangeably), so there is no prior contract to break. Documented in README.md + documentation/error-handling.md.

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

Caesarsage · 2026-07-09T14:17:11Z

HOLD

till PR #489 is review and merged

Vaishnav88sk · 2026-07-22T09:28:27Z

Stacked PR of #489

Vaishnav88sk

Please resolve the conflicts.

Also add copyright block for pkg/errors/error_test.go

Vaishnav88sk · 2026-07-27T06:50:12Z

 	watcher, err := fsnotify.NewWatcher()
 	if err != nil {
-		fmt.Printf("Failed to create file watcher: %s\n", err)
-		return false
+		return fmt.Errorf("failed to create file watcher: %w", err)
 	}
 	defer watcher.Close()

 	// Watch the directory, not the file: editors replace files on save
 	// (rename + create), which silently drops a watch set on the file itself.
 	artifactPath, err := filepath.Abs(opts.artifact)
 	if err != nil {
-		fmt.Printf("Failed to resolve artifact path: %s\n", err)
-		return false
+		return fmt.Errorf("failed to resolve artifact path: %w", err)
 	}
 	if err := watcher.Add(filepath.Dir(artifactPath)); err != nil {
-		fmt.Printf("Failed to watch %s: %s\n", filepath.Dir(artifactPath), err)
-		return false
+		return fmt.Errorf("failed to watch %s: %w", filepath.Dir(artifactPath), err)
 	}


These new error paths are still unclassified fmt.Errorf(...) returns.

Since this PR is introducing a kind-based exit funnel, these should be wrapped consistently as well (likely KindEnvironment for watcher/runtime setup problems, or KindUsage where appropriate). Otherwise these cases still collapse to the generic 20 path.

Vaishnav88sk · 2026-07-27T06:51:37Z

 			if len(args) == 0 {
-				fmt.Println("import command require <specificationFile1[:primary],specificationFile2[:primary]> args")
-				cmd.HelpFunc()(cmd, args)
-				os.Exit(1)
+				return errors.Wrapf(errors.KindUsage, "import requires a <specificationFile1[:primary],specificationFile2[:primary]> argument")
 			}


One behavioral change to call out here: this no longer prints help/usage, whereas the previous implementation did.

I’m fine with the RunE refactor, but if the goal is “no impact on the binary”, we should either preserve the old help/usage behavior for bad invocations or explicitly document/test that this user-facing behavior is changing.

Thanks for catching this. I will make a fix for this by creating am helper usage function

Vaishnav88sk · 2026-07-27T06:52:42Z

 			if len(args) != 1 {
-				cmd.HelpFunc()(cmd, args)
-				os.Exit(1)
+				return errors.Wrapf(errors.KindUsage, "login requires exactly one SERVER argument")
 			}


Same comment here: this changes the UX on invalid invocation from “show help + exit” to “print one error line + exit”.

That may be acceptable, but it is user-visible behavior and contradicts the current PR description of “no impact on the binary”. Please either preserve the old behavior or call out the change explicitly.

Vaishnav88sk · 2026-07-27T06:53:20Z

@@ -197,6 +191,7 @@ func NewImportDirCommand(globalClientOpts *connectors.ClientOptions) *cobra.Comm
 			}

 			fmt.Printf("\nImport completed: %d/%d files imported successfully\n", result.SuccessCount, result.TotalFiles)


Please double-check this path against the intended import-dir semantics.

As written, partial failures still return nil, so the single exit funnel will still exit 0 even when some files failed. If we want import-dir to be CI-friendly, this likely needs to return a non-zero error when FailedCount > 0.

Vaishnav88sk · 2026-07-27T07:02:54Z

 	u, err := url.Parse(realmURL)
 	if err != nil {
-		panic(err)
+		return nil, errors.Wrap(errors.KindUsage, fmt.Errorf("invalid Keycloak URL %q: %w", realmURL, err))
 	}


I flagged in #489 and you said was fixed here. Please point to the current line number and confirm it now returns errors.Wrap(errors.KindUsage or KindAPI, err) instead of panic(err).

this is returning the errors helper as i mentioned on #489. I am not sure I understand the comment here

Vaishnav88sk · 2026-07-27T07:04:05Z

 	u, err := url.Parse(apiURL)
 	if err != nil {
-		panic(err)
+		return nil, errors.Wrap(errors.KindUsage, fmt.Errorf("invalid server URL %q: %w", apiURL, err))
 	}
 	mc.APIURL = u



Same as keycloak_client.go, confirm the url.Parse panic here is replaced with a returned classified error, and share the line number.

this is returning the errors helper as i mentioned on #489 and all are documented on the documentation folder. I am not sure I understand the comment here

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

Caesarsage added 4 commits July 9, 2026 14:55

feat(errors): add Failure Kind classification

3829a79

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

fix(connectors): return classified errors instead of panicking

179ecbc

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

refactor(cmd): route commands through RunE and a single exit funnel

9b38b1f

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

docs: document exit codes and error-handling model

4675819

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

Caesarsage requested review from Harsh4902, lbroudoux and yada as code owners July 9, 2026 14:13

Caesarsage temporarily deployed to Build July 9, 2026 14:13 — with GitHub Actions Inactive

Caesarsage mentioned this pull request Jul 9, 2026

refactor(cli): remove the legacy exit shim and enforce it in CI #491

Open

This was referenced Jul 22, 2026

feat(test): add --output (text/json/yaml/github-actions) for machine-readable results #488

Open

fix(cli): classify failures and partial de-panic in the client library #489

Merged

Vaishnav88sk suggested changes Jul 27, 2026

View reviewed changes

Address review feedback

830a40a

Signed-off-by: caesarsage <destinyerhabor6@gmail.com>

Caesarsage temporarily deployed to Build July 27, 2026 10:40 — with GitHub Actions Inactive

		@@ -197,6 +191,7 @@ func NewImportDirCommand(globalClientOpts connectors.ClientOptions) cobra.Comm
		}

		fmt.Printf("\nImport completed: %d/%d files imported successfully\n", result.SuccessCount, result.TotalFiles)

Uh oh!

Uh oh!

Conversation

Caesarsage commented Jul 9, 2026

Exit codes (new, user-visible)

Uh oh!

Caesarsage commented Jul 9, 2026

Uh oh!

Vaishnav88sk commented Jul 22, 2026

Uh oh!

Vaishnav88sk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants