Skip to content
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

Add SSM-based feature flags #873

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add SSM-based feature flags #873

wants to merge 4 commits into from

Conversation

lorenyu
Copy link
Contributor

@lorenyu lorenyu commented Feb 12, 2025

Ticket

Resolves #{TICKET NUMBER OR URL}

Changes

  • Add modules/feature_flags
  • Add feature_flags_config in app-config/env-config/feature_flags.tf with ability to override per environment via feature_flags_override
  • Create feature flags as SSM parameters in infra/<app_name>/service/feature_flags.tf
  • Add feature flags as environment variables with FF_ prefix

Context for reviewers

Re-adding feature flag support using SSM params and environment variables

Testing

see navapbc/platform-test#178

@@ -0,0 +1,4 @@
output "ssm_parameter_arns" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, but given this is tied to SSM/returns ARNs, I wonder if we should name the module like ssm_feature_flags or feature_flags__ssm (assuming we eventually develop a "feature_flag" interface of which this is one implementation) or something?

Comment on lines +1 to +8
import logging
import os
import boto3
logger = logging.getLogger()
def is_feature_enabled(feature_name: str) -> bool:
value = os.environ.get(f"FF_{feature_name}")
return value == "true" if value else False

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import logging
import os
import boto3
logger = logging.getLogger()
def is_feature_enabled(feature_name: str) -> bool:
value = os.environ.get(f"FF_{feature_name}")
return value == "true" if value else False
import os
def is_feature_enabled(feature_name: str) -> bool:
value = os.environ.get(f"FF_{feature_name}")
return value == "true" if value else False

Comment on lines +3 to +4
FOO = false
BAR = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be empty (or commented out) in the template?

Doesn't really hurt anything, but potentially confusing/worth a callout in docs that you'll see a FF_FOO/FF_BAR env var in your runtime environment unless you delete these.

Similar for the override in infra/{{app_name}}/app-config/dev.tf

# checkov:skip=CKV2_AWS_34:Feature flags values don't need to be encrypted
for_each = var.feature_flags

name = "/service/${var.service_name}/${each.key}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to have the FF stuff partitioned out, something like:

Suggested change
name = "/service/${var.service_name}/${each.key}"
name = "/service/${var.service_name}/feature-flag/${each.key}"

If folks name their flags enable_ (or similar) then /service/my-service-dev/enable-new-flow isn't too bad, but /service/my-service-dev/bar would be more confusing than service/my-service-dev/feature-flag/bar.

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.

2 participants