Conversation
There was a problem hiding this comment.
Pull request overview
Removes the CLI’s hardcoded default "description" field from various create/import payloads so descriptions are only sent when intentionally provided, improving payload predictability.
Changes:
- Removed default
"description"stamping from item import payloads and environment import-create payloads. - Removed default
"description"stamping from multiplemkdirpayload builders (workspace, item, folder, connection, gateway). - Updated/added tests to stop expecting default descriptions and to assert description absence by default.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils/test_fab_cmd_mkdir_utils.py | Updates existing connection-payload tests and adds new assertions about description presence/absence. |
| tests/test_core/test_fab_hiearchy.py | Removes "Imported from fab" expectations and adds a regression test that import payloads don’t auto-stamp description. |
| src/fabric_cli/utils/fab_cmd_import_utils.py | Removes hardcoded "Imported from fab" from get_payload_for_item_type() payloads. |
| src/fabric_cli/commands/fs/mkdir/fab_fs_mkdir_workspace.py | Removes hardcoded "Created by fab" from workspace create payload. |
| src/fabric_cli/commands/fs/mkdir/fab_fs_mkdir_item.py | Removes hardcoded "Created by fab" from item create payload. |
| src/fabric_cli/commands/fs/mkdir/fab_fs_mkdir_gateway.py | Removes leftover commented default description from gateway payload block. |
| src/fabric_cli/commands/fs/mkdir/fab_fs_mkdir_folder.py | Removes hardcoded "Created by fab" from folder create payload. |
| src/fabric_cli/commands/fs/mkdir/fab_fs_mkdir_connection.py | Removes hardcoded "Created by fab" from connection create payload. |
| src/fabric_cli/commands/fs/impor/fab_fs_import_item.py | Removes hardcoded "Imported from fab" from environment item create payload. |
| utils_ui.print_grey(f"Creating a new Connection...") | ||
|
|
||
| # Base payload | ||
| payload = { | ||
| "description": "Created by fab", | ||
| "displayName": connection.short_name, | ||
| "connectivityType": connectivityType, | ||
| } |
There was a problem hiding this comment.
description is listed as an optional param for mkdir connection, but it is never copied into the request payload. After removing the hardcoded default description, any user-supplied description in args.params will be silently ignored. Consider populating payload["description"] from params (or handling it inside get_connection_config_from_params) when provided, so the CLI behavior matches the advertised params.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| def test_connection_payload_contains_description_when_user_provides_it_success(): | ||
| """Verify that when a user explicitly passes description via params it is | ||
| forwarded into the payload returned by get_connection_config_from_params.""" | ||
| payload = { | ||
| "displayName": "test-connection", | ||
| "connectivityType": "ShareableCloud", | ||
| "description": "My custom description", # user supplied before calling helper | ||
| } | ||
|
|
||
| con_type = "FabricDataPipelines" | ||
| con_type_def = { | ||
| "type": "FabricDataPipelines", | ||
| "creationMethods": [ | ||
| { | ||
| "name": "FabricDataPipelines.Actions", | ||
| "parameters": [], | ||
| } | ||
| ], | ||
| "supportedCredentialTypes": ["WorkspaceIdentity"], | ||
| } | ||
|
|
||
| params = { | ||
| "connectiondetails": { | ||
| "type": "FabricDataPipelines", | ||
| "creationmethod": "FabricDataPipelines.Actions", | ||
| }, | ||
| "credentialdetails": { | ||
| "type": "WorkspaceIdentity", | ||
| }, | ||
| } | ||
|
|
||
| result = get_connection_config_from_params( | ||
| payload, con_type, con_type_def, params) | ||
|
|
||
| assert result.get("description") == "My custom description", ( | ||
| f"Connection payload must preserve a user-supplied description, got: {result}" | ||
| ) |
There was a problem hiding this comment.
This test claims that a user supplies description “via params”, but the test actually injects description directly into the payload before calling get_connection_config_from_params. That doesn’t exercise the real CLI path (where description would come from args.params) and will keep passing even if the CLI continues to ignore description. Update the test to provide description in params (and/or adjust the docstring) so it verifies the intended behavior end-to-end for param handling.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
Co-authored-by: Alon Yeshurun <98805507+ayeshurun@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/fabric_cli/commands/fs/mkdir/fab_fs_mkdir_connection.py:121
mkdir connectionadvertisesdescriptionas an optional param, but the request payload never copiesargs.params['description']intopayload(andget_connection_config_from_paramsdoes not apply it either). After removing the hardcoded default description, any user-provided description will be silently ignored. Populate the payloaddescriptionfrom params (or haveget_connection_config_from_paramsdo it) when provided so CLI behavior matches the parameter contract.
# Base payload
payload = {
"displayName": connection.short_name,
"connectivityType": connectivityType,
}
if gateway_id:
payload["gatewayId"] = gateway_id
payload = mkdir_utils.get_connection_config_from_params(
payload, con_type, con_type_def, args.params
)
| def test_connection_payload_contains_description_when_user_provides_it_success(): | ||
| """Verify that when a user explicitly passes description via params it is | ||
| forwarded into the payload returned by get_connection_config_from_params.""" | ||
| payload = { | ||
| "displayName": "test-connection", | ||
| "connectivityType": "ShareableCloud", | ||
| "description": "My custom description", # user supplied before calling helper | ||
| } | ||
|
|
There was a problem hiding this comment.
This test claims the user provides description “via params”, but it injects description directly into the input payload instead. That doesn’t verify the actual CLI path (where description would arrive in args.params) and will still pass even if mkdir connection continues to ignore description. Move the description into params (and/or adjust the docstring) so the test validates param forwarding behavior.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
📥 Pull Request
✨ Description of new changes
This pull request removes the automatic addition of default
"description"fields (such as "Created by fab" or "Imported from fab") from payloads created by the CLI for items, folders, gateways, and connections. Now, descriptions are only included in payloads if explicitly provided by the user. The change is thoroughly tested to ensure backward compatibility and correct behavior.Key changes:
Removal of hardcoded descriptions from payloads:
"description"field from payloads in item, folder, gateway, workspace, and connection creation logic across files likefab_fs_import_item.py,fab_fs_mkdir_item.py,fab_fs_mkdir_folder.py,fab_fs_mkdir_gateway.py,fab_fs_mkdir_workspace.py, and related utility functions. [1] [2] [3] [4] [5] [6] [7]Test updates and new test coverage:
"description"fields in payloads. [1] [2] [3] [4] [5] [6] [7]"description"unless explicitly provided, and that user-supplied descriptions are preserved and forwarded correctly. [1] [2]Connection payload improvements:
get_connection_config_from_params) do not add a"description"by default, and only include it if the user supplies one. [1] [2] [3] [4]These changes make payload construction more predictable and ensure that metadata is only included when intentionally specified by the user.