[Issue 626] Add basic feature flag support to the API #640
Conversation
Great prototype!| parameters: [] | ||
| parameters: | ||
| - in: header | ||
| name: X-FF-Enable-Opportunity-Log-Msg |
| ENABLE_OPPORTUNITY_LOG_MSG = "enable_opportunity_log_msg" | ||
|
|
||
| def get_header_name(self) -> str: | ||
| value = "-".join([v.capitalize() for v in self.value.lower().split("_")]) |
There was a problem hiding this comment.
Not clear, but aren't headers case-insensitive? What's the point of the funky capitalization + this logic to handle it?
There was a problem hiding this comment.
Mainly so the docs generate with the normal convention
|
|
||
| Naming of feature flags has the following convention: | ||
| * Environment variables are always snake-case all-caps like `ENABLE_OPPORTUNITY_LOG_MSG` | ||
| * Header fields are always prefixed with `X-FF` and are capitalized-kebab-cased like `X-FF-Enable-Opportunity-Log-Msg` |
There was a problem hiding this comment.
Honestly, this is just following some behavior from other systems, so isn't something I thought too in-depth about. The capitalized-kebab-case approach seems to be the standard for headers. I didn't even know they were case-insensitive (and just tested that that is the case). Still prefer to follow the convention so our docs generate with the standard formatting.
Starting a header with X- seems to not be recommended much anymore (although it's not recommended against). Seems like the idea was that it was for internal values, but if they became a standard then it caused issues as people would not want the X- anymore. I don't think any of our variables will ever hit that case, but I can remove the X-. I like the FF bit to try to signal that it's overriding a feature in some way (although open to other naming conventions - this isn't something I'm super familiar with): https://datatracker.ietf.org/doc/html/rfc6648
| @@ -74,3 +79,26 @@ class OpportunitySearchSchema(request_schema.OrderedSchema): | |||
| required=True, | |||
| ) | |||
| paging = fields.Nested(PaginationSchema(), required=True) | |||
|
|
|||
|
|
|||
| class OpportunitySearchHeaderSchema(request_schema.OrderedSchema): | |||
There was a problem hiding this comment.
This is outside the scope of this ticket, but when reviewing this, I'm noting we could remove the OrderedSchema workaround in a future ticket.
There was a problem hiding this comment.
When we do the restructuring for error formatting of responses, I'll do it then. That approach will need a new base schema that we derive everything from to make some of the error formatting utils work happily.
Summary
Fixes #626
Time to review: 10 mins
Changes proposed
Added basic support for feature flags to the API with the following behavior:
Added a very basic feature flag at the moment that logs a message - will replace in the future once we have more realistic feature flags.
Context for reviewers
This covers a lot of pretty reasonable use cases for feature flags in the API. The primary one that isn't yet supported is updating the feature flag on a running system, but the beginnings of that can be seen in this implementation (the initialize + global configuration approach could eventually allow for us to modify that global value periodically at run-time without adjusting the usage elsewhere).
Additional information
You can see the header value working in the added test, but also can test it locally at localhost:8080/docs - a convenient dropdown is added for you.
