PIN-5931: added SH feature flag and whitelist #1369
Conversation
borgesis95
requested review from
beetlecrunch,
galales,
AsterITA,
Carminepo2 and
taglioni-r
as code owners
|
I'm also adding @sandrotaje and @MalpenZibo as reviewers since they joined the feature flag discussion |
| isSignalHubEnabled: checkSignalhubFeatureFlag( | ||
| authData.organizationId, | ||
| seed.isSignalHubEnabled | ||
| ), |
There was a problem hiding this comment.
What happens if we disable the feature flag on an environment (after its activation) or if we remove an organization from the whitelist? The service could maintain this flag to true.
We should decide if it's possible to disable a feature flag and what happen to the persisted data in that case
There was a problem hiding this comment.
on this topic. We should probably apply changes to models and persistent data in an agnostic way respect the feature flag (using optional and default values).
Maybe we could organize a meeting to decide on a common strategy
| @@ -16,8 +16,18 @@ const CatalogProcessConfig = CommonHTTPServiceConfig.and(ReadModelDbConfig) | |||
| .object({ | |||
| ESERVICE_DOCUMENTS_PATH: z.string(), | |||
| PRODUCER_ALLOWED_ORIGINS: z.string(), | |||
| FEATURE_FLAG_SIGNALHUB_WHITELIST: z | |||
There was a problem hiding this comment.
I suggest to create a new config in common for feature flags, so every service can use the same flag.
Plus, feature flags could be optional and default to false. In my mind the idea is to enable only the ones needed, when needed.
No strong opinions, let me know your thoughts
There was a problem hiding this comment.
I'm afraid to make the feature flag an optional value (with a default false) but I don't have an example of a situation where this behavior could cause problems.
I had to think about it
| authData.organizationId, | ||
| seed.isSignalHubEnabled | ||
| ) | ||
| : seed.isSignalHubEnabled, |
There was a problem hiding this comment.
Shouldn't the fallback be false?
There was a problem hiding this comment.
if FEATURE_FLAG_SIGNALHUB_WHITELIST = false it means that hasn't to check WHITELIST so that value will be what has been set from UI (seed.isSignalHubEnabled). Maybe could result a bit tricky due to name of isSignalhubEnabled variable that hasn't not to do with FEATURE_FLAG_SIGNALHUB
| FEATURE_FLAG_SIGNALHUB_WHITELIST: z | ||
| .enum(["true", "false"]) | ||
| .transform((value) => value === "true"), |
There was a problem hiding this comment.
| FEATURE_FLAG_SIGNALHUB_WHITELIST: z | |
| .enum(["true", "false"]) | |
| .transform((value) => value === "true"), | |
| FEATURE_FLAG_SIGNALHUB_WHITELIST: z | |
| .enum(["true", "false"]) | |
| .default("false") | |
| .transform((value) => value === "true"), |
As suggested by @galales we should set a default in case the env variable is not set and I also agree to define dedicated featureFlags configs, you could do it like this maybe by creating a new file featureFlags that we will use to define all the flags' configs
There was a problem hiding this comment.
Moved FEATURE_FLAG_SIGNALHUB_WHITELIST on dedicated file, however I let SIGNALHUB_WHITELIST var on catalog-process config file. I think that whitelist is closely related to catalog-process. Let me know what you think about it.
There was a problem hiding this comment.
Moved
FEATURE_FLAG_SIGNALHUB_WHITELISTon dedicated file, however I letSIGNALHUB_WHITELISTvar on catalog-process config file. I think that whitelist is closely related to catalog-process. Let me know what you think about it.
I think that it should be included inside the dedicated config since that variable is strictly related to FEATURE_FLAG_SIGNALHUB_WHITELIST, so these should be together IMHO.
Even if, for now, this config is used only in the catalog-process having all the featureFlags configs in commons would be easier for future development
There was a problem hiding this comment.
you've convinced me! moved var into feature flag config file 👍
This PR allow to add feature flag to enable SH checks and whitelist in order to give chance to selected organization to test signal-hub platform.