-
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
refactor: [migration] convert iframe chart into dashboard markdown component #10590
refactor: [migration] convert iframe chart into dashboard markdown component #10590
Conversation
537b5b3
to
baf5a09
Compare
ca6b05f
to
78a0816
Compare
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 couple initial comments, i need to reread the getInitialState
changes again when i get more time though (I think it's mostly indentation changes, but confirmation would be helpful).
Overall, this looks awesome! I'm so happy to get rid of the iframe viz!
@@ -2040,25 +2040,6 @@ def get_data(self, df: pd.DataFrame) -> VizData: | |||
return d | |||
|
|||
|
|||
class IFrameViz(BaseViz): |
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.
Do we need to remove markdown components from viz.py too?
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.
yes i think i need to.
Actually they were removed in another PR: #9997
@@ -0,0 +1,38 @@ | |||
# Licensed to the Apache Software Foundation (ASF) under one |
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.
Looks like this can be removed? I assume it was a placeholder for the migration I just ran
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 create this migration yesterday, but today you merge a new migration script. so both of us are based from same root. this file is to resolve the sequence problem for alembic. otherwise i have to re-create the migration script.
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 think you could copy paste your migration into a new migration script and then not need this blank file. Sorry for the migration conflict!
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.
use superset db merge heads
will generate migration script and fix the conflicts. no need to modify original file. There are many empty migration script in the codebase which was created for this purpose.
).delete(synchronize_session=False) | ||
|
||
except Exception as ex: | ||
logging.exception(f"dashboard {dashboard.id} has error: {ex}") |
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.
What state would we end in if only part of the iframes got deleted? I assume the charts would break because we're removing the backend api. Maybe it's not an issue though if the chance of an exception here is quite small
|
||
|
||
def downgrade(): | ||
pass |
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 agree this is nearly impossible to do a downgrade for. Maybe add a comment explaining why though?
@@ -29,9 +29,7 @@ import ForceDirectedChartPlugin from '@superset-ui/legacy-plugin-chart-force-dir | |||
import HeatmapChartPlugin from '@superset-ui/legacy-plugin-chart-heatmap'; | |||
import HistogramChartPlugin from '@superset-ui/legacy-plugin-chart-histogram'; | |||
import HorizonChartPlugin from '@superset-ui/legacy-plugin-chart-horizon'; | |||
import IframeChartPlugin from '@superset-ui/legacy-plugin-chart-iframe'; |
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.
oooo, we can delete these packages next! So much red code!
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.
correct. need another PR in superset-ui codebase (after everything looks good in superset).
# find iframe chart position in metadata | ||
# and replace it with markdown component | ||
position_dict = json.loads(dashboard.position_json or "{}") | ||
for key, value in position_dict.items(): |
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.
nit: replace value
with chart_position
(or perhaps position_value
as it's more clear
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 like chart_position
. fixed.
dashboards = ( | ||
session.query(Dashboard).filter(Dashboard.id.in_(dashboard_ids)).all() | ||
) | ||
for i, dashboard in enumerate(dashboards): |
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.
we should probably add to UPDATING.md that this should be done during off hours as it could break people's charts while they're editing. They might save over the migrated dash and accidentally add an iframe chart back into it
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.
fixed.
78a0816
to
a9cf02e
Compare
).all() | ||
slices_ids = [slc.id for slc in slices_to_remove] | ||
|
||
# remove dependencies first |
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 tested in dev-box use production data.
It seems I have to remove dependencies first, where those table use slice_id as foreign key.
modified: slice.modified, | ||
changed_on: new Date(slice.changed_on).getTime(), | ||
}; | ||
const form_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.
correct. the only change is just removed if viz_type change. I didn't touch any other code.
896900b
to
47e3cba
Compare
47e3cba
to
9da43ac
Compare
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.
apart from the empty migration that i think you can get rid of, this lgtm!
bc59ac3
to
9de1d87
Compare
Codecov Report
@@ Coverage Diff @@
## master #10590 +/- ##
==========================================
- Coverage 64.37% 59.83% -4.54%
==========================================
Files 775 777 +2
Lines 36555 36619 +64
Branches 3459 3456 -3
==========================================
- Hits 23531 21910 -1621
- Misses 12912 14519 +1607
- Partials 112 190 +78
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
With 0.37 this is no long possible as I understand. ERROR: Separator
ERROR: markup
|
@stevensuting Superset will not support |
…mponent (apache#10590) * refactor: [migration] convert iframe chart into dashboard markdown component * remove 3 viz_types * fix comments
…mponent (apache#10590) * refactor: [migration] convert iframe chart into dashboard markdown component * remove 3 viz_types * fix comments
SUMMARY
Dashboard v2 support native markdown component, and user can also create iframe inside markdown. So that it's not necessary to have iframe/markup/separator viz_type in slices (or add them into dashboard).
This PR will do:
Markdown
component. Slices inmarkup
andseparator
viz_type already converted into dashboard component when dashboard v2 rollout.TEST PLAN
Before migration, create a iframe chart and add into dashboard.
After migration, check the dashboard, iframe chart should be converted into Markdown component with a iframe.
ADDITIONAL INFORMATION
@mistercrunch @etr2460 @john-bodley @serenajiang