fix: add host app when running deploy cmd - #269
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes Fabric CLI User-Agent construction into a dedicated utility module and ensures the deploy command always sets a CLI-controlled User-Agent (optionally prefixed with the fabric-cicd library version) to improve attribution/telemetry for CICD-triggered deployments.
Changes:
- Added
fab_user_agent.pyutility with sharedbuild_user_agent(...)andresolve_library_user_agent(...)helpers (including host-app suffix handling). - Updated API request handling and
deploy_with_config_fileto use the centralized User-Agent logic and enforce a CLI-controlled deploy User-Agent. - Added/updated tests to validate User-Agent behavior and moved host-app suffix tests into the new utility test module; added a changelog entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/fabric_cli/utils/fab_user_agent.py |
New shared utility for building CLI User-Agent strings and resolving host library versions. |
src/fabric_cli/client/fab_api_client.py |
Replaces inline User-Agent construction with the new shared utility. |
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py |
Forces CLI-controlled User-Agent for deploy runs (with optional CICD prefix). |
tests/test_utils/test_fab_user_agent.py |
New unit tests for the new User-Agent utility functions. |
tests/test_core/test_fab_api_client.py |
Removes host-app suffix tests that moved to the new utility test module. |
tests/test_commands/test_deploy.py |
Adds deploy-specific tests to assert CICD-prefixed User-Agent behavior and prevent spoofing via params. |
.changes/unreleased/fixed-20260726-120000.yaml |
Changelog entry documenting the deployment User-Agent attribution fix. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py:67
- This
raise FabricCLIError(...)is now split across multiple lines even though it fits within Black's configured 88-char limit (tox.toml [tool.black]). To keep formatting consistent and avoid lint churn, format it as a single line.
raise FabricCLIError(
f"Deployment failed: {str(e)}", fab_constant.ERROR_IN_DEPLOYMENT
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/fabric_cli/utils/fab_user_agent.py:20
- The docstring example mentions
host-app/ado/..., but_get_host_app()actually emitshost-app/<allowed-name-lowercased>/...(e.g.,host-app/fabric-azuredevops-extension/...). This makes the example misleading for callers and tests.
"""Build the User-Agent header for API requests.
Example:
ms-fabric-cli/1.0.0 (create; Windows/10; Python/3.10.2) host-app/ado/2.0.0
"""
tests/test_commands/test_deploy.py:446
- Same issue as the prior test: the expected value should follow the conditional prefix behavior used by
deploy_with_config_file()when the fabric-cicd package version cannot be resolved.
cicd_user_agent = resolve_library_user_agent(
"fabric-cicd", "ms-fabric-cicd")
assert (
captured["user_agent"]
== f"{cicd_user_agent},{build_user_agent('deploy')}"
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
tests/test_utils/test_fab_user_agent.py:72
importlib.metadata.PackageNotFoundErrorexpects the missing distribution name; using the exception class asside_effectcan raise aTypeErrorinstead of the intendedPackageNotFoundError, making this test fail on supported Python versions.
with patch(
"fabric_cli.utils.fab_user_agent.importlib.metadata.version",
side_effect=importlib.metadata.PackageNotFoundError,
):
src/fabric_cli/utils/fab_user_agent.py:20
- The docstring example uses
ado/2.0.0, but the actual host-app suffix token produced by_get_host_app()is derived fromALLOWED_FAB_HOST_APP_VALUES(e.g.,fabric-azuredevops-extension). Updating the example avoids misleading callers.
"""Build the User-Agent header for API requests.
Example:
ms-fabric-cli/1.0.0 (create; Windows/10; Python/3.10.2) ado/2.0.0
"""
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py:14
- This import line exceeds the repo's Black line length (88) and will be reformatted/fail
tox -e lint. Please wrap it so formatting is stable.
from fabric_cli.utils.fab_user_agent import build_user_agent, resolve_library_user_agent
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py:52
- These lines aren’t Black-formatted (and split the call oddly), which will fail the repo’s
blacklint step. Reformat to a Black-compliant layout.
cicd_user_agent = resolve_library_user_agent(
"fabric-cicd", "ms-fabric-cicd")
deploy_parameters["user_agent"] = (
f"{cicd_user_agent},{build_user_agent(args.command_path)}"
if cicd_user_agent
…led fabric-cicd actually accepts a user_agent argument
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/fabric_cli/utils/fab_user_agent.py:20
- The docstring example uses an
ado/…suffix, but the implementation only allows theFabric-AzureDevops-Extensionhost-app value (rendered asfabric-azuredevops-extension). Updating the example avoids misleading readers about the expected suffix format.
"""Build the User-Agent header for API requests.
Example:
ms-fabric-cli/1.0.0 (create; Windows/10; Python/3.10.2) ado/2.0.0
"""
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
tests/test_commands/test_deploy.py:10
pytestandyamlare imported but never used in this file. Removing them avoids unused-import lint failures.
import pytest
import yaml
tests/test_commands/test_deploy.py:5
import osis unused in this test module (noosreferences). Keeping unused imports can fail linting and makes dependencies unclear.
This issue also appears on line 9 of the same file.
import os
import platform
.changes/unreleased/fixed-20260726-120000.yaml:2
- Changelog entry uses "User Agent" whereas other entries use the standard "User-Agent" spelling (e.g., fixed-20260706-165104.yaml). Aligning the wording improves consistency/searchability.
body: Set User Agent suffix for Fabric CLI deploy command.
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py:48
- This hardcodes the deploy
host_appstring in-line. The PR description mentions centralizing User-Agent construction in a dedicated utility, but there is currently no shared implementation (andfab_api_client.pystill contains its own UA builder). Consider extracting a shared helper for the CLI UA/host_app string (and updating callers) or adjust the PR description to match the actual scope.
deploy_parameters["host_app"] = (
f"{fab_constant.API_USER_AGENT}/{fab_constant.FAB_VERSION}"
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/test_commands/test_deploy.py:404
- This call wrapping will be reformatted by Black (tox.toml:39-44) since it comfortably fits within the line length limit. Keeping it on one line helps avoid churn and potential lint failures when contributors run
tox -e lint.
self._run_deploy_with_config_file(
fake_deploy_with_config, params=params)
tests/test_commands/test_deploy.py:384
- This
patch.objectcall is split in a way that Black will reformat (andtox -e lintruns Black; see tox.toml:39-44). Consider keeping it on one line to match Black’s preferred formatting and avoid CI lint failures.
This issue also appears on line 403 of the same file.
patch.object(deploy_mod.fab_ui,
"print_output_format", MagicMock()),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/test_commands/test_deploy.py:384
- This context-manager entry is formatted with a manual alignment that Black will reformat; keeping it Black-compliant avoids noisy diffs and helps CI pass
tox -e lint(Black).
patch.object(deploy_mod.fab_ui,
"print_output_format", MagicMock()),
tests/test_commands/test_deploy.py:7
osis imported but not used anywhere in this test module, which will fail linting in many setups and adds noise. Remove the unused import.
This issue also appears on line 383 of the same file.
import os
import platform
from argparse import Namespace
from unittest.mock import MagicMock, patch
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py:48
- The PR description claims a larger refactor (new
fab_user_agent.py, moving User-Agent logic out offab_api_client.py, and adding a newtest_fab_user_agent.py), but those files/symbols are not present in this change set. Please either update the PR description to match the actual changes or include the missing refactor/test changes.
deploy_parameters["host_app"] = (
f"{fab_constant.API_USER_AGENT}/{fab_constant.FAB_VERSION}"
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py:48
- The PR description claims broader refactoring (new
fab_user_agent.py, changes infab_api_client.py, and moving host app suffix tests) that are not present in this diff. Please update the PR description to reflect the actual changes in this PR (addinghost_appfor deploy and bumpingfabric-cicd), or include the missing commits/files if they were intended to be part of this PR.
deploy_parameters["host_app"] = (
f"{fab_constant.API_USER_AGENT}/{fab_constant.FAB_VERSION}"
)
📥 Pull Request
✨ Description of new changes
This pull request refactors how the Fabric CLI sets and manages the User-Agent string for deployment commands, making it easier to attribute deployments and improving telemetry for the Fabric CICD service. The logic for building and resolving User-Agent strings is now centralized in a new utility module, and the deployment command ensures that the correct User-Agent is always set, regardless of user input. Comprehensive tests have been added and updated to verify this behavior.
User-Agent construction and attribution:
fab_user_agent.pythat centralizes logic for building the User-Agent string (build_user_agent) and resolving library versions for attribution (resolve_library_user_agent). This ensures consistent and maintainable User-Agent formatting across the CLI.fab_fs_deploy_config_file.pyto always set a CLI-controlled User-Agent, combining both the CICD library version and the CLI's own User-Agent. This prevents users from spoofing the User-Agent via parameters and enables accurate attribution for telemetry and analytics. [1] [2]Refactoring and code cleanup:
fab_api_client.pyand replaced it with calls to the new utility functions, simplifying the client code and removing redundant code. [1] [2] [3] [4]Testing improvements:
test_fab_user_agent.pyto thoroughly test User-Agent construction, environment variable handling, and library version resolution.test_fab_api_client.pytotest_fab_user_agent.pyfor better organization. [1] [2]Documentation and changelog: