-
Notifications
You must be signed in to change notification settings - Fork 92
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
[WIP] add command for migrating views #1232
[WIP] add command for migrating views #1232
Conversation
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.
Hi @ericvergnaud could you take a look at and double check this issue #1273? I think the old _migrate_view_table
function is creating UC view with old HMS dependencies.
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.
Your changes LGTM. However, there was a problem already that should be addressed first, see the comment above.
cluster_locations = {} | ||
if ws.config.is_azure: | ||
locations = ExternalLocations(ws, sql_backend, cfg.inventory_database) | ||
azure_client = AzureAPIClient( |
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.
See message in Slack from @nfx about these calls only existing from the cli not within a job
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1232 +/- ##
==========================================
+ Coverage 90.02% 90.19% +0.16%
==========================================
Files 62 62
Lines 7430 7709 +279
Branches 1335 1387 +52
==========================================
+ Hits 6689 6953 +264
- Misses 470 483 +13
- Partials 271 273 +2 ☔ View full report in Codecov by Sentry. |
closing in favor of #1325 |
Changes
add a command for migrating views
factorize code
Linked issues
Resolves #1172
Functionality
databricks labs ucx ...
...
...
Tests
Missing integration tests
Depends on #1177