fix: Add impersonate_user setting to import/export datasource #14718
Conversation
itay
changed the title
|
@itay sorry for the long delay, but would you be able to rebase this PR to resolve the conflict? The fix looks legit but just got buried under other PRs 🙁 |
|
@villebro done - hope it helps, would be great to have it merged. |
|
@itay LGTM, but it appears you need to update the failing tests |
When exporting or importing a datasource/DB, it is necessary get the impersonate_user setting, as it is a key part of the configuration of that datasource. This unfortunately requires a code change, as this list of fields is whitelisted today.
|
@villebro let's see if this update resolves it |
Codecov Report
@@ Coverage Diff @@
## master #14718 +/- ##
==========================================
+ Coverage 76.75% 77.01% +0.26%
==========================================
Files 1043 1047 +4
Lines 56376 56565 +189
Branches 7797 7797
==========================================
+ Hits 43269 43565 +296
+ Misses 12851 12744 -107
Partials 256 256
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.
Sorry, I completely missed this when @junlincc tagged me!
There's a few more things that we need to test before merging this. Exports are now created with an additional field, and your test captures that. But we also need to test that:
- Can these files be imported correctly? Is the new field used when the database is imported, or is it ignored?
- Can old exports, without the field, be imported correctly? For old exports it's fine to assume that
impersonate_userisFalseif it's not present, since that's the default. But they should be imported without errors.
|
@betodealmeida thanks for the feedback. The requirements for getting the change in are getting bigger than I have time to put in at the moment - I originally needed this change back in May and have a mini-fork of Superset with just this change, and wanted to contribute it upstream. However, given 6 months have passed, I don't quite have the time to understand the test structure and add the changes right now. I'm happy if someone else wants to take over this change, or I can try and add the tests as I get some time to do it, but it likely won't be soon. |
I totally understand, and I know how this is frustrating, so my apologies for the delay. Either me or @villebro can take a stab at the tests. |
|
No problem - open source maintenance is hard especially for a project as successful as Superset, so I know how difficult it can be to get to the PRs, and I appreciate the response and engagement from the team. |
|
I'm happy to push a few tests to this PR if needed! Again, thanks so much for you patience and understanding! |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
When exporting or importing a datasource/DB, it is necessary get the impersonate_user setting, as it is a key part of the configuration of that datasource. This unfortunately requires a code change, as this list of fields is whitelisted today.
SUMMARY
The UI lets a user control whether a particular datasource supports impersonation, which is very helpful when creating it interactively. However, when exporting the datasource or importing it programmatically, the value for
impersonate_useris not exported/imported, leading to a gap in functionality.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
ADDITIONAL INFORMATION