create-uber-principal fixes and improvements
#2941
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JCZuurmond
commented
Oct 14, 2024
| @@ -13,29 +13,31 @@ | |||
| StorageAccount, | |||
| ) | |||
|
|
|||
| from . import azure_api_client, get_az_api_mapping | |||
| from . import azure_api_client as create_azure_api_client, get_az_api_mapping | |||
There was a problem hiding this comment.
I needed to update the API client in the test_access.py, was on the way to make it a fixture but decided to leave the remainder for #2949 as it became too many changes
JCZuurmond
commented
Oct 14, 2024
| @pytest.fixture | ||
| def clean_warehouse_config(env_or_skip, az_cli_ctx) -> Generator[None, None, None]: | ||
| """Clean workspace warehouse configuration.""" | ||
| env_or_skip("IDE_PROJECT_ROOTS") # Only run from editor |
There was a problem hiding this comment.
What do we think about only running this in the editor not in the CI?
- Pro: The failing
test_create_global_spnwent unnoticed because it is not ran in the CI. To be more precise: I cannot run that integration test as I miss permissions to change Azure resources, I cannot confirm it fails, but confirmed the logic fails when running the UCX cli. This put me on the wrong track because I assumed the test was passing, letting me to think the issue solved by this PR was due to a context different then our testing environment. Running the integration test more often will make issues visible quicker - Con: we are changing a global config that will interfere with other tests when they run in parallel. Especially now the tests uses dummy resources thus blocking the warehouse to start for a moment as the config is invalid
JCZuurmond
had a problem deploying
to
account-admin
GitHub Actions
Error
JCZuurmond
force-pushed
the
fix/improve-create-uber-principal
branch
from
4ede2e7 to
5d65f9b
Compare
JCZuurmond
temporarily deployed
to
account-admin
GitHub Actions
Inactive
|
✅ 6/6 passed, 14 skipped, 1m13s total Running from acceptance #6689 |
nfx
added a commit
that referenced
this pull request
Oct 14, 2024
* Added `imbalanced-learn` to known list ([#2943](#2943)). A new open-source library, "imbalanced-learn," has been added to the project's known list of libraries, providing various functionalities for handling imbalanced datasets. The addition includes modules such as "imblearn", "imblearn._config", "imblearn._min_dependencies", "imblearn._version", "imblearn.base", and many others, enabling features such as over-sampling, under-sampling, combining sampling techniques, and creating ensembles. This change partially resolves issue [#1931](#1931), which may have been related to the handling of imbalanced datasets, thereby enhancing the project's ability to manage such datasets. * Added `importlib_resources` to known list ([#2944](#2944)). In this update, we've added the `importlib_resources` package to the known list in the `known.json` file. This package offers a consistent and straightforward interface for accessing resources such as data files and directories in Python packages. It includes several modules, including `importlib_resources`, `importlib_resources._adapters`, `importlib_resources._common`, `importlib_resources._functional`, `importlib_resources._itertools`, `importlib_resources.abc`, `importlib_resources.compat`, `importlib_resources.compat.py38`, `importlib_resources.compat.py39`, `importlib_resources.future`, `importlib_resources.future.adapters`, `importlib_resources.readers`, and `importlib_resources.simple`. These modules provide various functionalities for handling resources within a Python package. By adding this package to the known list, we enable its usage and integration with the project's codebase. This change partially addresses issue [#1931](#1931), improving the management and accessibility of resources within our Python packages. * Dependency update: ensure we install with at least version 0.9.1 of `databricks-labs-blueprint` ([#2950](#2950)). In the updated `pyproject.toml` file, the version constraint for the `databricks-labs-blueprint` dependency has been revised to range between 0.9.1 and 0.10, specifically targeting 0.9.1 or higher. This modification ensures the incorporation of a fixed upstream issue (databrickslabs/blueprint[#157](#157)), which was integrated in the 0.9.1 release. This adjustment was triggered by a preceding change ([#2920](#2920)) that standardized notebook paths, thereby addressing issue [#2882](#2882), which was dependent on this upstream correction. By embracing this upgrade, users can engage the most recent dependency version, thereby ensuring the remediation of the aforementioned issue. * Fixed an issue with source table deleted after migration ([#2927](#2927)). In this release, we have addressed an issue where a source table was marked as migrated even after it was deleted following migration. An exception handling mechanism has been added to the `is_migrated` method to return `True` and log a warning message if the source table does not exist, indicating that it has been migrated. A new test function, `test_migration_index_deleted_source`, has also been included to verify the migration index behavior when the source table no longer exists. This function creates a source and destination table, sets the destination table's `upgraded_from` property to the source table, drops the source table, and checks if the migration index contains the source table and if an error message was recorded, indicating that the source table no longer exists. The `get_seen_tables` method remains unchanged in this diff. * Improve robustness of `sqlglot` failure handling ([#2952](#2952)). This PR introduces changes to improve the robustness of error handling in the `sqlglot` library, specifically targeting issues with inadequate parsing quality. The `collect_table_infos` method has been updated and renamed to `collect_used_tables` to accurately gather information about tables used in a SQL expression. The `lint_expression` and `collect_tables` methods have also been updated to use the new `collect_used_tables` method for better accuracy. Additionally, methods such as `find_all`, `walk_expressions`, and the test suite for the SQL parser have been enhanced to handle potential failures and unsupported SQL syntax more gracefully, by returning empty lists or logging warning messages instead of raising errors. These changes aim to improve the reliability and robustness of the `sqlglot` library, enabling it to handle unexpected input more effectively. * Log warnings when mounts are discovered on incorrect cluster type ([#2929](#2929)). The `migrate-tables` command in the ucx project's CLI now includes a verification step to ensure the successful completion of a prerequisite assessment workflow before execution. If this workflow has not been completed, a warning message is logged and the command is not executed. A new exception handling mechanism has been implemented for the `dbutils.fs.mounts()` method, which logs a warning and skips mount point discovery if an exception is raised. A new unit test has been added to verify that a warning is logged when attempting to discover mounts on an incompatible cluster type. The diff also includes a new method `VerifyProgressTracking` for verifying progress tracking and updates to existing test methods to include verification of successful runs and error handling before assessment. These changes improve the handling of edge cases in the mount point discovery process, add warnings for mounts on incorrect cluster types, and increase test coverage with progress tracking verification. * `create-uber-principal` fixes and improvements ([#2941](#2941)). This change introduces fixes and improvements to the `create-uber-principal` functionality within the `databricks-sdk-py` project, specifically targeting the Azure access module. The main enhancements include addressing an issue with the Databricks warehouses API by adding the `set_workspace_warehouse_config_wrapper` function, modifying the command to request the uber principal name only when necessary, improving storage account crawl logic, and introducing new methods to manage workspace-level configurations. Error handling mechanisms have been fortified through added and modified try-except blocks. Additionally, several unit and integration tests have been implemented and verified to ensure the functionality is correct and running smoothly. These changes improve the overall robustness and versatility of the `create-uber-principal` command, directly addressing issues [#2764](#2764), [#2771](#2771), and progressing on [#2949](#2949).
Merged
nfx
added a commit
that referenced
this pull request
Oct 14, 2024
* Added `imbalanced-learn` to known list ([#2943](#2943)). A new open-source library, "imbalanced-learn," has been added to the project's known list of libraries, providing various functionalities for handling imbalanced datasets. The addition includes modules such as "imblearn", "imblearn._config", "imblearn._min_dependencies", "imblearn._version", "imblearn.base", and many others, enabling features such as over-sampling, under-sampling, combining sampling techniques, and creating ensembles. This change partially resolves issue [#1931](#1931), which may have been related to the handling of imbalanced datasets, thereby enhancing the project's ability to manage such datasets. * Added `importlib_resources` to known list ([#2944](#2944)). In this update, we've added the `importlib_resources` package to the known list in the `known.json` file. This package offers a consistent and straightforward interface for accessing resources such as data files and directories in Python packages. It includes several modules, including `importlib_resources`, `importlib_resources._adapters`, `importlib_resources._common`, `importlib_resources._functional`, `importlib_resources._itertools`, `importlib_resources.abc`, `importlib_resources.compat`, `importlib_resources.compat.py38`, `importlib_resources.compat.py39`, `importlib_resources.future`, `importlib_resources.future.adapters`, `importlib_resources.readers`, and `importlib_resources.simple`. These modules provide various functionalities for handling resources within a Python package. By adding this package to the known list, we enable its usage and integration with the project's codebase. This change partially addresses issue [#1931](#1931), improving the management and accessibility of resources within our Python packages. * Dependency update: ensure we install with at least version 0.9.1 of `databricks-labs-blueprint` ([#2950](#2950)). In the updated `pyproject.toml` file, the version constraint for the `databricks-labs-blueprint` dependency has been revised to range between 0.9.1 and 0.10, specifically targeting 0.9.1 or higher. This modification ensures the incorporation of a fixed upstream issue (databrickslabs/blueprint[#157](#157)), which was integrated in the 0.9.1 release. This adjustment was triggered by a preceding change ([#2920](#2920)) that standardized notebook paths, thereby addressing issue [#2882](#2882), which was dependent on this upstream correction. By embracing this upgrade, users can engage the most recent dependency version, thereby ensuring the remediation of the aforementioned issue. * Fixed an issue with source table deleted after migration ([#2927](#2927)). In this release, we have addressed an issue where a source table was marked as migrated even after it was deleted following migration. An exception handling mechanism has been added to the `is_migrated` method to return `True` and log a warning message if the source table does not exist, indicating that it has been migrated. A new test function, `test_migration_index_deleted_source`, has also been included to verify the migration index behavior when the source table no longer exists. This function creates a source and destination table, sets the destination table's `upgraded_from` property to the source table, drops the source table, and checks if the migration index contains the source table and if an error message was recorded, indicating that the source table no longer exists. The `get_seen_tables` method remains unchanged in this diff. * Improve robustness of `sqlglot` failure handling ([#2952](#2952)). This PR introduces changes to improve the robustness of error handling in the `sqlglot` library, specifically targeting issues with inadequate parsing quality. The `collect_table_infos` method has been updated and renamed to `collect_used_tables` to accurately gather information about tables used in a SQL expression. The `lint_expression` and `collect_tables` methods have also been updated to use the new `collect_used_tables` method for better accuracy. Additionally, methods such as `find_all`, `walk_expressions`, and the test suite for the SQL parser have been enhanced to handle potential failures and unsupported SQL syntax more gracefully, by returning empty lists or logging warning messages instead of raising errors. These changes aim to improve the reliability and robustness of the `sqlglot` library, enabling it to handle unexpected input more effectively. * Log warnings when mounts are discovered on incorrect cluster type ([#2929](#2929)). The `migrate-tables` command in the ucx project's CLI now includes a verification step to ensure the successful completion of a prerequisite assessment workflow before execution. If this workflow has not been completed, a warning message is logged and the command is not executed. A new exception handling mechanism has been implemented for the `dbutils.fs.mounts()` method, which logs a warning and skips mount point discovery if an exception is raised. A new unit test has been added to verify that a warning is logged when attempting to discover mounts on an incompatible cluster type. The diff also includes a new method `VerifyProgressTracking` for verifying progress tracking and updates to existing test methods to include verification of successful runs and error handling before assessment. These changes improve the handling of edge cases in the mount point discovery process, add warnings for mounts on incorrect cluster types, and increase test coverage with progress tracking verification. * `create-uber-principal` fixes and improvements ([#2941](#2941)). This change introduces fixes and improvements to the `create-uber-principal` functionality within the `databricks-sdk-py` project, specifically targeting the Azure access module. The main enhancements include addressing an issue with the Databricks warehouses API by adding the `set_workspace_warehouse_config_wrapper` function, modifying the command to request the uber principal name only when necessary, improving storage account crawl logic, and introducing new methods to manage workspace-level configurations. Error handling mechanisms have been fortified through added and modified try-except blocks. Additionally, several unit and integration tests have been implemented and verified to ensure the functionality is correct and running smoothly. These changes improve the overall robustness and versatility of the `create-uber-principal` command, directly addressing issues [#2764](#2764), [#2771](#2771), and progressing on [#2949](#2949).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
create-uber-principalfixes and improvements:set_workspace_warehouse_config_wrapperin Databricks warehouses API.Linked issues
Resolves #2764
Resolves #2771
Progresses #2949
Reference databricks/databricks-sdk-py#305
Functionality
databricks labs ucx create-uber-principalTests