[APPS] Default apps plugin auth to OAuth without keys#427
Conversation
sdkennedy2
changed the title
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| ### apps.authOverrides.method | ||
|
|
||
| > default: `apiKey` | ||
| > default: `apiKey` when both `DD_API_KEY` and `DD_APP_KEY` are configured, otherwise `oauth` |
There was a problem hiding this comment.
if OAuth is loaded when nothing is configured, then I think the default is oauth now.
| > default: `apiKey` when both `DD_API_KEY` and `DD_APP_KEY` are configured, otherwise `oauth` | |
| > default: `oauth` |
There was a problem hiding this comment.
Wouldn't it be when DD_API_KEY and DD_APP_KEY are set, the default is apiKey, otherwise the default is oauth?
yoannmoinet
left a comment
yoannmoinet
left a comment
There was a problem hiding this comment.
LGTM.
I have a comment, but it's not blocking.
Could be a follow-up to avoid duplications and enforce existing patterns.
| const authEnvVars = [ | ||
| 'DATADOG_API_KEY', | ||
| 'DD_API_KEY', | ||
| 'DATADOG_APP_KEY', | ||
| 'DD_APP_KEY', | ||
| 'DATADOG_APPS_AUTH_METHOD', | ||
| 'DD_APPS_AUTH_METHOD', | ||
| ] as const; | ||
|
|
||
| const savedAuthEnv = Object.fromEntries( | ||
| authEnvVars.map((key) => [key, process.env[key]]), | ||
| ) as Record<(typeof authEnvVars)[number], string | undefined>; | ||
|
|
||
| const restoreAuthEnv = () => { | ||
| for (const key of authEnvVars) { | ||
| const value = savedAuthEnv[key]; | ||
| if (value === undefined) { | ||
| delete process.env[key]; | ||
| } else { | ||
| process.env[key] = value; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
We have this cleanEnv() system here:
build-plugins/packages/tests/src/_jest/helpers/env.ts
Lines 68 to 99 in ad82fc2
Currently, we only call it in beforeAll/afterAll but maybe we could extend/modify it to suit your use-case?
Not sure if it would have a real impact if we were to do it on beforeEach/afterEach instead.
(And also make it leverage OVERRIDE_VARIABLES instead of duplicating them)
3 participants
Motivation
Datadog Apps local development and upload should use OAuth by default when API/App keys are not configured. This matches the intended customer setup where
npm run devandnpm run uploadcan start an OAuth browser flow instead of failing with missing key guidance.Changes
The apps plugin now resolves
apps.authOverrides.methodin this order:Explicit config and environment overrides still win, and users can force API/App-key auth with
DD_APPS_AUTH_METHOD=apiKey. The README documents the conditional default, and the validation tests cover configured keys, env keys, incomplete auth, explicit override precedence, and env isolation.QA Instructions
In a Datadog App using
@datadog/vite-plugin, runnpm run devornpm run uploadwithoutDD_API_KEY/DD_APP_KEY; the apps plugin should select OAuth. Set both keys and rerun to confirm API/App-key auth remains the default. SetDD_APPS_AUTH_METHOD=apiKeyorDD_APPS_AUTH_METHOD=oauthto confirm explicit method selection still wins.Blast Radius
This affects the Datadog Apps plugin auth default for app upload and local backend function execution. Other build plugin products keep their existing auth behavior because the override remains scoped to
apps.authOverrides.method.Documentation