Skip to content

feat(build): add defaults-without-telemetry feature alias - #2843

Open
russellb wants to merge 1 commit into
NVIDIA:mainfrom
russellb:feat/defaults-without-telemetry
Open

feat(build): add defaults-without-telemetry feature alias#2843
russellb wants to merge 1 commit into
NVIDIA:mainfrom
russellb:feat/defaults-without-telemetry

Conversation

@russellb

Copy link
Copy Markdown
Contributor

Summary

Adds a defaults-without-telemetry Cargo feature alias to the three binary crates that carry telemetry, so compiling telemetry out no longer means hand-maintaining a keep-list of each crate's other default features. Telemetry-free builds become --no-default-features --features defaults-without-telemetry.

Related Issue

No accepted issue yet — raising this for maintainer disposition. It came out of a contributor question about whether --no-default-features is a safe stand-in for "disable telemetry." It is not, and that is already a live correctness bug (see Changes), so this is filed as a PR rather than a spike. Happy to open a feature request issue first if the convention should apply here.

Changes

Cargo cannot subtract a single default feature, so the documented way to compile telemetry out is --no-default-features plus every other default re-added by hand. That keep-list is already wrong for operators:

Crate default Bare --no-default-features also drops
openshell-server ["telemetry"]
openshell-driver-vm ["telemetry"]
openshell-sandbox ["telemetry", "bundled-ca-roots"] bundled-ca-roots

Following the README today therefore silently swaps the supervisor from bundled Mozilla roots onto the platform trust store. The alias fixes that and stays correct as the default set grows.

  • Add defaults-without-telemetry to openshell-server, openshell-sandbox, and openshell-driver-vm, each enumerating its defaults minus telemetry.
  • Add a compile_error! in each crate root for telemetry + defaults-without-telemetry. The alias is a keep-list, not a switch — without the guard, --features defaults-without-telemetry on its own (no --no-default-features) would compile a telemetry-on binary that reads as telemetry-free.
  • Add rust:verify:defaults-without-telemetry (tasks/scripts/verify-defaults-without-telemetry.sh), which asserts via cargo metadata that each alias still equals its crate's defaults minus telemetry, and that the mutual-exclusion error is wired up. The second check matches on the compile_error! text rather than a nonzero exit code, so it cannot pass vacuously on hosts where openshell-driver-vm fails to build for unrelated reasons.
  • Point rust:verify:telemetry-off at the alias, so CI builds the artifacts the same way the docs tell operators to.
  • Run the new task in branch-checks.yml.
  • Update the telemetry section of README.md and the Build Features section of architecture/build.md.

openshell-core also defaults to telemetry, but is left alone: every consumer already depends on it with default-features = false, so an alias there would be a no-op.

Testing

Command Result
mise run pre-commit pass
mise run test pass, 0 failures
mise run rust:verify:telemetry-off pass — positive control present in the default gateway, markers absent from both telemetry-free binaries
mise run rust:verify:defaults-without-telemetry pass, 6/6 checks
mise run rust:verify:system-ca-roots pass

Negative-controlled the drift check by temporarily setting the sandbox alias to []; it failed with the expected diff and exit 1, then passed again on restore.

Not run: mise run e2e. This changes build-time feature wiring, not sandbox runtime behavior, and the telemetry-off guard already inspects the resulting binaries.

  • mise run pre-commit passes
  • Unit tests added/updated — covered by the two rust:verify:* guards rather than #[test]s; feature-set drift and compile_error! wiring are not observable from inside a compiled test binary
  • E2E tests added/updated (if applicable) — not applicable

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Cargo cannot subtract a single default feature, so compiling telemetry out
meant `--no-default-features` plus a hand-maintained keep-list of the crate's
other defaults. That keep-list was already wrong for operators: telemetry is
the only default on openshell-server and openshell-driver-vm, but
openshell-sandbox also defaults to `bundled-ca-roots`, so a bare
`--no-default-features` silently swapped the supervisor onto the platform
trust store.

