-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Secrets set now handles test vs production secrets #8359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great devx
export async function maybeAddSecretToYaml(secretName: string): Promise<void> { | ||
export async function maybeAddSecretToYaml( | ||
secretName: string, | ||
fileName: string = APPHOSTING_BASE_YAML_FILE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update docstring above
Given a secret name, guides the user whether they want to add that secret to the specified apphosting yaml file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed other bullets too. Good catch.
src/apphosting/secrets/dialogs.ts
Outdated
} else if (upper.startsWith("TEST")) { | ||
upper = upper.substring("TEST".length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not stick with "TEST_" only?
Unless there's a common pattern we want to support, "TEST" could always lead to surprises for secrets coincidentally named with a "TEST" prefix (TESTER, TESTING, , and instead we could just say we only support stripping "TEST_".
especially since we're forcing this upon the user as long as they tell us some secret is only for the local emulator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable point
apphosting:secrets:set now asks if the secret is test-only and can update apphosting.emulator.yaml instead for these cases (as well as granting individuals or groups access).
stripe-key
andtest-stripe-key
to both point toSTRIPE_KEY
in apphosting.yaml and apphosting.emulator.yaml respectively