Conversation
There was a problem hiding this comment.
Pull request overview
Adds hard-delete support to the rm command by introducing a new --purge flag that triggers permanent deletion (via hardDelete=true) and updates docs + VCR recordings accordingly.
Changes:
- Added
--purgeflag tofab rmand documented the new behavior. - Wired
--purgethrough item/workspace deletion flows by setting request query params (hardDelete=true) and adding stronger confirmation messaging. - Added/updated VCR recordings covering purge-delete success across multiple item types.
Reviewed changes
Copilot reviewed 39 out of 57 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[SparkJobDefinition].yaml | Adds purge-delete success recording for SparkJobDefinition. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[Reflex].yaml | Adds purge-delete success recording for Reflex. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[MirroredDatabase].yaml | Adds purge-delete success recording for MirroredDatabase. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[MLModel].yaml | Adds purge-delete success recording for MLModel (async create flow included). |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[Lakehouse].yaml | Adds purge-delete success recording for Lakehouse. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[KQLQueryset].yaml | Adds purge-delete success recording for KQLQueryset. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[KQLDashboard].yaml | Adds purge-delete success recording for KQLDashboard. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[GraphQLApi].yaml | Adds purge-delete success recording for GraphQLApi. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[Eventstream].yaml | Adds purge-delete success recording for Eventstream (async create flow included). |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[Eventhouse].yaml | Adds purge-delete success recording for Eventhouse. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[Environment].yaml | Adds purge-delete success recording for Environment. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[Dataflow].yaml | Adds purge-delete success recording for Dataflow. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[DataPipeline].yaml | Adds purge-delete success recording for DataPipeline. |
| tests/test_commands/recordings/test_commands/test_rm/test_rm_item_with_purge_delete_success[CopyJob].yaml | Adds purge-delete success recording for CopyJob. |
| tests/test_commands/recordings/test_commands/test_rm/class_setup.yaml | Updates test setup recording (User-Agent/version + workspace ids/contents). |
| src/fabric_cli/parsers/fab_fs_parser.py | Adds --purge flag to rm parser and updates examples/help. |
| src/fabric_cli/commands/fs/rm/fab_fs_rm_workspace.py | Propagates purge deletion to item deletes when deleting items within a workspace flow. |
| src/fabric_cli/commands/fs/rm/fab_fs_rm_item.py | Sets request params for purge deletion when removing a single item. |
| src/fabric_cli/client/fab_api_utils.py | Adds stronger confirmation/warnings for purge deletion, plus minor formatting changes. |
| docs/commands/fs/rm.md | Documents --purge flag and adds examples for soft/force/purge deletion. |
| if hasattr(args, 'purge') and args.purge: | ||
| confirm_message = "Your item will be deleted forever. Are you sure you want to proceed?" | ||
| else: | ||
| confirm_message = "Are you sure?" | ||
|
|
||
| if utils_ui.prompt_confirm(confirm_message): | ||
| if hasattr(args, 'purge') and args.purge and verbose: | ||
| utils_ui.print_warning("! Executing purge delete") |
There was a problem hiding this comment.
The confirmation text is item-specific ("Your item") even though delete_resource appears to be a generic helper (used across resource types/operations). Consider making the prompt message resource-agnostic (or include the actual resource name/type) so the UX remains accurate for non-item deletes.
| raise FabricCLIError( | ||
| ErrorMessages.Client.resource_type_not_found_in_provider(args.resource_type, args.provider_namespace), | ||
| ErrorMessages.Client.resource_type_not_found_in_provider( | ||
| args.resource_type, args.provider_namespace), |
There was a problem hiding this comment.
get_api_version(resource_uri: str) references args.resource_type / args.provider_namespace, but args is not defined within this function scope. Unless args is intentionally a module-global (which would be very brittle), this will raise NameError at runtime. Prefer passing the needed values into get_api_version(...) (or deriving them from resource_uri) and using local variables in the error message.
| args.resource_type, args.provider_namespace), | |
| resource_type, namespace | |
| ), |
| if hasattr(args, 'purge') and args.purge: | ||
| args.request_params = {"hardDelete": "true"} | ||
| else: | ||
| args.request_params = {} |
There was a problem hiding this comment.
This unconditionally overwrites args.request_params, which can accidentally discard other query params that may have been set earlier in the call chain. Consider merging into any existing params (e.g., via getattr(args, 'request_params', {})) and using getattr(args, 'purge', False) to avoid repeated hasattr(...) patterns.
| if hasattr(args, 'purge') and args.purge: | |
| args.request_params = {"hardDelete": "true"} | |
| else: | |
| args.request_params = {} | |
| request_params = getattr(args, "request_params", {}) | |
| if getattr(args, "purge", False): | |
| # Merge hard delete flag into any existing request params | |
| merged_params = dict(request_params) | |
| merged_params["hardDelete"] = "true" | |
| args.request_params = merged_params | |
| else: | |
| # Ensure request_params attribute exists without discarding existing values | |
| args.request_params = request_params |
| if hasattr(args, 'purge') and args.purge: | ||
| args.request_params = {"hardDelete": "true"} | ||
| else: | ||
| args.request_params = {} |
There was a problem hiding this comment.
Same issue as the single-item path: overwriting args.request_params can drop previously configured request parameters. Since this runs inside a loop, it also increases the chance of subtle cross-iteration state leakage if future params are added. Prefer merging into existing params and using getattr(args, 'purge', False) for clarity.
| if hasattr(args, 'purge') and args.purge: | |
| args.request_params = {"hardDelete": "true"} | |
| else: | |
| args.request_params = {} | |
| # Preserve any existing request parameters and only toggle hardDelete | |
| params = dict(getattr(args, "request_params", {}) or {}) | |
| if getattr(args, "purge", False): | |
| params["hardDelete"] = "true" | |
| else: | |
| params.pop("hardDelete", None) | |
| args.request_params = params |
| "# remove a table", | ||
| "$ rm lh1.Lakehouse/Tables/fabtbl", | ||
| "# permanently remove an item (purge)", | ||
| "$ rm nb1.Notebook --purge --force", |
There was a problem hiding this comment.
The new --purge example uses nb1.Notebook without a workspace-qualified path, while other examples demonstrate fully-qualified Fabric paths. To align with the CLI’s path-first UX and avoid ambiguity, consider updating this example to a fully-qualified path (e.g., ws1.Workspace/nb1.Notebook --purge --force).
| "$ rm nb1.Notebook --purge --force", | |
| "$ rm ws1.Workspace/nb1.Notebook --purge --force", |
Adds
--purgeflag tormcommand, which permanently deletes the item.