Merge pull request #75 from warptools/cli-refactor

Refactoring CLI wiring

.github/workflows/actions.yml

@@ -29,7 +29,7 @@ jobs:
  working-directory: go-serum-analyzer
  run: go install ./cmd/go-serum-analyzer/
  - name: Run Serum Analyzer
- run: go-serum-analyzer -strict ./...
+ run: go-serum-analyzer ./...
  test:
  runs-on: ubuntu-latest
  steps:

app/HACKME.md

@@ -0,0 +1,92 @@
+HACKME for the "cmd/warpforge/app" packages
+===========================================
+
+Why this package?
+-----------------
+
+### Why any package at all?
+
+The CLI args object needs to be accessable by other packages.
+
+- if it's stuck in one package and unexported, it's much harder to write large-scale testing.
+- if it's stuck in one package and unexported, it's much harder to write other features like docs generation that uses it.
+
+### Why not just put this in the `main` package?
+
+We can't use the `main` package and just export it the CLI args object,
+because `main` packages cannot be imported in golang, due to rules enforced by the compiler.
+
+### Why so many sub-packages?
+
+One subpackage per top-level subcommand.
+
+(This is a lot of subpackages, but helps development in two ways:
+for the human, it makes it easy to related groups of commands together when developing, without having to test "everything";
+and for the compiler and test harness, it allows package-level parallelism.)
+
+There's also:
+
+- one sub-package for `base` (which everything else imports).
+ - This contains the root CLI object. (It's a global. Other packages shovel their wiring into it when loaded.)
+ - This also also contains global config stuff. Environment vars, etc.
+- one sub-package for `testutil` (which everything else imports in their tests),
+ - This is mostly just for quickly wiring up testmark harnesses (because every subcommand writes tests in a similar fashion).
+- and this package itself, which brings everything together (the real CLI, many tests, and the docs generation tools import this, because they all want to see everything at once).
+ - There's not much here other than the imports that tie everything together.
+
+
+
+Why are package-scoped vars used?
+---------------------------------
+
+Fair question. Generally, that _is_ a bad code smell.
+
+### The `urfave/cli` library forced our hand.
+
+There's some pieces of customization in the library we use for handling the command line that are already package-scope (aka global) vars,
+up in _that_ package.
+
+Unfortunate though this is, it's kind of a dam-busting scenario.
+It's hard to see a point in refraining from just doubling-down on package-scope vars once we're already stuck with more than none of them.
+
+### Doesn't this bone test parallelization?
+
+It totally does. For tests that use the full CLI, anyway.
+
+But it's not all bad news.
+We still have package-level parallelization,
+since golang compiles a separate binary and runs a separate process for each package's tests.
+So in practice, when running `go test ./...` over the whole repo, that means things are still pretty fine.
+(The use of sub-packages per command group helps a lot here, too.)
+
+
+
+How much "business logic" is supposed to be in here?
+----------------------------------------------------
+
+Drawing the line is hard.
+
+Here's a couple rules-of-thumb that usually help decide where code goes:
+
+- If it refers to any `cli` packages or variables: **it goes in here**.
+- If one of these packages would call into one of its neighbors: that's bad; **shared code should be factored out to `pkg/*`**.
+- If the above two are contradictory: refactor until you have functions that *can* be out in `pkg/*` and are *not* refering to `cli` wiring directly.
+
+If you *can* move some code out into `pkg/*` without too much fuss... it's probably a good target-of-opportunity.
+But don't add complexity for no reason.
+If that code has zero reuse, maybe it doesn't justify the creation of another package.
+
+Not all existing code is a good example of the goalposts.
+
+
+
+TODOs
+-----
+
+### Add serum annotations on every 'action' function.
+
+Serum analysis in the code that's the very nearest to the user's eyeballs is the most important place!
+Every one of the "action" functions that are wired into the CLI should be annotated for analysis.
+
+A more aggressive variation of this might also be to create an interface type and use it to declare _all_ error codes valid to see returned from the CLI,
+and then make all the "action" functions conform to that interface, so the analyzer makes sure each "action" is returning a subset of the total.

app/app.go

@@ -0,0 +1,18 @@
+package wfapp
+
+import (
+ appbase "github.com/warptools/warpforge/app/base"
+ _ "github.com/warptools/warpforge/app/catalog"
+ _ "github.com/warptools/warpforge/app/check"
+ _ "github.com/warptools/warpforge/app/enter"
+ _ "github.com/warptools/warpforge/app/healthcheck"
+ _ "github.com/warptools/warpforge/app/plan"
+ _ "github.com/warptools/warpforge/app/quickstart"
+ _ "github.com/warptools/warpforge/app/run"
+ _ "github.com/warptools/warpforge/app/spark"
+ _ "github.com/warptools/warpforge/app/status"
+ _ "github.com/warptools/warpforge/app/ware"
+ _ "github.com/warptools/warpforge/app/watch"
+)
+
+var App = appbase.App

app/app_test.go

@@ -0,0 +1,15 @@
+package wfapp_test
+
+import (
+ "testing"
+
+ "github.com/warptools/warpforge/app/testutil"
+)
+
+func TestExampleDirCLI(t *testing.T) {
+ testutil.TestFileContainingTestmarkexec(t, "../examples/500-cli/cli.md", nil)
+}
+
+func TestExampleDirCLIHelp(t *testing.T) {
+ testutil.TestFileContainingTestmarkexec(t, "../examples/500-cli/help.md", nil)
+}

app/base/appbase.go

@@ -0,0 +1,103 @@
+package appbase
+
+import (
+ "encoding/json"
+ "fmt"
+ "io"
+
+ "github.com/urfave/cli/v2"
+)
+
+const VERSION = "v0.4.0"
+
+var App = &cli.App{
+ Name: "warpforge",
+ Version: VERSION,
+ Usage: "the everything-builder and any-environment manager",
+
+ Reader: closedReader{}, // Replace with os.Stdin in real application; or other wiring, in tests.
+ Writer: panicWriter{}, // Replace with os.Stdout in real application; or other wiring, in tests.
+ ErrWriter: panicWriter{}, // Replace with os.Stderr in real application; or other wiring, in tests.
+
+ Flags: []cli.Flag{
+ &cli.BoolFlag{
+ Name: "verbose",
+ Aliases: []string{"v"},
+ EnvVars: []string{"WARPFORGE_DEBUG"},
+ },
+ &cli.BoolFlag{
+ Name: "quiet",
+ },
+ &cli.BoolFlag{
+ Name: "json",
+ Usage: "Enable JSON API output",
+ },
+ &cli.StringFlag{
+ Name: "trace.file",
+ Usage: "Enable tracing and emit output to file",
+ TakesFile: true,
+ },
+ &cli.BoolFlag{
+ Name: "trace.grpc.enable",
+ Usage: "Enable remote tracing",
+ Hidden: true, // not implemented yet
+ },
+ &cli.StringFlag{
+ Name: "trace.grpc.endpoint",
+ Usage: "Sets an endpoint for remote open-telemetry tracing collection",
+ Hidden: true, // not implemented yet
+ },
+ &cli.BoolFlag{
+ Name: "trace.http.enable",
+ Usage: "Enable remote tracing over http",
+ },
+ &cli.BoolFlag{
+ Name: "trace.http.insecure",
+ Usage: "Allows insecure http",
+ },
+ &cli.StringFlag{
+ Name: "trace.http.endpoint",
+ Usage: "Sets an endpoint for remote open-telemetry tracing collection",
+ },
+ },
+
+ // The commands slice is updated by each package that contains commands.
+ // Import the parent of this package to get that all done for you!
+ Commands: []*cli.Command{},
+
+ ExitErrHandler: func(c *cli.Context, err error) {
+ if err == nil {
+ return
+ }
+ if c.Bool("json") {
+ bytes, err := json.Marshal(err)
+ if err != nil {
+ panic("error marshaling json")
+ }
+ fmt.Fprintf(c.App.ErrWriter, "%s\n", string(bytes))
+ } else {
+ fmt.Fprintf(c.App.ErrWriter, "error: %s\n", err)
+ }
+ },
+}
+
+// Aaaand the other modifications to `urfave/cli` that are unfortunately only possible by manipulating globals:
+func init() {
+ cli.VersionFlag = &cli.BoolFlag{
+ Name: "version", // And no short aliases. "-v" is for "verbose"!
+ }
+}
+
+type closedReader struct{}
+
+// Read is a dummy method that always returns EOF.
+func (c closedReader) Read(p []byte) (int, error) {
+ return 0, io.EOF
+}
+
+type panicWriter struct{}
+
+// Write is a dummy method that always panics. You're supposed to replace panicWriter values before use.
+func (p panicWriter) Write(data []byte) (int, error) {
+ panic("replace the Writer and ErrWriter on the App value in packages that use it!")
+}

cmd/warpforge/catalog.go → app/catalog/catalog_cli.go

@@ -1,4 +1,4 @@
-package main
+package catalogcli
 
 import (
  "bytes"
@@ -17,8 +17,8 @@ import (
  "github.com/urfave/cli/v2"
  "go.opentelemetry.io/otel/trace"
 
- "github.com/warptools/warpforge/cmd/warpforge/internal/catalog"
- "github.com/warptools/warpforge/cmd/warpforge/internal/util"
+ appbase "github.com/warptools/warpforge/app/base"
+ "github.com/warptools/warpforge/app/base/util"
  "github.com/warptools/warpforge/pkg/cataloghtml"
  "github.com/warptools/warpforge/pkg/config"
  "github.com/warptools/warpforge/pkg/dab"
@@ -29,7 +29,11 @@ import (
  "github.com/warptools/warpforge/wfapi"
 )
 
-var catalogCmdDef = cli.Command{
+func init() {
+ appbase.App.Commands = append(appbase.App.Commands, catalogCmdDef)
+}
+
+var catalogCmdDef = &cli.Command{
  Name: "catalog",
  Usage: "Subcommands that operate on catalogs",
  Flags: []cli.Flag{
@@ -405,7 +409,7 @@ func cmdCatalogUpdate(c *cli.Context) error {
  }
  }
 
- if err = catalog.InstallDefaultRemoteCatalog(c.Context, catalogPath); err != nil {
+ if err = InstallDefaultRemoteCatalog(c.Context, catalogPath); err != nil {
  return fmt.Errorf("failed to install default catalog: %s", err)
  }
 
@@ -454,6 +458,46 @@ func cmdCatalogUpdate(c *cli.Context) error {
  return nil
 }
 
+const defaultCatalogUrl = "https://github.com/warptools/warpsys-catalog.git"
+
+// InstallDefaultRemoteCatalog creates the default catalog by cloning a remote catalog over network.
+// This function will do nothing if the default catalog already exists.
+//
+// Errors:
+//
+// - warpforge-error-git -- Cloning catalog fails
+// - warpforge-error-io -- catalog path exists but is in a strange state
+func InstallDefaultRemoteCatalog(ctx context.Context, path string) error {
+ log := logging.Ctx(ctx)
+ // install our default remote catalog as "default-remote" by cloning from git
+ // this will noop if the catalog already exists
+ defaultCatalogPath := filepath.Join(path, "warpsys")
+ _, err := os.Stat(defaultCatalogPath)
+ if !os.IsNotExist(err) {
+ if err == nil {
+ // a dir exists for this catalog, do nothing
+ return nil
+ }
+ return wfapi.ErrorIo("unknown error with catalog path", defaultCatalogPath, err)
+ }
+
+ log.Info("", "installing default catalog to %s...", defaultCatalogPath)
+
+ gitCtx, gitSpan := tracing.Start(ctx, "clone catalog", trace.WithAttributes(tracing.AttrFullExecNameGit, tracing.AttrFullExecOperationGitClone))
+ defer gitSpan.End()
+ _, err = git.PlainCloneContext(gitCtx, defaultCatalogPath, false, &git.CloneOptions{
+ URL: defaultCatalogUrl,
+ })
+ tracing.EndWithStatus(gitSpan, err)
+
+ log.Info("", "installing default catalog complete")
+
+ if err != nil {
+ return wfapi.ErrorGit("Unable to git clone catalog", err)
+ }
+ return nil
+}
+
 func cmdCatalogRelease(c *cli.Context) error {
  ctx := c.Context
  var err error

cmd/warpforge/check.go → app/check/check_cli.go

@@ -1,4 +1,4 @@
-package main
+package checkcli
 
 import (
  "context"
@@ -13,13 +13,18 @@ import (
  "github.com/serum-errors/go-serum"
  "github.com/urfave/cli/v2"
 
- "github.com/warptools/warpforge/cmd/warpforge/internal/util"
+ appbase "github.com/warptools/warpforge/app/base"
+ "github.com/warptools/warpforge/app/base/util"
  "github.com/warptools/warpforge/pkg/dab"
  "github.com/warptools/warpforge/pkg/plotexec"
  "github.com/warptools/warpforge/wfapi"
 )
 
-var checkCmdDef = cli.Command{
+func init() {
+ appbase.App.Commands = append(appbase.App.Commands, checkCmdDef)
+}
+
+var checkCmdDef = &cli.Command{
  Name: "check",
  Usage: "Check file(s) for syntax and sanity",
  Action: util.ChainCmdMiddleware(cmdCheck,
@@ -142,7 +147,11 @@ func cmdCheck(c *cli.Context) error {
  continue
  }
  if c.Bool("verbose") && n != nil {
- c.App.Metadata["result"] = *n
+ serial, err := ipld.Encode(*n, json.Encode)
+ if err != nil {
+ panic("failed to serialize output")
+ }
+ fmt.Fprintf(c.App.Writer, "%s\n", serial)
  }
  }
 

cmd/warpforge/ferk.go → app/enter/ferk_cli.go

@@ -1,4 +1,4 @@
-package main
+package entercli
 
 import (
  "fmt"
@@ -12,7 +12,8 @@ import (
  "github.com/serum-errors/go-serum"
  "github.com/urfave/cli/v2"
 
- "github.com/warptools/warpforge/cmd/warpforge/internal/util"
+ appbase "github.com/warptools/warpforge/app/base"
+ "github.com/warptools/warpforge/app/base/util"
  "github.com/warptools/warpforge/pkg/config"
  "github.com/warptools/warpforge/pkg/dab"
  "github.com/warptools/warpforge/pkg/logging"
@@ -21,7 +22,11 @@ import (
  "github.com/warptools/warpforge/wfapi"
 )
 
-var ferkCmdDef = cli.Command{
+func init() {
+ appbase.App.Commands = append(appbase.App.Commands, ferkCmdDef)
+}
+
+var ferkCmdDef = &cli.Command{
  Name: "ferk",
  Usage: "Starts a containerized environment for interactive use",
  Action: util.ChainCmdMiddleware(cmdFerk,