fix: optimize catalog permission sync when importing dashboards#33679
fix: optimize catalog permission sync when importing dashboards#33679betodealmeida merged 2 commits intoapache:masterfrom
Conversation
…queries, only sync permissions for the default catalog
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/commands/database/utils.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #33679 +/- ##
===========================================
+ Coverage 60.48% 83.04% +22.56%
===========================================
Files 1931 558 -1373
Lines 76236 40990 -35246
Branches 8568 0 -8568
===========================================
- Hits 46114 34042 -12072
+ Misses 28017 6948 -21069
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
betodealmeida
left a comment
There was a problem hiding this comment.
Awesome, thanks for fixing this!
| ssh_tunnel=ssh_tunnel, | ||
| ) | ||
| else: | ||
| catalogs = {database.get_default_catalog()} |
There was a problem hiding this comment.
I just realized we probably want to pass the ssh_tunnel to get_default_catalog, since for some DB engine specs we need to make a connection and run a query in order to figure out the default catalog (it can't be inferred from the SQLAlchemy URI). But we can do that in the future when we need it.
SUMMARY
Following up from #33000. The previous PR fixed catalog permission syncing for databases like Postgres, which don't support multiple catalogs in one instance. However, permissions were still being synced for all the catalogs when creating a database connection (e.g. during a dashboard import).
This PR applies the same logic as the previous fix to
CreateDatabaseCommand.Fixes #32993.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
Added unit tests.
ADDITIONAL INFORMATION