Add a `defaults-without-telemetry` alias to each of the three telemetry-
carrying binary crates, enumerating every default except `telemetry`.
Telemetry-free builds become `--no-default-features --features
defaults-without-telemetry` and stay correct as the default set grows.

The alias is a keep-list, not a switch. Enabling it on top of the defaults
would otherwise produce a telemetry-on binary that reads as telemetry-free, so
each crate root carries a `compile_error!` for the `telemetry` +
`defaults-without-telemetry` combination.

Add `rust:verify:defaults-without-telemetry` to guard both properties: each
alias still equals its crate's defaults minus `telemetry`, and the
mutual-exclusion error is wired up. The additive-misuse check matches on the
`compile_error!` text rather than a nonzero exit code so it cannot pass
vacuously on hosts where openshell-driver-vm fails to build for unrelated
reasons. `rust:verify:telemetry-off` now builds through the alias.

Signed-off-by: Russell Bryant <rbryant@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mrunalp

mrunalp commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

The changes look fine. @drew @johntmyers wdyt?

@johntmyers johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback test:e2e Requires end-to-end coverage labels Aug 24, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/2843 does not exist yet. A maintainer needs to comment /ok to test 9d9688a15692ec2c33768a0ef17391379f83af83 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 9d9688a

@johntmyers johntmyers added test:e2e Requires end-to-end coverage and removed test:e2e Requires end-to-end coverage labels Aug 24, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 9d9688a. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Thanks @mrunalp. You said the changes look fine and asked for another look; I checked the complete current-head patch, including the Cargo feature aliases, mutual-exclusion guards, verification script, CI wiring, and documentation. The independent review found no blocking or non-blocking findings.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • None

The test:e2e label is applied, the contributor mirror matches this head, and Branch/Helm/E2E workflows have started. The E2E Label Help bot requires current-head Branch E2E run 32782778291 to be rerun after its first attempt becomes rerunnable; Gator will perform that already-authorized action in a later watch cycle.

Gator metadata
  • Validation: Concentrated build correctness fix with a clear operator impact; no duplicate candidate found, and trusted maintainer feedback supports project fit.
  • Docs: Source-build behavior is documented in README.md and architecture/build.md; no separate Fern page is needed because the published docs do not currently expose a source-build Cargo-feature workflow.
  • Checks: DCO and vouch are green; current-head Branch Checks and Helm Lint are queued or running.
  • E2E: test:e2e applied; /ok to test 9d9688a15692ec2c33768a0ef17391379f83af83 created the exact-head mirror; bot-required rerun remains pending until run 32782778291 becomes rerunnable.
  • Head SHA: 9d9688a15692ec2c33768a0ef17391379f83af83
  • Base SHA: 4d7f402ce2ac88958c3fb3ae0018d7e1a17a9783
  • Merge base SHA: 7909fb5d0f54a06e26eb79e47885d7dd105aef24
  • Patch ID: 42411c46c88c79dfc1f47f79816e001a6d25ca96
  • Gator payload: 7
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

@johntmyers
johntmyers enabled auto-merge August 24, 2026 22:07
@johntmyers
johntmyers added this pull request to the merge queue Aug 24, 2026
@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Aug 24, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 24, 2026
@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates gator:merge-ready and removed gator:watch-pipeline Gator is monitoring PR CI/CD status gator:blocked Gator is blocked by process or repository gates gator:merge-ready labels Aug 24, 2026
@johntmyers johntmyers added gator:merge-ready gator:blocked Gator is blocked by process or repository gates and removed gator:merge-ready gator:blocked Gator is blocked by process or repository gates labels Aug 25, 2026
@russellb

Copy link
Copy Markdown
Contributor Author

@johntmyers

#2843 was automatically removed from the merge queue due to failed status checks.

though I don't see any checks on github in a failing state? Is there something not visible to me? Or does it just need to be re-queued? Let me know if there's something I need to fix!

@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates gator:merge-ready gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:merge-ready gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status labels Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:merge-ready test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants