-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix: feature flags typing #15254
fix: feature flags typing #15254
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15254 +/- ##
==========================================
- Coverage 76.94% 76.74% -0.21%
==========================================
Files 1042 1043 +1
Lines 56254 56371 +117
Branches 7785 7794 +9
==========================================
- Hits 43286 43260 -26
- Misses 12710 12855 +145
+ Partials 258 256 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I contributed some code to this PR, so leaving this to be stamped by @betodealmeida
@@ -2414,7 +2413,7 @@ def validate_sql_json( | |||
) | |||
|
|||
spec = mydb.db_engine_spec | |||
validators_by_engine = get_feature_flags().get("SQL_VALIDATORS_BY_ENGINE") | |||
validators_by_engine = app.config["SQL_VALIDATORS_BY_ENGINE"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that SQL validation has never worked based on the SQL_VALIDATORS_BY_ENGINE
top level config parameter, as this was referencing the feature flag map.
@@ -392,8 +397,7 @@ class SqlEditor extends React.PureComponent { | |||
canValidateQuery() { | |||
// Check whether or not we can validate the current query based on whether | |||
// or not the backend has a validator configured for it. | |||
const validatorMap = window.featureFlags.SQL_VALIDATORS_BY_ENGINE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - instead of checking the featureFlags
map, we should check bootstrap data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We also have SCHEDULED_QUERIES
which is non-boolean:
const jsonSchema = window.featureFlags.SCHEDULED_QUERIES?.JSONSCHEMA; |
const getUISchema = () => window.featureFlags.SCHEDULED_QUERIES?.UISCHEMA; |
Thanks @betodealmeida , let's get that one fixed, too! |
const appContainer = document.getElementById('app'); | ||
const bootstrapData = JSON.parse( | ||
appContainer.getAttribute('data-bootstrap') || '{}', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to move this boilerplate into a util, but I'd prefer to do that in a follow up
@dpgaspar Hi, can you clarify in UPDATING that this turns on |
* fix: feature flags typing * fix tests * add note in UPDATING.md * fix frontend * also move SCHEDULED_QUERIES to top level * fix test Co-authored-by: Ville Brofeldt <[email protected]>
SUMMARY
Adds type annotations to feature flags, clearly stating that all feature flags are booleans. Because of that
QUERY_COST_FORMATTERS_BY_ENGINE
,SCHEDULED_QUERIES
andSQL_VALIDATORS_BY_ENGINE
are no longer feature flags but config keys. Note that currently on master,SQL_VALIDATORS_BY_ENGINE
is defined as a top level key onconfig.py
, but is erroneously being read from the feature flag object on the frontend. This fixes the issue and adds a note toUPDATING.md
.Testing instructions
ADDITIONAL INFORMATION