Soft Deletion Support CLI #9424
Conversation
- Implemented 12 soft delete commands (list, show, delete/purge, recover for accounts, databases, and collections) - Added soft delete configuration parameters to cosmosdb create/update commands - Added comprehensive test coverage with 6 test methods - Optimized test execution by removing unnecessary sleep statements - Added linter exclusions for framework-generated parameters - Updated HISTORY.rst and bumped version to 1.7.0
…letion - Updated _params.py to use enable_soft_deletion parameter name - Updated custom.py function signatures and implementations - Updated _help.py examples to use --enable-soft-deletion - Updated test file to use correct CLI option - Removed unnecessary linter exclusions for soft delete parameters - Kept required linter exclusions for list commands (no_ids_for_list_commands)
|
Validation for Breaking Change Starting...
Thanks for your contribution! |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
| rule_exclusions: | ||
| - require_wait_command_if_no_wait | ||
|
|
||
| cosmosdb sql softdeleted-database list: |
There was a problem hiding this comment.
i dont know why but i kept getting these errors, so i had to exclude
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive soft-deletion support for Azure Cosmos DB SQL API, enabling users to list, view, recover, and permanently delete (purge) soft-deleted accounts, databases, and collections. The extension version is bumped from 1.6.1 to 1.7.0.
- Introduces three new command groups:
az cosmosdb sql softdeleted-account,az cosmosdb sql softdeleted-database, andaz cosmosdb sql softdeleted-collection - Adds soft-delete configuration parameters to account creation and update operations
- Includes comprehensive integration tests covering recovery and purge scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Bumps extension version to 1.7.0 |
| test_cosmosdb_softdelete_scenario.py | Adds comprehensive integration tests for soft-delete operations across accounts, databases, and collections |
| custom.py | Implements soft-delete configuration for account operations and CLI functions for managing soft-deleted resources |
| commands.py | Registers new command groups for soft-deleted resource management |
| _params.py | Defines parameters for soft-delete commands and account configuration options |
| _help.py | Provides help documentation and examples for new soft-delete commands |
| _client_factory.py | Adds client factories for soft-deleted resource operations |
| HISTORY.rst | Documents the new soft-delete feature in version 1.7.0 release notes |
| linter_exclusions.yml | Adds linter rule exclusions for soft-deleted list commands |
| self.cmd('az cosmosdb delete -n {acc} -g {rg} --yes') | ||
|
|
||
| logger.info("Waiting for account deletion to complete") | ||
| time.sleep(120) |
There was a problem hiding this comment.
[nitpick] Hard-coded sleep times of 60-120 seconds are used throughout the tests. While this may be necessary for integration tests waiting for Azure operations, consider documenting why these specific wait times are needed or using configurable timeout values. Additionally, consider adding timeout logic to avoid indefinite waits if operations fail.
| try: | ||
| self.cmd( | ||
| 'az cosmosdb sql softdeleted-database delete ' | ||
| '--location {loc} --account-name {acc} --name {db_name} -g {rg} --yes' | ||
| ) | ||
|
|
||
| time.sleep(60) | ||
|
|
||
| logger.info("Database successfully purged") | ||
|
|
||
| soft_deleted_dbs = self.cmd( | ||
| 'az cosmosdb sql softdeleted-database list ' | ||
| '--location {loc} --account-name {acc} -g {rg}' | ||
| ).get_output_in_json() | ||
|
|
||
| purged_db_found = any(db.get('name') == db_name for db in soft_deleted_dbs) | ||
| assert not purged_db_found, "Database should not appear in soft-deleted list after purge" | ||
|
|
||
| except Exception as e: | ||
| logger.warning(f"Database purge test skipped or failed: {e}") |
There was a problem hiding this comment.
The purge test for databases includes a try-except block that logs a warning and continues if the operation fails (line 398-399), while other similar purge operations (e.g., account purge at line 156, collection purge at line 463) don't have this exception handling. This inconsistency suggests uncertainty about the operation's reliability. Either add similar exception handling to all purge operations or remove it if the operation is expected to work reliably.
| short-summary: List all soft-deleted Azure Cosmos DB accounts in a subscription. | ||
| examples: | ||
| - name: List all soft-deleted Azure Cosmos DB accounts in a subscription. |
There was a problem hiding this comment.
The help text states "List all soft-deleted Azure Cosmos DB accounts in a subscription" but the command requires a resource group parameter, which limits the scope to a resource group, not the entire subscription. The help text should accurately describe the scope as "List all soft-deleted Azure Cosmos DB accounts in a resource group" or make the resource group parameter optional if subscription-wide listing is intended.
| short-summary: List all soft-deleted Azure Cosmos DB accounts in a subscription. | |
| examples: | |
| - name: List all soft-deleted Azure Cosmos DB accounts in a subscription. | |
| short-summary: List all soft-deleted Azure Cosmos DB accounts in a resource group. | |
| examples: | |
| - name: List all soft-deleted Azure Cosmos DB accounts in a resource group. |
| with self.argument_context('cosmosdb update') as c: | ||
| c.argument('key_uri', help="The URI of the key vault", is_preview=True) | ||
| c.argument('enable_soft_deletion', arg_type=get_three_state_flag(), help="Flag to enable or disable soft deletion on the account.", is_preview=True, arg_group='Soft Delete') | ||
| c.argument('soft_deletion_retention_period_in_minutes', options_list=['--soft-deletion-retention-period-in-minutes', '--sd-retention'], type=int, help="Soft deletion retention period in minutes. Must be at least equal to min_minutes_before_permanent_deletion_allowed.", is_preview=True, arg_group='Soft Delete') |
There was a problem hiding this comment.
The help text for soft_deletion_retention_period_in_minutes states "Must be at least equal to min_minutes_before_permanent_deletion_allowed" but uses the internal parameter name. For better user experience, it should reference the user-facing option name: "Must be at least equal to --min-purge-minutes".
| c.argument('soft_deletion_retention_period_in_minutes', options_list=['--soft-deletion-retention-period-in-minutes', '--sd-retention'], type=int, help="Soft deletion retention period in minutes. Must be at least equal to min_minutes_before_permanent_deletion_allowed.", is_preview=True, arg_group='Soft Delete') | |
| c.argument('soft_deletion_retention_period_in_minutes', options_list=['--soft-deletion-retention-period-in-minutes', '--sd-retention'], type=int, help="Soft deletion retention period in minutes. Must be at least equal to --min-purge-minutes.", is_preview=True, arg_group='Soft Delete') |
| import os | ||
| import unittest | ||
| import time | ||
| import datetime |
There was a problem hiding this comment.
[nitpick] The datetime module is imported on line 9 but doesn't appear to be used anywhere in the test file. Consider removing this unused import to keep the code clean.
| import datetime |
| min_minutes_before_permanent_deletion_allowed is not None: | ||
| from azext_cosmosdb_preview.vendored_sdks.azure_mgmt_cosmosdb.models import SoftDeleteConfiguration | ||
| soft_delete_configuration = SoftDeleteConfiguration( | ||
| enable_soft_deletion=enable_soft_deletion, |
There was a problem hiding this comment.
Inconsistent parameter names used when creating SoftDeleteConfiguration. In cli_cosmosdb_update (line 1034), it uses enable_soft_deletion=enable_soft_deletion, while in _create_database_account (line 1307), it uses enabled_soft_deletion=enable_soft_deletion. These should use the same parameter name. Please verify the correct parameter name for the SoftDeleteConfiguration model and use it consistently in both functions.
| enable_soft_deletion=enable_soft_deletion, | |
| enabled_soft_deletion=enable_soft_deletion, |
| soft_delete_configuration = SoftDeleteConfiguration( | ||
| enabled_soft_deletion=enable_soft_deletion, | ||
| soft_deletion_retention_period_in_minutes=soft_deletion_retention_period_in_minutes, | ||
| min_minutes_before_permanent_deletion_allowed=min_minutes_before_permanent_deletion_allowed | ||
| ) |
There was a problem hiding this comment.
Inconsistent parameter names used when creating SoftDeleteConfiguration. In cli_cosmosdb_update (line 1034), it uses enable_soft_deletion=enable_soft_deletion, while here in _create_database_account, it uses enabled_soft_deletion=enable_soft_deletion. These should use the same parameter name. Please verify the correct parameter name for the SoftDeleteConfiguration model and use it consistently in both functions.
|
@mohhef please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
4 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)For new extensions:
About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update
src/index.jsonautomatically.You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify
src/index.json.