-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: Considers GENERIC_CHART_AXES in viz migrations #23905
feat: Considers GENERIC_CHART_AXES in viz migrations #23905
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23905 +/- ##
==========================================
- Coverage 68.08% 68.04% -0.04%
==========================================
Files 1939 1939
Lines 75007 75023 +16
Branches 8154 8154
==========================================
- Hits 51066 51049 -17
- Misses 21858 21891 +33
Partials 2083 2083
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
A few first pass comments
if not granularity_sqla or not time_range: | ||
return |
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.
Is this correct - shouldn't we create at least a "No filter" time filter as long as granularity_sqla
is defined? Or is time_range
in practice always defined?
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.
You're right. I changed it to use DEFAULT_TIME_FILTER
when there's no time_range
but there's a time column.
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 with one last safeguard/comment
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Should migrations be affected by feature flags? What happens when a user runs a migration and later turns the feature flag? |
Not generically. In this case, we're talking about the |
@betodealmeida I changed the PR's title to make it clear that it's only affecting viz migrations. |
SUMMARY
This PR changes the base migration class so that if
GENERIC_CHART_AXES
is enabled when migrating a chart, the temporal filter will be created for the new viz type. It also fixes an issue where renaming a key was not deleting the old key and ensures that theform_data
backup is a deep copy of the originalform_data
.ADDITIONAL INFORMATION