Skip to content

fix: add host app when running deploy cmd - #269

Open
aviatco wants to merge 7 commits into
microsoft:mainfrom
aviatco:dev/aviatcohen/fix-deploy-cmd-telemetry
Open

fix: add host app when running deploy cmd#269
aviatco wants to merge 7 commits into
microsoft:mainfrom
aviatco:dev/aviatcohen/fix-deploy-cmd-telemetry

Conversation

@aviatco

@aviatco aviatco commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

📥 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:

  • Introduced a new utility module fab_user_agent.py that 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.
  • Updated the deployment command in fab_fs_deploy_config_file.py to 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:

  • Removed the old User-Agent construction logic from fab_api_client.py and replaced it with calls to the new utility functions, simplifying the client code and removing redundant code. [1] [2] [3] [4]

Testing improvements:

  • Added a new test module test_fab_user_agent.py to thoroughly test User-Agent construction, environment variable handling, and library version resolution.
  • Updated deployment tests to verify that the correct User-Agent is passed and cannot be overridden by user-supplied parameters.
  • Moved and consolidated tests for host app suffix logic from test_fab_api_client.py to test_fab_user_agent.py for better organization. [1] [2]

Documentation and changelog:

  • Added a changelog entry describing the fix and the benefits of the new User-Agent attribution for deployments.

@aviatco
aviatco requested a review from a team as a code owner July 26, 2026 14:50
Copilot AI review requested due to automatic review settings July 26, 2026 14:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py utility with shared build_user_agent(...) and resolve_library_user_agent(...) helpers (including host-app suffix handling).
  • Updated API request handling and deploy_with_config_file to 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.

Comment thread src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py Outdated
Comment thread tests/test_utils/test_fab_user_agent.py Outdated
@@ -0,0 +1,6 @@
kind: fixed
body: Set User Agent suffix for Fabric CLI deploy command. This allows the Fabric CICD service to identify the host application that is invoking the deployment, which can be useful for logging, analytics, and debugging purposes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why all these changes? only need to tell cicd on it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: Set User Agent suffix for Fabric CLI deploy command. This allows the Fabric CICD service to identify the host application that is invoking the deployment, which can be useful for logging, analytics, and debugging purposes.
body: Set User Agent suffix for Fabric CLI deploy command.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should skip change log because this is internal fix?

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 27, 2026 08:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
        )

Comment thread src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py Outdated
Copilot AI review requested due to automatic review settings July 27, 2026 08:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment thread src/fabric_cli/utils/fab_user_agent.py Outdated
Copilot AI review requested due to automatic review settings July 27, 2026 08:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 emits host-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')}"
        )

Comment thread tests/test_commands/test_deploy.py Outdated
Comment on lines +410 to +415
cicd_user_agent = resolve_library_user_agent(
"fabric-cicd", "ms-fabric-cicd")
assert (
captured["user_agent"]
== f"{cicd_user_agent},{build_user_agent('deploy')}"
)
@@ -0,0 +1,6 @@
kind: fixed
body: Set User Agent suffix for Fabric CLI deploy command.
Copilot AI review requested due to automatic review settings July 27, 2026 09:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.PackageNotFoundError expects the missing distribution name; using the exception class as side_effect can raise a TypeError instead of the intended PackageNotFoundError, 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 from ALLOWED_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 black lint 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
Copilot AI review requested due to automatic review settings July 27, 2026 10:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the Fabric-AzureDevops-Extension host-app value (rendered as fabric-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
    """

Comment thread src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py Outdated
assert result == expected_suffix


@pytest.fixture()
Copilot AI review requested due to automatic review settings July 28, 2026 09:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • pytest and yaml are 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 os is unused in this test module (no os references). 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_app string in-line. The PR description mentions centralizing User-Agent construction in a dedicated utility, but there is currently no shared implementation (and fab_api_client.py still 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}"
        )

@aviatco aviatco changed the title fix: add ms-fabric-cli user agent to cicd when running deploy cmd fix: add host app to cicd when running deploy cmd Jul 28, 2026
@aviatco aviatco changed the title fix: add host app to cicd when running deploy cmd fix: add host app when running deploy cmd Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants