feat(ctl): allow library embedders to override CLI branding#3382
Open
daviddahl wants to merge 14 commits into
Open
feat(ctl): allow library embedders to override CLI branding#3382daviddahl wants to merge 14 commits into
daviddahl wants to merge 14 commits into
Conversation
Add an additive, opt-in Branding type (binary name + output-envelope schemaVersion) so a downstream binary embedding the otap-df-ctl library can emit help, shell completions, and machine-readable JSON envelopes under its own identity instead of dfctl. - New Branding struct + process-global set-once active branding (mirrors the crypto-provider OnceLock pattern); default reproduces the dfctl identity. - New run_with_terminal_and_diagnostics_branded entrypoint installs branding then delegates to the existing run path; existing entrypoints and the standalone dfctl binary are unchanged. - Route the binary-name and envelope schemaVersion reads through the active branding (lib diagnostics, render/output envelopes, error envelope, completions). - Add an integration test proving the override reaches the emitted envelope. Scope is intentionally minimal (binary name + envelope schemaVersion); schema catalog identifiers, schema $id URLs, DFCTL_* env-var prefixes, and the TUI remain dfctl-branded and may be addressed in follow-ups. Closes open-telemetry#3370 Signed-off-by: David Dahl <d.dahl@f5.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3382 +/- ##
==========================================
+ Coverage 86.24% 86.26% +0.02%
==========================================
Files 741 742 +1
Lines 292447 292619 +172
==========================================
+ Hits 252211 252441 +230
+ Misses 39712 39654 -58
Partials 524 524
🚀 New features to boost your workflow:
|
…s paths Raise patch coverage above the project target by exercising the branding-aware code paths the initial change left untested: - output.rs: unit tests for the NDJSON mutation, agent-json mutation, NDJSON stream event, and NDJSON log envelopes (assert the default dfctl/v1 schemaVersion). - completions.rs: unit tests for script generation, the missing-shell usage hint, and the Elvish activation note (assert they use the active binary name). - tests/branding.rs: a verbose 'config view' integration test asserting the diagnostics line on stderr carries the embedder binary name. Test-only change; no library behavior changes. Signed-off-by: David Dahl <d.dahl@f5.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in, process-global Branding mechanism to the otap-df-ctl library so downstream binaries embedding the crate can override the CLI’s user-visible identity (binary name) and machine-readable envelope schemaVersion, while keeping the standalone dfctl behavior and existing public entrypoints unchanged.
Changes:
- Added a new public
Brandingtype and a new branded entrypointrun_with_terminal_and_diagnostics_branded(...)that installs branding set-once per process. - Routed envelope
schemaVersion, diagnostics prefixing, error agent envelope schema version, and shell completions naming through the active branding. - Added coverage via a new integration test validating branded schema version and diagnostics binary-name prefixing, plus unit tests ensuring default branding remains
dfctl/v1.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rust/otap-dataflow/crates/ctl/src/branding.rs | Adds the Branding type and process-global active branding storage. |
| rust/otap-dataflow/crates/ctl/src/lib.rs | Exposes Branding publicly and adds a branded library entrypoint; routes some messages through branding. |
| rust/otap-dataflow/crates/ctl/src/render/output.rs | Uses active branding for JSON envelope schemaVersion and adds tests for default behavior. |
| rust/otap-dataflow/crates/ctl/src/error.rs | Routes agent JSON error envelope schema version through active branding. |
| rust/otap-dataflow/crates/ctl/src/commands/completions.rs | Uses active branding for completion generation/install naming and improves test coverage. |
| rust/otap-dataflow/crates/ctl/tests/branding.rs | New integration test validating branding overrides in schemaVersion and diagnostics. |
| rust/otap-dataflow/.chloggen/ctl-configurable-branding.yaml | Adds a changelog entry for the new embedders-facing branding feature. |
Comments suppressed due to low confidence (1)
rust/otap-dataflow/crates/ctl/src/lib.rs:30
- The
BIN_NAMEdoc comment still claims it is used for help/completions/diagnostics, but those code paths now route through the activeBranding(e.g.,branding::active().bin_name). Updating the comment would prevent embedders from using the wrong source of truth.
/// Installed command name used in help, completions, generated command
/// metadata, diagnostics, and TUI command hints.
pub const BIN_NAME: &str = "dfctl";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lalitb
reviewed
Jun 29, 2026
lalitb
reviewed
Jun 29, 2026
| /// (set-once) before the command runs; the non-branded entrypoints leave it | ||
| /// unset and therefore use [`Branding::default`]. | ||
| pub async fn run_with_terminal_and_diagnostics_branded( | ||
| cli: ParsedCli, |
Member
There was a problem hiding this comment.
Seems this method receives an already parsed Cli, so the branding is installed too late for Clap-generated help, usage text, and parse errors. Should the PR/changelog wording around branded help be narrowed, or do we need a branded parse/command construction path for embedders?
Contributor
Author
There was a problem hiding this comment.
Added Cli::command_branded(&Branding) and Cli::try_parse_from_branded(args, &Branding) so an embedder can produce a clap Command with their own binary name before parsing: --help, --version, and parse errors now all carry the embedder identity.
Tested via command.get_name() and by asserting parse errors contain the branded name. The existing run_*_branded entrypoint installs runtime branding separately (same flow as before).
…ding Addresses review feedback on open-telemetry#3382: - Add Cli::command_branded() and Cli::try_parse_from_branded() so an embedder can produce a clap Command with their own binary name before parsing -- --help, --version, and parse errors now carry the embedder identity at parse time, not just run time. Tests: command name + parse-error output. - Route the command catalog (commands --output json) through branding::active().bin_name: the catalog's binary field, command IDs (dfctl.pipelines.get), command-line prefixes, examples, and human-render headers now follow the active branding rather than the compile-time BIN_NAME constant. Integration test asserts embedder-branded catalog. - Deduplicate the dfctl literal: introduce branding::DEFAULT_BIN_NAME as the single source of truth; BIN_NAME and Branding::default() both reference it. Fix BIN_NAME's doc to point embedders at Branding + command_branded. Verified: clippy -D warnings clean, cargo test -p otap-df-ctl (120 unit + 3 integration), cargo xtask check (workspace green), cargo build --locked. Signed-off-by: David Dahl <d.dahl@f5.com>
lalitb
reviewed
Jul 7, 2026
| schema_version: SCHEMA_VERSION, | ||
| generated_at: humantime::format_rfc3339_seconds(SystemTime::now()).to_string(), | ||
| binary: BIN_NAME, | ||
| binary: bin_name, |
Member
There was a problem hiding this comment.
Now that the catalog can use a custom binary name, the schema still says binary must always be dfctl. Should we update the schema too, or keep the catalog binary field fixed as dfctl?
Contributor
Author
There was a problem hiding this comment.
Thanks! Relaxed the schema: "binary" is now { "type": "string", "description": "Active binary name at catalog generation time." }
ThomsonTan
reviewed
Jul 7, 2026
…tput The catalog JSON Schema previously required the binary field to always be 'dfctl' (const BIN_NAME), but embedders can now override it via Branding. Replace the const with 'type: string' and a description noting it reflects the active binary name at generation time. Also route the remaining BIN_NAME references in schemas.rs through branding::active().bin_name (human-render header, command example, catalog header), matching the catalog.rs treatment. Verified: cargo xtask check (workspace green), cargo build --locked. Signed-off-by: David Dahl <d.dahl@f5.com>
Co-authored-by: Tom Tan <lilotom@gmail.com>
The applied review suggestion left a duplicated, unterminated expression in try_parse_from_branded, breaking compilation (expected ';', found keyword 'Self'). Collapse to the single intended chain that parses via the branded command and applies agent-mode effective defaults through from_arg_matches_with_effective_defaults. Verified: cargo clippy -D warnings, cargo test -p otap-df-ctl, cargo xtask check. Signed-off-by: David Dahl <d.dahl@f5.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Summary
Add an additive, opt-in
Brandingtootap-df-ctlso a downstream binary thatembeds the library can emit help, shell completions, and machine-readable
JSON envelopes under its own identity instead of
dfctl.Brandingtype (bin_name+ envelopeschema_version) and a newrun_with_terminal_and_diagnostics_branded(..., branding)entrypoint. Thebranding is installed process-wide (set-once, mirroring the existing
crypto-provider
OnceLock); the existing entrypoints leave it unset and useBranding::default().schemaVersionreads are routed through theactive branding (lib diagnostics,
render/outputenvelopes, the errorenvelope, and completions).
dfctlbinary and all existing public entrypoints/signaturesare unchanged — the default branding reproduces the current
dfctl/dfctl/v1identity exactly.Scope is intentionally minimal — binary name and envelope
schemaVersion.Out of scope here (possible follow-ups): schema-catalog identifiers
(
dfctl.*.v1), schema$idURLs/titles,DFCTL_*environment-variableprefixes, and the interactive TUI command hints.
What issue does this PR close?
How are these changes tested?
dfctloutput/branding tests remain green viaBranding::default().crates/ctl/tests/branding.rs) drives the brandedentrypoint with a non-default
Brandingand asserts the emittedschemaVersionis overridden. It uses a connection-free command(
commands --output agent-json) so no admin server is required, and runs as aseparate integration-test process so the process-global branding does not
affect other tests.
cargo xtask check(structure checks,cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace) passes.Are there any user-facing changes?
Yes — a new public library entrypoint (
run_with_terminal_and_diagnostics_branded)and a new public
Brandingtype for library embedders. No change to thestandalone
dfctlbinary's behavior or output.Changelog
.chloggen/*.yamlentry.