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: Support feature flag overrides in ephemeral test envs #14008

Merged

Conversation

robdiciuccio
Copy link
Member

SUMMARY

Adds support for feature flag overrides in ephemeral test environments. Also adds support for setting feature flags via environment variables prefixed with SUPERSET_FEATURE_.

TEST PLAN

This functionality won't be available until it is merged to master. It has been tested here.

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

sweet!

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #14008 (23da118) into master (f3becbe) will decrease coverage by 0.56%.
The diff coverage is 83.95%.

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

@@            Coverage Diff             @@
##           master   #14008      +/-   ##
==========================================
- Coverage   78.64%   78.08%   -0.57%     
==========================================
  Files         935      944       +9     
  Lines       47375    48069     +694     
  Branches     5973     6146     +173     
==========================================
+ Hits        37260    37533     +273     
- Misses       9972    10388     +416     
- Partials      143      148       +5     
Flag Coverage Δ
hive 80.28% <85.45%> (+0.01%) ⬆️
mysql ?
postgres ?
presto 80.30% <85.45%> (+0.01%) ⬆️
python 80.79% <85.45%> (-0.36%) ⬇️
sqlite ?

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

Impacted Files Coverage Δ
...rset-frontend/src/components/ErrorMessage/types.ts 100.00% <ø> (ø)
...end/src/components/IndeterminateCheckbox/index.tsx 94.11% <ø> (ø)
...ntend/src/dashboard/components/CssEditor/index.jsx 70.83% <ø> (ø)
...src/dashboard/components/PropertiesModal/index.jsx 59.85% <ø> (ø)
...shboard/components/filterscope/FilterFieldItem.jsx 57.14% <ø> (+7.14%) ⬆️
...shboard/components/filterscope/FilterFieldTree.jsx 50.00% <ø> (+5.55%) ⬆️
...hboard/components/filterscope/FilterScopeModal.tsx 59.09% <ø> (-15.91%) ⬇️
...ard/components/filterscope/FilterScopeSelector.jsx 2.05% <ø> (ø)
...shboard/components/filterscope/FilterScopeTree.jsx 56.25% <ø> (+6.25%) ⬆️
...ponents/filterscope/renderFilterFieldTreeNodes.jsx 0.00% <ø> (ø)
... and 65 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 f3becbe...a0be83c. Read the comment docs.

Copy link
Contributor

@benjreinhart benjreinhart left a comment

Choose a reason for hiding this comment

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

Minor comment on the SUPERSET_ prefix but looks fine either way

let results = [];
[...context.payload.comment.body.matchAll(pattern)].forEach(match => {
const config = {
name: `SUPERSET_FEATURE_${match[1]}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to prefix these with SUPERSET_?

Copy link
Member Author

Choose a reason for hiding this comment

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

Namespacing and convention with other superset env vars.

{
k[len("SUPERSET_FEATURE_") :]: bool(strtobool(v))
for k, v in os.environ.items()
if re.search(r"^SUPERSET_FEATURE_\w+", k)
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit but startswith might be simpler here

Copy link
Member

@amitmiran137 amitmiran137 left a comment

Choose a reason for hiding this comment

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

this is so nice! PR review become super easy all of a sudden

@junlincc
Copy link
Member

junlincc commented Apr 8, 2021

Thank you so much Rob!!!!! ♥️ much appreciated!! @robdiciuccio
This will largely help enable community testing as well!

@srinify let's highlight this in our next release note :)

@robdiciuccio robdiciuccio merged commit 6a81a79 into apache:master Apr 8, 2021
@robdiciuccio robdiciuccio deleted the rd/ephemeral-env-feature-flags branch April 8, 2021 18:06
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
…4008)

* Add support for feature flag overrides in ephemeral env cmd

* update docs to reference correct config

* Update ephemeral env docs
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…4008)

* Add support for feature flag overrides in ephemeral env cmd

* update docs to reference correct config

* Update ephemeral env docs
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 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 release:note size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants