-
Notifications
You must be signed in to change notification settings - Fork 14k
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: migrate charts on import #24703
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24703 +/- ##
==========================================
- Coverage 68.97% 68.95% -0.03%
==========================================
Files 1901 1902 +1
Lines 74006 73929 -77
Branches 8182 8175 -7
==========================================
- Hits 51045 50975 -70
- Misses 20840 20841 +1
+ Partials 2121 2113 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Ping @michael-s-molina, care to take another look? 🙇🏼 |
@@ -31,6 +32,12 @@ | |||
Base = declarative_base() | |||
|
|||
|
|||
class ChartMigratorStatus(Enum): |
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's only the Area migration that is incomplete. I don't think it's a good idea to create this enum because we don't want to allow incomplete or not implemented migrations. Can we assume that all new migrations should be complete and just skip area for now when importing?
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.
Sure, I just worried that in the future someone's going to fix the area migration but will not change the import logic. I'll leave a comment in the area migrator.
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.
@betodealmeida I'm already working on the Area migration in #23870. Adding a comment to the code sounds great!
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. Thank you for the PR @betodealmeida and for addressing all comments!
(cherry picked from commit abb8e28)
SUMMARY
When importing a chart, migrate it to a new visualization type if possible.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
A legacy area chart:
When exported and imported into Superset running this branch we get a new echarts area chart:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION