-
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
bugfix: cli export_datasources (fixes 7437) #7728
bugfix: cli export_datasources (fixes 7437) #7728
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7728 +/- ##
=======================================
Coverage 65.77% 65.77%
=======================================
Files 459 459
Lines 21911 21911
Branches 2410 2410
=======================================
Hits 14411 14411
Misses 7379 7379
Partials 121 121
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.
I'm not super familiar with this code, but I would've expected a change to the import function if the export format was changing. Does this still work when you try to import the new exported file?
Actually I think the fix may be to not sort the list at all. I'm guessing the intention was to have a deterministic ordering so that aligns well in source control. I think the ordering of the keys is stable/predictable given that it follow the |
@mistercrunch I guessed that the sorting was for the same thoughtful reason - to reduce VCS thrash, and that's why I though a hacky string coercion would be a suitable fix. @etr2460 Since this affects only the ordering of the yml that is output by the function, there is no change required on the import code. |
What's the next step in the process to get this merged? |
Ok I resolved a conflict, let's move forward with this once CI passes |
Note that it fixes the bug but could also lead to random re-ordering when objects change. Maybe once #7472 lands, we can use the new |
@mistercrunch we have identified more problems with export/import and now consider fixes that we would love to send upstream. The current proposed solution is also to use uuids. You can find the document here: https://docs.google.com/document/d/1vfiAw7yCzqvb3ALo_aiBX3XFPjLNZ1Mira-X7HCK1lI/edit?usp=sharing |
Also related, found issues that I fixed here as bycatch #7773 in the |
Ping @mistercrunch what do we need to do to either get this merged or apply another fix? This 5 character PR has been open for 20 days now. What can I do to help? |
CATEGORY
Choose one
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
superset db upgrade
superset init
superset load_examples
superset export_datasources
and note the yml output.ADDITIONAL INFORMATION
REVIEWERS