Skip to content

fix: clean bulk_publish flags when deploy complete - #272

Merged
ohadedry merged 3 commits into
microsoft:mainfrom
ohadedry:fix-bulk-publish-cleanup
Aug 17, 2026
Merged

fix: clean bulk_publish flags when deploy complete#272
ohadedry merged 3 commits into
microsoft:mainfrom
ohadedry:fix-bulk-publish-cleanup

Conversation

@ohadedry

Copy link
Copy Markdown
Contributor

This pull request improves the handling of the --bulk_publish feature flag during deployments and enhances test coverage to ensure correct behavior. The main changes include updating dependencies, ensuring feature flags are cleaned up after deployment, and adding tests to verify both the addition and removal of feature flags.

Bulk publish feature flag management:

  • Ensured that when --bulk_publish is used, the corresponding feature flags (enable_experimental_features and enable_bulk_publish) are removed from global state after deployment by introducing the _remove_bulk_publish_feature_flags function and calling it in a finally block in deploy_with_config_file. [1] [2]
  • Updated the import from fabric_cicd to include remove_feature_flag.

Testing improvements:

  • Enhanced tests in test_fab_deploy_bulk_publish.py to verify that feature flags are correctly appended and removed when --bulk_publish is set, and that no removal occurs when it is not set. [1] [2]
  • Improved test helper method to return both append and remove feature flag mocks for assertions.

Dependency updates:

  • Updated fabric-cicd dependency to version >=1.3.0 in requirements-dev.txt.

Copilot AI lite review requested due to automatic review settings August 13, 2026 12:28
@ohadedry
ohadedry requested a review from a team as a code owner August 13, 2026 12:28

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 improves the fab fs deploy bulk publish feature-flag lifecycle by ensuring fabric-cicd bulk publish flags are removed after a deployment run, and updates tests/deps to support that behavior.

Changes:

  • Added _remove_bulk_publish_feature_flags() and invoked it in a finally block when --bulk_publish is enabled.
  • Expanded unit tests to assert both flag append and removal behavior.
  • Bumped fabric-cicd dev dependency to >=1.3.0 to pick up remove_feature_flag.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py Imports remove_feature_flag, tracks bulk_publish usage, and cleans up bulk-publish flags after deploy completes.
tests/test_utils/test_fab_deploy_bulk_publish.py Updates test helper to mock/remove feature flags and adds assertions around cleanup behavior.
requirements-dev.txt Updates fabric-cicd minimum dev version to support the new API.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py
Comment thread src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py
Comment thread tests/test_utils/test_fab_deploy_bulk_publish.py

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 3 out of 3 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:69

  • The broad except Exception will also catch and rewrap FabricCLIError, which can change the original CLI error code/message and make upstream handling inconsistent with other commands (which generally re-raise FabricCLIError unchanged). Consider re-raising FabricCLIError and only wrapping unexpected exceptions (optionally chaining with from e).
    except Exception as e:
        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 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 3 out of 3 changed files in this pull request and generated no new comments.

@ohadedry
ohadedry merged commit 21d3cd3 into microsoft:main Aug 17, 2026
10 checks passed
@ohadedry
ohadedry deleted the fix-bulk-publish-cleanup branch August 17, 2026 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants