From e150a2f03df17c302c6fc394c80a711fdf8f5b7f Mon Sep 17 00:00:00 2001 From: Ryan Schumacher Date: Mon, 23 Feb 2026 21:27:01 -0500 Subject: [PATCH 1/2] =?UTF-8?q?refactor:=20complete=20FlagHelper=E2=86=92F?= =?UTF-8?q?lags=20migration=20and=20remove=20deprecated=20config=20cmd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace all 20 remaining `.FlagHelper` usages with `.Flags` across 6 files, completing the migration started in d6932f30 - Remove the `FlagHelper` field alias from `Cli` struct - Remove deprecated `config` command and docs (functionality migrated to `profile` in PR #719) - Add `docs/ARCHITECTURE.md` documenting current and intended architecture with alignment assessment Co-Authored-By: Claude Opus 4.6 --- cmd/auth/login.go | 6 +- cmd/auth/logout.go | 2 +- cmd/common/common.go | 12 +- cmd/config/config.go | 28 --- cmd/policy/attributeValues.go | 10 +- cmd/policy/kasRegistry.go | 2 +- cmd/profile.go | 8 +- cmd/root.go | 4 - docs/ARCHITECTURE.md | 363 ++++++++++++++++++++++++++++++++++ docs/man/config/_index.md | 10 - docs/man/config/output.md | 15 -- pkg/cli/cli.go | 7 +- 12 files changed, 385 insertions(+), 82 deletions(-) delete mode 100644 cmd/config/config.go create mode 100644 docs/ARCHITECTURE.md delete mode 100644 docs/man/config/_index.md delete mode 100644 docs/man/config/output.md diff --git a/cmd/auth/login.go b/cmd/auth/login.go index ae26ab88..7a17d668 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -14,13 +14,13 @@ import ( func codeLogin(cmd *cobra.Command, args []string) { c := cli.New(cmd, args) cp := common.InitProfile(c) - clientID := c.FlagHelper.GetRequiredString("client-id") - port := c.FlagHelper.GetOptionalString("port") + clientID := c.Flags.GetRequiredString("client-id") + port := c.Flags.GetOptionalString("port") tok, err := auth.LoginWithPKCE( cmd.Context(), cp.GetEndpoint(), clientID, - c.FlagHelper.GetOptionalBool("tls-no-verify"), + c.Flags.GetOptionalBool("tls-no-verify"), port, ) if err != nil { diff --git a/cmd/auth/logout.go b/cmd/auth/logout.go index 2d143c6a..3a633422 100644 --- a/cmd/auth/logout.go +++ b/cmd/auth/logout.go @@ -23,7 +23,7 @@ func logout(cmd *cobra.Command, args []string) { cp.GetEndpoint(), creds.AccessToken.ClientID, creds.AccessToken.RefreshToken, - c.FlagHelper.GetOptionalBool("tls-no-verify"), + c.Flags.GetOptionalBool("tls-no-verify"), ); err != nil { c.ExitWithError("An error occurred while revoking the access token", err) } diff --git a/cmd/common/common.go b/cmd/common/common.go index 5bdd315a..00232cab 100644 --- a/cmd/common/common.go +++ b/cmd/common/common.go @@ -39,7 +39,7 @@ func applyOutputFormatPreference(c *cli.Cli, store *profiles.OtdfctlProfileStore // returns the profile and the current profile store func InitProfile(c *cli.Cli) *profiles.OtdfctlProfileStore { var err error - profileName := c.FlagHelper.GetOptionalString("profile") + profileName := c.Flags.GetOptionalString("profile") hasKeyringStore, err := osprofiles.HasGlobalStore(config.AppName, osprofiles.WithKeyringStore()) if err != nil { @@ -89,11 +89,11 @@ func NewHandler(c *cli.Cli) handlers.Handler { var cp *profiles.OtdfctlProfileStore // Non-profile flags - host := c.FlagHelper.GetOptionalString("host") - tlsNoVerify := c.FlagHelper.GetOptionalBool("tls-no-verify") - withClientCreds := c.FlagHelper.GetOptionalString("with-client-creds") - withClientCredsFile := c.FlagHelper.GetOptionalString("with-client-creds-file") - withAccessToken := c.FlagHelper.GetOptionalString("with-access-token") + host := c.Flags.GetOptionalString("host") + tlsNoVerify := c.Flags.GetOptionalBool("tls-no-verify") + withClientCreds := c.Flags.GetOptionalString("with-client-creds") + withClientCredsFile := c.Flags.GetOptionalString("with-client-creds-file") + withAccessToken := c.Flags.GetOptionalString("with-access-token") var inMemoryProfile bool authFlags := []string{"--with-access-token", "--with-client-creds", "--with-client-creds-file"} diff --git a/cmd/config/config.go b/cmd/config/config.go deleted file mode 100644 index 09871e25..00000000 --- a/cmd/config/config.go +++ /dev/null @@ -1,28 +0,0 @@ -package config - -import ( - "github.com/opentdf/otdfctl/pkg/man" -) - -var ( - outputDoc = man.Docs.GetCommand("config/output") - configDoc = man.Docs.GetCommand("config", man.WithSubcommands(outputDoc)) - Cmd = &configDoc.Command -) - -const ( - cfgDeprecationNotice = "use profile commands" - cfgOutputDeprecationMsg = "use profile set-output-format instead" -) - -func InitCommands() { - // Mark the entire config command as deprecated so users migrate to profiles. - Cmd.Deprecated = cfgDeprecationNotice - outputDoc.Deprecated = cfgOutputDeprecationMsg - - outputDoc.Flags().String( - outputDoc.GetDocFlag("format").Name, - outputDoc.GetDocFlag("format").Default, - outputDoc.GetDocFlag("format").Description, - ) -} diff --git a/cmd/policy/attributeValues.go b/cmd/policy/attributeValues.go index 1afeb5be..250920c0 100644 --- a/cmd/policy/attributeValues.go +++ b/cmd/policy/attributeValues.go @@ -19,9 +19,9 @@ func createAttributeValue(cmd *cobra.Command, args []string) { defer h.Close() ctx := cmd.Context() - attrID := c.FlagHelper.GetRequiredID("attribute-id") - value := c.FlagHelper.GetRequiredString("value") - metadataLabels = c.FlagHelper.GetStringSlice("label", metadataLabels, cli.FlagsStringSliceOptions{Min: 0}) + attrID := c.Flags.GetRequiredID("attribute-id") + value := c.Flags.GetRequiredString("value") + metadataLabels = c.Flags.GetStringSlice("label", metadataLabels, cli.FlagsStringSliceOptions{Min: 0}) attr, err := h.GetAttribute(ctx, attrID) if err != nil { @@ -41,7 +41,7 @@ func getAttributeValue(cmd *cobra.Command, args []string) { h := common.NewHandler(c) defer h.Close() - id := c.FlagHelper.GetRequiredID("id") + id := c.Flags.GetRequiredID("id") v, err := h.GetAttributeValue(cmd.Context(), id) if err != nil { @@ -56,7 +56,7 @@ func listAttributeValue(cmd *cobra.Command, args []string) { h := common.NewHandler(c) defer h.Close() - attrID := c.FlagHelper.GetRequiredID("attribute-id") + attrID := c.Flags.GetRequiredID("attribute-id") state := cli.GetState(cmd) limit := c.Flags.GetRequiredInt32("limit") offset := c.Flags.GetRequiredInt32("offset") diff --git a/cmd/policy/kasRegistry.go b/cmd/policy/kasRegistry.go index 1dbd99eb..2aa0d759 100644 --- a/cmd/policy/kasRegistry.go +++ b/cmd/policy/kasRegistry.go @@ -21,7 +21,7 @@ func getKeyAccessRegistry(cmd *cobra.Command, args []string) { h := common.NewHandler(c) defer h.Close() - id := c.FlagHelper.GetRequiredID("id") + id := c.Flags.GetRequiredID("id") kas, err := h.GetKasRegistryEntry(cmd.Context(), handlers.KasIdentifier{ ID: id, diff --git a/cmd/profile.go b/cmd/profile.go index 19422b99..cf9cc421 100644 --- a/cmd/profile.go +++ b/cmd/profile.go @@ -38,7 +38,7 @@ func newProfilerFromCLI(c *cli.Cli) *osprofiles.Profiler { func getDriverTypeFromUser(c *cli.Cli) profiles.ProfileDriver { driverTypeStr := string(profiles.ProfileDriverDefault) - store := c.FlagHelper.GetOptionalString("store") + store := c.Flags.GetOptionalString("store") if len(store) > 0 { driverTypeStr = store } @@ -69,9 +69,9 @@ var profileCreateCmd = &cobra.Command{ profileName := args[0] endpoint := args[1] - setDefault := c.FlagHelper.GetOptionalBool("set-default") - tlsNoVerify := c.FlagHelper.GetOptionalBool("tls-no-verify") - outputFormat := c.FlagHelper.GetOptionalString("output-format") + setDefault := c.Flags.GetOptionalBool("set-default") + tlsNoVerify := c.Flags.GetOptionalBool("tls-no-verify") + outputFormat := c.Flags.GetOptionalString("output-format") if !profiles.IsValidOutputFormat(outputFormat) { c.ExitWithError("Output format must be either 'styled' or 'json'", nil) } diff --git a/cmd/root.go b/cmd/root.go index 50101426..885aff38 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -6,7 +6,6 @@ import ( "os" "github.com/opentdf/otdfctl/cmd/auth" - cfg "github.com/opentdf/otdfctl/cmd/config" "github.com/opentdf/otdfctl/cmd/dev" "github.com/opentdf/otdfctl/cmd/policy" "github.com/opentdf/otdfctl/cmd/tdf" @@ -84,8 +83,6 @@ func init() { } RootCmd.AddCommand( - // config - cfg.Cmd, // tdf tdf.EncryptCmd, tdf.DecryptCmd, @@ -159,7 +156,6 @@ func init() { RootCmd.AddGroup(&cobra.Group{ID: tdf.GroupID}) // Initialize all subcommands that have been refactored to use explicit initialization - cfg.InitCommands() auth.InitCommands() policy.InitCommands() dev.InitCommands() diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md new file mode 100644 index 00000000..5c3d79b0 --- /dev/null +++ b/docs/ARCHITECTURE.md @@ -0,0 +1,363 @@ +# otdfctl Architecture + +This document describes the architecture of `otdfctl`, the CLI for managing the OpenTDF Platform. It is split into two sections: **As-Is** (current state) and **To-Be** (intended direction). The goal is to enable alignment assessment between what exists in the code and what the architecture intends. + +--- + +## As-Is: Current Architecture + +### Overview + +otdfctl is a Go CLI application built on [Cobra](https://cobra.dev/) that provides CRUD operations for the OpenTDF Platform's policy objects, TDF encrypt/decrypt, and profile-based configuration. It communicates with the platform via gRPC through the `opentdf/platform/sdk`. + +**Module**: `github.com/opentdf/otdfctl` +**Go version**: 1.25.0 +**Current version**: 0.29.0 (managed by release-please) + +### Layered Architecture + +The codebase follows a three-layer architecture: + +``` +┌─────────────────────────────────────────────────────┐ +│ cmd/ Commands (Cobra) │ +│ ├── policy/ Policy CRUD subcommands │ +│ ├── tdf/ Encrypt/decrypt/inspect │ +│ ├── auth/ Login/logout/credentials │ +│ ├── config/ (deprecated) │ +│ ├── dev/ Dev/playground commands │ +│ ├── common/ Shared command helpers │ +│ ├── root.go Root command, init, wiring │ +│ ├── profile.go Profile management commands │ +│ └── interactive.go TUI launcher │ +├─────────────────────────────────────────────────────┤ +│ pkg/ Core Library │ +│ ├── cli/ CLI framework (Cli, Printer, │ +│ │ flags, tables, styles, confirm) │ +│ ├── handlers/ SDK wrapper (Handler struct, │ +│ │ CRUD methods per resource) │ +│ ├── man/ Doc-driven command system │ +│ ├── profiles/ Profile management (stores) │ +│ ├── auth/ OIDC auth flows │ +│ ├── config/ Build-time constants │ +│ ├── tdf/ TDF type constants │ +│ └── utils/ URL, PEM, file, HTTP helpers │ +├─────────────────────────────────────────────────────┤ +│ External │ +│ ├── opentdf/platform/sdk gRPC SDK │ +│ ├── opentdf/platform/protocol Protobuf types │ +│ └── opentdf/platform/lib Crypto, flattening │ +└─────────────────────────────────────────────────────┘ +``` + +**Data flow for a typical command**: +``` +User → Cobra command → cli.New() → common.NewHandler() → handlers.Method() → SDK → Platform gRPC + ↓ + Profile store (auth, endpoint) + ↓ + common.HandleSuccess() → Printer (styled | JSON) → stdout +``` + +### Documentation-Driven Command System + +The most distinctive architectural pattern. Commands are defined in Markdown files with YAML frontmatter, not in Go code. + +**Location**: `docs/man/` (100+ markdown files) +**Mechanism**: `pkg/man/` parses embedded markdown at `init()` time via `go:embed` + +Each markdown file defines: +- `command.name`: The cobra `Use` field +- `command.arguments`: Positional args +- `command.aliases`: Command aliases +- `command.flags`: Flag definitions (name, description, shorthand, default, enum) +- `command.hidden`: Whether the command is hidden +- Markdown body: Becomes the `Long` description (rendered via glamour) + +**Usage pattern in Go code**: +```go +// Retrieve a doc-defined command and attach a run function +doc := man.Docs.GetCommand("policy/attributes/create", man.WithRun(createAttribute)) +// Retrieve flag definitions from the doc +doc.Flags().StringP(doc.GetDocFlag("name").Name, ...) +``` + +**Supports i18n**: Files can be suffixed with language codes (e.g., `create.fr.md`), with English as the default fallback. + +### Command Initialization Pattern + +All command groups follow an `InitCommands()` pattern: + +1. `cmd/root.go:init()` adds top-level commands to `RootCmd` +2. `cmd/root.go:init()` calls `InitCommands()` on each command group +3. Each `InitCommands()` calls per-resource `init*Commands()` functions +4. `init*Commands()` wires doc-based commands, flags, and run functions + +This explicit initialization (rather than Go `init()`) gives control over ordering and avoids circular dependencies. + +### Handler Pattern (Service Layer) + +`pkg/handlers/Handler` wraps the platform SDK: + +```go +type Handler struct { + sdk *sdk.SDK + platformEndpoint string +} +``` + +Every command that talks to the platform follows this flow: +1. `c := cli.New(cmd, args)` — create CLI context with flag helpers and printer +2. `h := common.NewHandler(c)` — load profile, authenticate, create SDK connection +3. `defer h.Close()` — clean up the SDK connection +4. Call a handler method (e.g., `h.CreateAttribute(...)`) +5. Format output and exit via `common.HandleSuccess()` or `cli.ExitWith*()` + +**`common.NewHandler()`** orchestrates auth: +- If `--host` + auth flags are set → creates an in-memory profile (no filesystem) +- Otherwise → loads from the profile store (filesystem) +- Validates credentials before returning + +**Handler files** (18 files in `pkg/handlers/`): +Each file corresponds to a platform resource type (attributes, namespaces, kas-registry, etc.) and wraps the SDK's gRPC client methods. + +### CLI Framework (`pkg/cli`) + +The `Cli` struct is created at the start of every command handler: + +```go +type Cli struct { + cmd *cobra.Command + args []string + Flags *flagHelper // typed flag accessors + printer *Printer // dual-mode output +} +``` + +Key components: +- **`flagHelper`** (`flagValues.go`): Typed flag accessors (`GetRequiredString`, `GetRequiredID`, `GetOptionalBool`, `GetStringSlice`, `GetState`, etc.) +- **`Printer`** (`printer.go`): Dual-mode output controlled by `--json` flag or profile `outputFormat` setting +- **`errors.go`**: Exit functions (`ExitWithError`, `ExitWithNotFoundError`, `ExitWithWarning`, `ExitWithSuccess`) — all call `os.Exit()` +- **`table.go`**: Table rendering via `evertras/bubble-table` +- **`tabular.go`**: Key-value display for single-resource views +- **`style.go`**: Adaptive color palette and lipgloss styles +- **`confirm.go`**: Interactive confirmation prompts via `charmbracelet/huh` +- **`sdkHelpers.go`**: SDK data transformers (`GetSimpleAttribute`, `ConstructMetadata`, key algorithm converters) +- **`pipe.go`**: Stdin/file reading utilities + +**Note**: `Cli.FlagHelper` and `Cli.Flags` are the same object (aliased), with `FlagHelper` being the deprecated name. + +### Dual Output Mode + +Every command supports two output formats (per ADR-0001): + +| Mode | Trigger | Behavior | +|------|---------|----------| +| **Styled** | Default, or profile `outputFormat: styled` | lipgloss-colored terminal output with bubble-table rendering | +| **JSON** | `--json` flag, or profile `outputFormat: json` | Structured JSON to stdout/stderr | + +The `common.HandleSuccess()` function checks both the `--json` flag and the profile output format to decide which mode to use. + +### Profile System + +Replaces traditional config files with a profile-based approach: + +**Package**: `pkg/profiles/` (wraps `jrschumacher/go-osprofiles`) + +**Profile drivers**: +| Driver | Usage | +|--------|-------| +| Filesystem | Default. Stores in OS app config directories | +| Keyring | OS keyring (deprecated, auto-migrates to filesystem) | +| In-memory | For flag-based ephemeral usage (`--host` + auth flags) | + +**Profile contents** (`ProfileConfig`): +- `Name`, `Endpoint`, `TLSNoVerify`, `OutputFormat` +- `AuthCredentials`: type (client-credentials or access-token), client ID/secret, scopes, access token + +**Migration**: `common.InitProfile()` auto-migrates from keyring to filesystem on every invocation. + +### Authentication + +**Package**: `pkg/auth/` + +Three auth mechanisms: +1. **Client credentials** — stored in profile or via `--with-client-creds` / `--with-client-creds-file` flags +2. **Access token** — stored in profile or via `--with-access-token` flag +3. **PKCE login** — interactive browser-based flow via `auth login` + +Auth is resolved in `common.NewHandler()` and validated via `auth.ValidateProfileAuthCredentials()` before SDK initialization. + +### Error Handling + +Commands use `cli.ExitWith*()` functions which: +1. Print a styled or JSON error message (respecting output format) +2. Call `os.Exit()` with an appropriate exit code + +gRPC `NotFound` status codes get special treatment via `ExitWithNotFoundError`. Sentinel errors are defined per package in `errors.go` files. + +Destructive operations use `cli.ConfirmAction()` / `cli.ConfirmTextInput()` unless `--force` is passed. + +### Command Structure + +``` +otdfctl +├── auth +│ ├── login +│ ├── logout +│ ├── client-credentials +│ ├── clear-client-credentials +│ └── print-access-token +├── profile (aliases: profiles, prof) +│ ├── create, list, get, delete, delete-all +│ ├── set-default, set-endpoint, set-output-format +│ ├── migrate, cleanup +├── encrypt, decrypt, inspect (TDF group) +├── policy +│ ├── actions [get, list, create, update, delete] +│ ├── attributes [get, list, create, update, deactivate] +│ │ ├── values [get, list, create, update, deactivate] +│ │ │ ├── key [assign, remove] +│ │ │ └── unsafe [delete, reactivate, update] +│ │ ├── key [assign, remove] +│ │ ├── unsafe [delete, reactivate, update] +│ │ └── namespaces [get, list, create, update, deactivate] +│ │ ├── key [assign, remove] +│ │ └── unsafe [delete, reactivate, update] +│ ├── kas-registry [get, list, create, update, delete] +│ │ └── key [get, list, create, update, rotate, import, list-mappings] +│ │ ├── base [get, set] +│ │ └── unsafe [delete] +│ ├── kas-grants [assign, unassign, list] +│ ├── key-management +│ │ └── provider [get, list, create, update, delete] +│ ├── subject-condition-sets [get, list, create, update, delete] +│ ├── subject-mappings [get, list, create, update, delete] +│ ├── obligations [get, list, create, update, delete] +│ ├── resource-mappings [get, list, create, update, delete] +│ ├── resource-mapping-groups [get, list, create, update, delete] +│ └── registered-resources [get, list, create, update, delete] +├── config (deprecated → use profile) +│ └── output +├── dev +│ ├── design-system +│ └── selectors [generate, test] +└── interactive (launches Bubble Tea TUI) +``` + +### TUI (`tui/`) + +A Bubble Tea-based interactive mode. Launched via `otdfctl interactive`. + +**Status**: Work in progress (README warns against modifying) + +Contains views for: app menu, attribute list/detail/create, label list/update, read, update. Uses `tui/constants/` for shared state and `tui/form/` for form components. + +### Build System + +**Makefile targets**: +| Target | Description | +|--------|-------------| +| `run` | `go run .` | +| `test` | `go test -v ./...` | +| `build` | Cross-compile for 8 targets (darwin/linux/windows x amd64/arm/arm64), zip, checksum | +| `build-test` | Build with `TestMode=true` for BATS e2e testing | +| `test-bats` | Build test binary, resize terminal, run BATS | +| `clean` | Remove `target/` | + +Build-time injection via `-ldflags`: `Version`, `CommitSha`, `BuildTime`, and optionally `TestMode`. + +**CI/CD** (GitHub Actions): +- `ci.yaml`: govulncheck, golangci-lint, unit tests (short, race, cover), e2e (BATS against containerized platform), platform cross-tests +- `release.yaml`: release-please versioning, cross-compile, artifact upload +- Supporting: codeql, pr-lint, backport, dependabot, dependency-review + +### Testing + +**Unit tests**: Minimal — 3 test files: +- `cmd/execute_test.go` — Execute/MountRoot functions +- `pkg/utils/identifier_test.go` — `NormalizeEndpoint()` +- `pkg/utils/pemvalidate_test.go` — PEM validation + +**End-to-end tests (BATS)**: Primary testing strategy — 19 test suites in `e2e/`: +- Cover all major command groups (attributes, namespaces, auth, encrypt-decrypt, kas, etc.) +- Run against a real platform instance (containerized in CI) +- Test both styled and `--json` output modes +- Test binary built with `TestMode=true` (in-memory profile store) +- Terminal size controlled for consistent output testing +- Optional TestRail integration for result uploading + +### Key Dependencies + +| Category | Library | Purpose | +|----------|---------|---------| +| CLI | `spf13/cobra` | Command framework | +| SDK | `opentdf/platform/sdk` | Platform gRPC communication | +| Types | `opentdf/platform/protocol/go` | Protobuf-generated types | +| TUI | `charmbracelet/bubbletea` | Terminal UI framework | +| Style | `charmbracelet/lipgloss` | Terminal styling | +| Tables | `evertras/bubble-table` | Table rendering | +| Docs | `charmbracelet/glamour` | Markdown rendering | +| Prompts | `charmbracelet/huh` | Interactive forms | +| Auth | `zitadel/oidc/v3` | OIDC client | +| JWT | `go-jose/go-jose/v3`, `golang-jwt/jwt/v5` | JWT parsing | +| Profiles | `jrschumacher/go-osprofiles` | OS profile storage | +| MIME | `gabriel-vasile/mimetype` | File type detection | +| Frontmatter | `adrg/frontmatter` | YAML frontmatter parsing | + +### Conventions and Patterns Summary + +1. **Doc-driven commands**: Command metadata lives in `docs/man/*.md`, not Go code +2. **Three-layer separation**: `cmd/` → `pkg/handlers/` → `platform/sdk` +3. **Functional options**: Used for `Handler` and `Cli` construction +4. **Dual output**: Every command supports styled and JSON output +5. **Profile-based config**: No config files; profiles store everything +6. **Explicit initialization**: `InitCommands()` pattern over Go `init()` +7. **Confirmation for destructive ops**: `ConfirmAction`/`ConfirmTextInput` with `--force` bypass +8. **Mountable design**: `MountRoot()` allows embedding as a subcommand of another CLI +9. **Package-level variable state**: Some flag values stored as package-level vars (e.g., `attributeValues`, `metadataLabels`) +10. **Exit-on-error**: Commands call `os.Exit()` on error rather than returning errors up the stack + +--- + +## To-Be: Intended Architecture + +### Decided Direction + +**`FlagHelper` → `Flags` migration**: The `FlagHelper` field on `Cli` is a temporary alias introduced in `d6932f30` with the explicit comment "Temp wrapper for FlagHelper until we can remove it". Migration is ~93% complete (289 usages of `Flags` vs 20 remaining `FlagHelper` call sites across 6 files). Direction: complete the migration and remove the `FlagHelper` field. + +**Deprecated `config` command**: The `config` command was deprecated in PR #719 when output format storage moved to the profile system. It currently prints a Cobra deprecation warning but still executes. Direction: remove the `config` command entirely since the functionality is fully available via `profile` commands. + +**Testing strategy**: E2E (BATS) is the primary testing approach and provides good coverage for the CRUD-heavy command surface. Unit tests are appropriate for non-CRUD helpers and utility functions (e.g., `pkg/utils/`, `pkg/cli/` helpers, `pkg/man/` parsing). Direction: add unit tests for non-trivial helper logic, not for CRUD command wiring. + +**Package-level variable state**: The `StringSliceVarP` pattern (package-level vars bound to cobra flags) is a cobra limitation for slice flags, not an architectural concern. The pattern of declaring the var, binding via `VarP`, then reading through `c.Flags.GetStringSlice()` with validation is intentional and should be maintained where cobra requires it. + +**`NewHandler` as preRun hook**: There is a TODO and [issue #383](https://github.com/opentdf/otdfctl/issues/383) to move handler initialization to a Cobra `PersistentPreRunE` hook. This would reduce boilerplate in every command handler. Direction: pursue when ready, but not a blocker. + +### Open Questions + +- What is the intended future of the TUI (`tui/` directory)? (Undecided) +- How should the profile system evolve? (Current state is functional) + +### Known TODOs in Code + +From `README.md`: +- [ ] Add support for JSON input as piped input +- [ ] Add help level handler for each command +- [ ] Add support for `--verbose` persistent flag +- [x] Helper functions to support common tasks (done via `pkg/cli`) + +From `cmd/common/common.go`: +- [ ] Make `NewHandler` a preRun hook ([#383](https://github.com/opentdf/otdfctl/issues/383)) + +### Alignment Concerns + +| Area | Status | Notes | +|------|--------|-------| +| `FlagHelper` → `Flags` | 93% migrated | 20 remaining call sites in 6 files | +| Deprecated `config` cmd | Still present | Functional but warns; should be removed | +| Unit test coverage | Low | Only 3 test files; non-CRUD helpers lack coverage | +| Pattern consistency | Mostly consistent | Policy commands follow the same pattern well | +| Output formatting | Consistent | Dual-mode (styled/JSON) is well-implemented | +| Flag boilerplate | Verbose but consistent | `GetDocFlag` pattern is repetitive but uniform | +| Error handling | Consistent | Exit-on-error pattern used throughout | diff --git a/docs/man/config/_index.md b/docs/man/config/_index.md deleted file mode 100644 index 247aa36b..00000000 --- a/docs/man/config/_index.md +++ /dev/null @@ -1,10 +0,0 @@ ---- -title: Manage Configuration - -command: - name: config ---- - -## DEPRECATED - -**Please use `profile set-output-format` instead** diff --git a/docs/man/config/output.md b/docs/man/config/output.md deleted file mode 100644 index 7ed18831..00000000 --- a/docs/man/config/output.md +++ /dev/null @@ -1,15 +0,0 @@ ---- -title: Define the configured output format - -command: - name: output - flags: - - name: format - description: "'json' or 'styled' as the configured output format" - default: "styled" - required: false ---- - -## DEPRECATED - -**Please use `profile set-output-format` instead** diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index 13e64a59..cb12e83b 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -11,9 +11,8 @@ type Cli struct { args []string // Helpers - Flags *flagHelper - FlagHelper *flagHelper - printer *Printer + Flags *flagHelper + printer *Printer } // New creates a new Cli object @@ -35,8 +34,6 @@ func New(cmd *cobra.Command, args []string, options ...cliVariadicOption) *Cli { } cli.Flags = newFlagHelper(cmd) - // Temp wrapper for FlagHelper until we can remove it - cli.FlagHelper = cli.Flags cli.printer = newPrinter(cli) if opts.printerJSON { From d2bb1a1cec7c196038730c8e17bcb288b0a285da Mon Sep 17 00:00:00 2001 From: Ryan Schumacher Date: Mon, 23 Feb 2026 21:58:51 -0500 Subject: [PATCH 2/2] docs: fix stale content in ARCHITECTURE.md Update doc to reflect the actual codebase state: FlagHelper alias is removed, config command is deleted, alignment table is current. Co-Authored-By: Claude Opus 4.6 --- docs/ARCHITECTURE.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5c3d79b0..8dd52f4d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -24,7 +24,6 @@ The codebase follows a three-layer architecture: │ ├── policy/ Policy CRUD subcommands │ │ ├── tdf/ Encrypt/decrypt/inspect │ │ ├── auth/ Login/logout/credentials │ -│ ├── config/ (deprecated) │ │ ├── dev/ Dev/playground commands │ │ ├── common/ Shared command helpers │ │ ├── root.go Root command, init, wiring │ @@ -145,7 +144,7 @@ Key components: - **`sdkHelpers.go`**: SDK data transformers (`GetSimpleAttribute`, `ConstructMetadata`, key algorithm converters) - **`pipe.go`**: Stdin/file reading utilities -**Note**: `Cli.FlagHelper` and `Cli.Flags` are the same object (aliased), with `FlagHelper` being the deprecated name. +All command code accesses flags through `c.Flags` (e.g., `c.Flags.GetRequiredString("name")`). ### Dual Output Mode @@ -237,8 +236,6 @@ otdfctl │ ├── resource-mappings [get, list, create, update, delete] │ ├── resource-mapping-groups [get, list, create, update, delete] │ └── registered-resources [get, list, create, update, delete] -├── config (deprecated → use profile) -│ └── output ├── dev │ ├── design-system │ └── selectors [generate, test] @@ -324,9 +321,9 @@ Build-time injection via `-ldflags`: `Version`, `CommitSha`, `BuildTime`, and op ### Decided Direction -**`FlagHelper` → `Flags` migration**: The `FlagHelper` field on `Cli` is a temporary alias introduced in `d6932f30` with the explicit comment "Temp wrapper for FlagHelper until we can remove it". Migration is ~93% complete (289 usages of `Flags` vs 20 remaining `FlagHelper` call sites across 6 files). Direction: complete the migration and remove the `FlagHelper` field. +**`FlagHelper` → `Flags` migration**: Complete. The deprecated `FlagHelper` alias was removed; all code now uses `c.Flags`. -**Deprecated `config` command**: The `config` command was deprecated in PR #719 when output format storage moved to the profile system. It currently prints a Cobra deprecation warning but still executes. Direction: remove the `config` command entirely since the functionality is fully available via `profile` commands. +**Deprecated `config` command**: Removed. The `config` command was deprecated in PR #719 when output format storage moved to the profile system, and has since been deleted along with its docs. **Testing strategy**: E2E (BATS) is the primary testing approach and provides good coverage for the CRUD-heavy command surface. Unit tests are appropriate for non-CRUD helpers and utility functions (e.g., `pkg/utils/`, `pkg/cli/` helpers, `pkg/man/` parsing). Direction: add unit tests for non-trivial helper logic, not for CRUD command wiring. @@ -354,8 +351,8 @@ From `cmd/common/common.go`: | Area | Status | Notes | |------|--------|-------| -| `FlagHelper` → `Flags` | 93% migrated | 20 remaining call sites in 6 files | -| Deprecated `config` cmd | Still present | Functional but warns; should be removed | +| `FlagHelper` → `Flags` | Complete | All code uses `c.Flags`; alias removed | +| Deprecated `config` cmd | Removed | Deleted along with docs | | Unit test coverage | Low | Only 3 test files; non-CRUD helpers lack coverage | | Pattern consistency | Mostly consistent | Policy commands follow the same pattern well | | Output formatting | Consistent | Dual-mode (styled/JSON) is well-implemented |