[feat] Feature flag system via config #5960
Conversation
xtinec
force-pushed
the
feature-flags
branch
from
2ee9bbf to
e1f77c7
Compare
Codecov Report
@@ Coverage Diff @@
## master #5960 +/- ##
==========================================
+ Coverage 63.52% 64.72% +1.19%
==========================================
Files 393 395 +2
Lines 23667 23678 +11
Branches 2638 2639 +1
==========================================
+ Hits 15034 15325 +291
+ Misses 8620 8340 -280
Partials 13 13
Continue to review full report at Codecov.
|
xtinec
force-pushed
the
feature-flags
branch
from
e1f77c7 to
ee3fbee
Compare
| } | ||
|
|
||
| // Feature flags are not altered throughout the life time of the app | ||
| export default function featureFlagsReducer(state = {}) { |
There was a problem hiding this comment.
do you think this is necessary if it's never modified?
There was a problem hiding this comment.
I think it feels odd to have a subtree of the redux state tree dangling without a reducer. This also leaves room for enriching the feature flag system in the future if we'd like (e.g. allowing toggling of a feature flag without restarting the server). Thoughts?
There was a problem hiding this comment.
Feature flag override would be interesting. Some dogfood apps has Chrome plugin that you can turn on/off experimental features. With that said I am OK with having empty reducer here for now.
| # Feature flags | ||
| # --------------------------------------------------- | ||
| # Feature flags that are on by default go here. Their | ||
| # values can be overridden by those in super_config.py |
There was a problem hiding this comment.
We'll need a place in the docs where we document every single feature flag as we add them. It doesn't have to be setup in this PR, but let's do it when we add in the first feature flag.
| // `isFeatureEnabled` function which takes a feature and returns whether it is enabled. | ||
| // Note that we assume the featureFlags subtree is at the root of the redux state tree. | ||
| export function isFeatureEnabledCreator(state) { | ||
| return feature => !!state.featureFlags[feature]; |
There was a problem hiding this comment.
This makes it clear to me that feature flag can only be boolean, which seems reasonable given the name. It prevents having more complex data structures like vizTypeBlacklist = ['heatmap', 'sunburst'] . I'm not necessarily questioning this, just pointing it out.
There was a problem hiding this comment.
hopefully vis type lists will come from the plugin system not this!
| @@ -0,0 +1,69 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
I think you didn't use git mv and git/github thinks you created a new file, let's keep the git history in place
There was a problem hiding this comment.
Good catching. Fixing it.
| @@ -1,38 +0,0 @@ | |||
| // this test must be commented out because ChartContainer is now importing files | |||
There was a problem hiding this comment.
I'm confused as to why this was commented out to start with (?)
There was a problem hiding this comment.
Looks like it predates the history on github
https://github.com/apache/incubator-superset/commits/master/superset/assets/spec/javascripts/explore/components/ExploreViewContainer_spec.js
Guess we'll never know... 🤔
Adding a feature flag system that is driven by superset_config.py. This change includes:
- Server side changes to specify a dedicated FEATURE_FLAG dictionary for listing feature flags. E.g.
```
FEATURE_FLAGS = { 'SCOPED_FILTER': true }
```
- Pass the new feature flags to client via bootstrap-data
- Client side changes to inject feature flags into the redux state tree for dashboard, explore view and SqlLab
- Client side refactor/clean up so the feature flags can be properly tested. Also avoid modifying incoming bootstrap data when creating initial state for the redux state tree
- Re-enable tests that were previously disabled for ExploreViewContainer
…so we don't have to write ../../../src and such in tests). This will in a separate PR.
xtinec
force-pushed
the
feature-flags
branch
from
ac2bc05 to
b5b812c
Compare
|
@mistercrunch @williaster does this look good to merge? 🙏 |
* [feat] Feature flag system via config
Adding a feature flag system that is driven by superset_config.py. This change includes:
- Server side changes to specify a dedicated FEATURE_FLAG dictionary for listing feature flags. E.g.
```
FEATURE_FLAGS = { 'SCOPED_FILTER': true }
```
- Pass the new feature flags to client via bootstrap-data
- Client side changes to inject feature flags into the redux state tree for dashboard, explore view and SqlLab
- Client side refactor/clean up so the feature flags can be properly tested. Also avoid modifying incoming bootstrap data when creating initial state for the redux state tree
- Re-enable tests that were previously disabled for ExploreViewContainer
* Fix lint errors.
* Remove the partial attempt to get reference to src working in tests (so we don't have to write ../../../src and such in tests). This will in a separate PR.
mistercrunch
added
🏷️ bot
Adding a feature flag system that is driven by superset_config.py. This change includes:
@mistercrunch @kristw @williaster