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

feat: feature flag configurable custom backend #16618

Merged
merged 9 commits into from
Sep 13, 2021

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Sep 7, 2021

SUMMARY

Add the possibility to override is_feature_enabled instead of get_feature_flags, the later when using a feature flag management system, would force all feature flags to be evaluated when evaluating a single feature.

IS_FEATURE_ENABLED_FUNC and GET_FEATURE_FLAGS_FUNC should be mutually exclusive (but not obligatory) has stated on the config comments, this is because get_feature_flags will call is_feature_enabled to make the API consistent and safe to get all feature flags and be sure that all gets applied on the override.

Introduces a new Config key:

IS_FEATURE_ENABLED_FUNC: Callable that overrides current is_feature_enabled

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@jfrag1

@dpgaspar dpgaspar changed the title feat: feature flag configurable custom backend [WiP] feat: feature flag configurable custom backend Sep 7, 2021
@dpgaspar dpgaspar marked this pull request as ready for review September 7, 2021 12:37
@codecov
Copy link

codecov bot commented Sep 7, 2021

Codecov Report

Merging #16618 (f4472aa) into master (788c0c3) will increase coverage by 0.07%.
The diff coverage is 88.65%.

❗ Current head f4472aa differs from pull request most recent head 1b9b4fe. Consider uploading reports for the commit 1b9b4fe to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16618      +/-   ##
==========================================
+ Coverage   76.76%   76.83%   +0.07%     
==========================================
  Files        1004     1005       +1     
  Lines       53967    54014      +47     
  Branches     7335     7337       +2     
==========================================
+ Hits        41430    41504      +74     
+ Misses      12298    12270      -28     
- Partials      239      240       +1     
Flag Coverage Δ
hive 81.24% <88.09%> (+0.08%) ⬆️
mysql 81.66% <88.09%> (+0.12%) ⬆️
postgres 81.62% <88.09%> (+0.06%) ⬆️
presto ?
python 82.05% <88.09%> (-0.06%) ⬇️
sqlite 81.33% <88.09%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...mponents/nativeFilters/FiltersConfigModal/utils.ts 72.22% <ø> (-1.39%) ⬇️
...nd/src/dashboard/components/nativeFilters/utils.ts 56.25% <ø> (ø)
superset/db_engine_specs/base.py 88.39% <ø> (ø)
superset/db_engine_specs/presto.py 84.51% <ø> (-5.44%) ⬇️
superset/reports/api.py 88.46% <ø> (+0.67%) ⬆️
superset/sql_lab.py 80.70% <ø> (ø)
superset/views/core.py 76.00% <81.48%> (+0.37%) ⬆️
superset-frontend/src/components/Select/Select.tsx 89.95% <88.88%> (+17.30%) ⬆️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 73.20% <100.00%> (+0.07%) ⬆️
...nd/src/dashboard/components/nativeFilters/types.ts 100.00% <100.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 788c0c3...1b9b4fe. Read the comment docs.

@jfrag1
Copy link
Member

jfrag1 commented Sep 7, 2021

I think this feature flag backend would also need to be able to evaluate all feature flags to send via bootstrap here. Otherwise, you'd still need to define a GET_FEATURE_FLAGS_FUNC to get the proper values to the frontend, which seems like it would defeat the purpose of having a custom backend.

Update:
Disregard, I see now that this can also be achieved with the COMMON_BOOTSTRAP_OVERRIDES_FUNC config. It may be good to make a note that it is required to do this to ensure flags are consistent between bootstrap and the custom backend

@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 8, 2021
@dpgaspar dpgaspar changed the title [WiP] feat: feature flag configurable custom backend feat: feature flag configurable custom backend Sep 9, 2021
@pull-request-size pull-request-size bot added size/M and removed size/L labels Sep 9, 2021
@dpgaspar
Copy link
Member Author

dpgaspar commented Sep 9, 2021

I think this feature flag backend would also need to be able to evaluate all feature flags to send via bootstrap here. Otherwise, you'd still need to define a GET_FEATURE_FLAGS_FUNC to get the proper values to the frontend, which seems like it would defeat the purpose of having a custom backend.

Update:
Disregard, I see now that this can also be achieved with the COMMON_BOOTSTRAP_OVERRIDES_FUNC config. It may be good to make a note that it is required to do this to ensure flags are consistent between bootstrap and the custom backend

Actually that's a good point, I made a refactor on this PR to directly override is_feature_enabled it's simpler and ended up being simpler to integrate on our side. No need to make the note on the config, since get_feature_flags maps all flags to is_feature_enabled when the override is set.

@dpgaspar dpgaspar requested review from betodealmeida, craig-rueda and villebro and removed request for betodealmeida September 9, 2021 11:26
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

LGTM

@jfrag1
Copy link
Member

jfrag1 commented Sep 9, 2021

LGTM

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM with a few miniscule nit suggestions 👍

superset/config.py Outdated Show resolved Hide resolved
superset/utils/feature_flag_manager.py Outdated Show resolved Hide resolved
dpgaspar and others added 2 commits September 10, 2021 18:32
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@dpgaspar dpgaspar merged commit f2bc139 into apache:master Sep 13, 2021
@dpgaspar dpgaspar deleted the feat/feature-flag-hook-alt branch September 13, 2021 13:09
stevenuray pushed a commit to preset-io/superset that referenced this pull request Sep 14, 2021
* feat: feature flag configurable custom backend

* fix lint

* simpler approach

* fix tests

* revert dependency updates

* Update superset/utils/feature_flag_manager.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* Update superset/config.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
(cherry picked from commit f2bc139)
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* feat: feature flag configurable custom backend

* fix lint

* simpler approach

* fix tests

* revert dependency updates

* Update superset/utils/feature_flag_manager.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* Update superset/config.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* feat: feature flag configurable custom backend

* fix lint

* simpler approach

* fix tests

* revert dependency updates

* Update superset/utils/feature_flag_manager.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

* Update superset/config.py

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>

Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/M 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants