-
Notifications
You must be signed in to change notification settings - Fork 9
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
[Issue #1519] Cherry pick platform's pattern for env vars & ssm secrets #1516
Conversation
@@ -0,0 +1,58 @@ | |||
# Environment variables and secrets |
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.
The CI failures are related to the WAF. They dont seem related to this PR |
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.
Looks good, at least to my rough understanding of terraform
@@ -7,6 +7,5 @@ module "prod_config" { | |||
domain = "api.simpler.grants.gov" | |||
database_instance_count = 2 | |||
database_enable_http_endpoint = true | |||
enable_v01_endpoints = false |
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.
Could we add the ENABLE_V_0_1_ENDPOINTS
value in the service overrides as false?
Doesn't technically need to be there (null == false for this), but just to have the mapping defined for prod.
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.
@chouinar I wanted to leave it null since the string "False" is truthy, ya know?
$ python -c 'print(bool("False"))'
> True
Unless you're certain the that thing reading the env vars is casting strings to booleans "properly"!
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.
We use Pydantic for loading env vars / converting to Python types. It considers false
as a false value: https://docs.pydantic.dev/2.0/usage/types/booleans/
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.
Added! f32edb6
I was able to confirm that the terratest CI failures (eg. |
Summary
Relates to #784
Closes #1519
Copies navapbc/template-infra#549
Time to review: 10 mins
Changes proposed
ENABLE_V_0_1_ENDPOINTS
/enable_v01_endpoints
to use the above patternContext for reviewers
I created this PR via tactical copy-pasting from the https://github.com/navapbc/template-infra/ repo.
The goal of this PR is to DRY our methods for setting environment variables. Notice on the red side of the diff, how I've removed the need to set
enable_v01_endpoints
so many times. Then notice on the green side of the diff, that I only need to setENABLE_V_0_1_ENDPOINTS
twice (for dev and staging). That's the goal of this PR, to pull in platform's very nice pattern for DRY'ing environment variables.Testing
To test this, I added - then removed - the following block from
staging.tf
I then deployed to staging to see the difference. It worked as intended.