Skip to content

Resolve API key from env var in config show#7

Merged
dweinber merged 6 commits into
mainfrom
fix/config-show-resolved-source
Apr 29, 2026
Merged

Resolve API key from env var in config show#7
dweinber merged 6 commits into
mainfrom
fix/config-show-resolved-source

Conversation

@dweinber
Copy link
Copy Markdown
Member

@dweinber dweinber commented Apr 29, 2026

Summary

bitmovin config show only inspected the config file, so it reported API Key: (not set) even when the user was authenticated via the --api-key flag or BITMOVIN_API_KEY environment variable. The CLI's getClient() honors those sources, so this disagreement was misleading: users and tooling couldn't tell from config show which API key would actually be used.

Changes

  • Resolve the API key consistently with getClient() precedence: --api-key flag first, BITMOVIN_API_KEY env var second, config file third.
  • Report which source is currently in use (--api-key flag, BITMOVIN_API_KEY env var, config file, or none) so the user can tell at a glance where the key came from.
  • Surface the source as apiKeySource (flag / env / config-file / none) in --json output for scripting.
  • Keep masking behavior consistent with the shared maskSecret() helper.
  • Move API key resolution into a lightweight helper so config show does not import the Bitmovin SDK.

Tests

  • Tests cover resolution paths for flag, env, config file, and neither.
  • JSON mode tests cover apiKeySource for config-file, env, and none, including null for unset apiKey.
  • Tests verify empty API key values are shown as unset, matching getClient() authentication behavior.
  • All tests pass.

Test plan

  • npm run build
  • npm run lint
  • npm test
  • config show correctly identifies the API key source
  • No previously-stored API key leaks across resolution paths

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates bitmovin config show to resolve and display the effective API key consistently with the CLI’s runtime behavior (env var vs config file), including a new apiKeySource field in JSON output.

Changes:

  • Added shared API key resolution logic (resolveApiKey) with source tracking (env / config-file / none / flag).
  • Updated config show to display the resolved key (masked with ) and its source; added apiKeySource in --json output.
  • Expanded Vitest coverage for env/config/none paths and JSON output.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/lib/client.ts Introduces resolveApiKey and uses it in getClient()
src/commands/config/show.ts Uses resolved API key + source for human/JSON output; updates masking
test/commands/config.test.ts Adds tests for env precedence, none case, and JSON apiKeySource

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/commands/config.test.ts Outdated
Comment thread src/lib/client.ts Outdated
Comment thread src/commands/config/show.ts Outdated
Comment thread src/commands/config/show.ts Outdated
@lukaskroepfl
Copy link
Copy Markdown
Member

LGTM with a couple of breaking-change call-outs.

Strengths

  • resolveApiKey extracted into client.ts and reused by getClient — eliminates the silent disagreement between "what config show thinks" and "what getClient actually uses". Nice DRY.
  • apiKeySource enum is the right shape for scripted consumers.
  • Including 'flag' as a source is correct — --api-key is in BaseCommand.baseFlags, so config show --api-key foo works and now reports it accurately.

Breaking changes worth calling out in release notes

  1. --json value type change: apiKey, tenantOrgId, defaultRegion flipped from '(not set)' strings to null. Anyone doing if (data.apiKey === '(not set)') will break. Switching to null is the right design — just flag it.
  2. Mask format change: 12345678...789abc12345678…789abc (3 dots → U+2026). Cosmetic but visible in any output-scraping consumer.

Cross-PR consistency

This PR uses `<=12 → ***` then `8 + … + 4`. PR #6 uses `<=8 → ***` then `4 + ... + 4`. Two mask functions, two styles. Pick one and put it in `lib/output.ts` (or similar) before both land.

Tests cover all three resolution paths and the JSON `apiKeySource` field — coverage is solid.

@lukaskroepfl
Copy link
Copy Markdown
Member

Heads-up: PR #6 just landed a shared maskSecret helper in src/lib/secrets.ts and switched config/show.ts over to it (with the format 1234...9abc — 4 + ... + 4). When you rebase this PR on top of it, please drop the inline value.slice(0, 8) + '…' + value.slice(-4) here and import the shared helper instead, so the CLI ships one mask format end-to-end. The behavior of this PR (env-var resolution + apiKeySource) is otherwise unchanged.

dweinber and others added 3 commits April 29, 2026 15:21
`bitmovin config show` only inspected the config file, so it printed
"API Key: (not set)" even when the user was authenticated via the
BITMOVIN_API_KEY environment variable. The CLI's getClient() honors
the env var, so this disagreement was misleading: users (and tooling)
couldn't tell from `config show` whether they were authenticated.

Resolve the API key the same way getClient() does (env var first,
then config file) and report which source is in use ("BITMOVIN_API_KEY
env var" vs "config file"). Surface the source as `apiKeySource` in
--json output for scripting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extracts the env-var-first precedence chain into resolveApiKey() in
client.ts so both call sites use the same source of truth. Also tightens
describeSource exhaustiveness, removes a duplicate loadConfig() call,
and snapshots BITMOVIN_API_KEY in test setup/teardown for isolation.
@dweinber dweinber force-pushed the fix/config-show-resolved-source branch from 6b02271 to e235795 Compare April 29, 2026 13:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread CHANGELOG.md
Comment thread src/commands/config/show.ts Outdated
Comment thread src/commands/config/show.ts Outdated
Comment thread src/commands/config/show.ts
Comment thread test/commands/config.test.ts
Comment thread test/commands/config.test.ts
dweinber and others added 2 commits April 29, 2026 15:36
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Member

@lukaskroepfl lukaskroepfl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All review points addressed plus a couple of thoughtful additions:

  • Mask consistency — switched to the shared maskSecret (4 + ... + 4). One mask format CLI-wide. ✅
  • Breaking change documented — CHANGELOG entry calls out the --json shape change explicitly.
  • resolveApiKey extracted to lib/api-key.ts — cleaner home than client.ts, no public-API impact.
  • Empty-string handlingnormalizeForDisplay treats BITMOVIN_API_KEY="" as "(not set)" while getClient('') still throws. Behavior matches the old ??-chain semantics; nice that you caught this and locked it in with two regression tests.
  • --api-key flag precedence test — good addition.

The assertNever after the exhaustive switch is unreachable but harmless — leave it for future-proofing if you like.

Approving.

@dweinber dweinber merged commit 27c4b46 into main Apr 29, 2026
3 checks passed
@dweinber dweinber deleted the fix/config-show-resolved-source branch April 29, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants