fix: clean bulk_publish flags when deploy complete (#272)

Author: ohadedry

requirements-dev.txt

@@ -11,7 +11,7 @@ argcomplete>=3.6.2
 psutil==7.0.0
 requests
 cryptography
-fabric-cicd>=1.2.0
+fabric-cicd>=1.3.0
 
 # Testing and Building Requirements
 tox>=4.20.0

src/fabric_cli/commands/fs/deploy/fab_fs_deploy_config_file.py

@@ -4,7 +4,13 @@
 import json
 from argparse import Namespace
 
-from fabric_cicd import append_feature_flag, configure_external_file_logging, deploy_with_config, disable_file_logging  # type: ignore
+from fabric_cicd import (  # type: ignore
+    append_feature_flag,
+    configure_external_file_logging,
+    deploy_with_config,
+    disable_file_logging,
+    remove_feature_flag,
+)
 
 from fabric_cli.core import fab_constant, fab_state_config
 from fabric_cli.core import fab_logger
@@ -37,8 +43,7 @@ def deploy_with_config_file(args: Namespace) -> None:
         for param in deploy_parameters:
             if isinstance(deploy_parameters[param], str):
                 try:
-                    deploy_parameters[param] = json.loads(
-                        deploy_parameters[param])
+                    deploy_parameters[param] = json.loads(deploy_parameters[param])
                 except json.JSONDecodeError:
                     # If it's not a valid JSON string, keep it as is
                     pass
@@ -51,17 +56,20 @@ def deploy_with_config_file(args: Namespace) -> None:
             config_file_path=deploy_config_file,
             environment=args.target_env,
             token_credential=create_fabric_token_credential(),  # MSAL bridge TokenCredential
-            **deploy_parameters
+            **deploy_parameters,
         )
 
         if result:
-            fab_ui.print_output_format(
-                args, message=result.message)
+            fab_ui.print_output_format(args, message=result.message)
 
     except Exception as e:
         raise FabricCLIError(
-            f"Deployment failed: {str(e)}",
-            fab_constant.ERROR_IN_DEPLOYMENT)
+            f"Deployment failed: {str(e)}", fab_constant.ERROR_IN_DEPLOYMENT
+        )
+    finally:
+        bulk_publish_enabled = getattr(args, "bulk_publish", False)
+        if bulk_publish_enabled:
+            _remove_bulk_publish_feature_flags()
 
 
 def _apply_bulk_publish_feature_flags(args: Namespace) -> None:
@@ -82,3 +90,9 @@ def _apply_bulk_publish_feature_flags(args: Namespace) -> None:
             "fabric-cicd and may change or fail; omit the '--bulk_publish' flag "
             "to use standard per-item publish."
         )
+
+
+def _remove_bulk_publish_feature_flags() -> None:
+    """Remove command-scoped bulk publish flags from fabric-cicd global state."""
+    remove_feature_flag("enable_experimental_features")
+    remove_feature_flag("enable_bulk_publish")

tests/test_utils/test_fab_deploy_bulk_publish.py

@@ -3,6 +3,10 @@
 
 from unittest.mock import patch
 
+import pytest
+
+from fabric_cli.core.fab_exceptions import FabricCLIError
+
 
 class TestDeployBulkPublish:
     """
@@ -11,27 +15,30 @@ class TestDeployBulkPublish:
     HTTP cassettes.
     """
 
-    def _run_deploy(self, tmp_path, bulk_publish, mock_fab_set_state_config):
-        """Invoke deploy_with_config_file with fabric-cicd mocked, returning the
-        append_feature_flag mock for assertions."""
+    def _create_deploy_args(self, tmp_path, bulk_publish):
+        """Create arguments for deploy_with_config_file."""
         from argparse import Namespace
 
-        import fabric_cli.commands.fs.deploy.fab_fs_deploy_config_file as deploy_mod
-        from fabric_cli.core import fab_constant
-
-        # disable debug mode so fabric-cicd file logging is disabled during the run
-        mock_fab_set_state_config(fab_constant.FAB_DEBUG_ENABLED, "false")
-
-        args = Namespace(
+        return Namespace(
             config=str(tmp_path / "config.yml"),
             target_env="dev",
             params=None,
             bulk_publish=bulk_publish,
             command_path="deploy",
         )
 
+    def _run_deploy_success(self, tmp_path, bulk_publish, mock_fab_set_state_config):
+        """Run a successful deployment and return feature flag mocks."""
+        import fabric_cli.commands.fs.deploy.fab_fs_deploy_config_file as deploy_mod
+        from fabric_cli.core import fab_constant
+
+        # disable debug mode so fabric-cicd file logging is disabled during the run
+        mock_fab_set_state_config(fab_constant.FAB_DEBUG_ENABLED, "false")
+        args = self._create_deploy_args(tmp_path, bulk_publish)
+
         with (
             patch.object(deploy_mod, "append_feature_flag") as mock_flag,
+            patch.object(deploy_mod, "remove_feature_flag") as mock_remove_flag,
             patch.object(deploy_mod, "deploy_with_config", return_value=None),
             patch.object(deploy_mod, "disable_file_logging"),
             patch.object(deploy_mod, "configure_external_file_logging"),
@@ -41,28 +48,74 @@ def _run_deploy(self, tmp_path, bulk_publish, mock_fab_set_state_config):
         ):
             deploy_mod.deploy_with_config_file(args)
 
-        return mock_flag
+        return mock_flag, mock_remove_flag
+
+    def _run_deploy_failure(self, tmp_path, bulk_publish, mock_fab_set_state_config):
+        """Run a failed deployment and return feature flag mocks."""
+        import fabric_cli.commands.fs.deploy.fab_fs_deploy_config_file as deploy_mod
+        from fabric_cli.core import fab_constant
+
+        # disable debug mode so fabric-cicd file logging is disabled during the run
+        mock_fab_set_state_config(fab_constant.FAB_DEBUG_ENABLED, "false")
+        args = self._create_deploy_args(tmp_path, bulk_publish)
+
+        with (
+            patch.object(deploy_mod, "append_feature_flag") as mock_flag,
+            patch.object(deploy_mod, "remove_feature_flag") as mock_remove_flag,
+            patch.object(
+                deploy_mod,
+                "deploy_with_config",
+                side_effect=Exception("Simulated deployment failure"),
+            ),
+            patch.object(deploy_mod, "disable_file_logging"),
+            patch.object(deploy_mod, "configure_external_file_logging"),
+            patch.object(
+                deploy_mod, "create_fabric_token_credential", return_value=None
+            ),
+        ):
+            with pytest.raises(FabricCLIError):
+                deploy_mod.deploy_with_config_file(args)
+
+        return mock_flag, mock_remove_flag
 
     def test_deploy_bulk_publish_enabled_appends_experimental_flags_success(
         self, tmp_path, mock_fab_set_state_config
     ):
         """When --bulk_publish is set, both experimental bulk publish flags are appended."""
-        mock_flag = self._run_deploy(tmp_path, True, mock_fab_set_state_config)
+        mock_flag, mock_remove_flag = self._run_deploy_success(
+            tmp_path, True, mock_fab_set_state_config
+        )
 
         appended = [call.args[0] for call in mock_flag.call_args_list]
         assert "enable_experimental_features" in appended
         assert "enable_bulk_publish" in appended
         # existing behavior is preserved
         assert "disable_print_identity" in appended
+        removed = [call.args[0] for call in mock_remove_flag.call_args_list]
+        assert removed == ["enable_experimental_features", "enable_bulk_publish"]
+
+    def test_deploy_bulk_publish_enabled_removes_flags_on_failure(
+        self, tmp_path, mock_fab_set_state_config
+    ):
+        """Bulk publish flags are removed when deployment raises an exception."""
+        _, mock_remove_flag = self._run_deploy_failure(
+            tmp_path, True, mock_fab_set_state_config
+        )
+
+        removed = [call.args[0] for call in mock_remove_flag.call_args_list]
+        assert removed == ["enable_experimental_features", "enable_bulk_publish"]
 
     def test_deploy_bulk_publish_disabled_by_default_omits_flags_success(
         self, tmp_path, mock_fab_set_state_config
     ):
-        """When --bulk_publish is not set (default), bulk publish flags are not appended."""
-        mock_flag = self._run_deploy(tmp_path, False, mock_fab_set_state_config)
+        """When --bulk_publish is not set, bulk publish flags are not changed."""
+        mock_flag, mock_remove_flag = self._run_deploy_success(
+            tmp_path, False, mock_fab_set_state_config
+        )
 
         appended = [call.args[0] for call in mock_flag.call_args_list]
         assert "enable_experimental_features" not in appended
         assert "enable_bulk_publish" not in appended
         # existing behavior is preserved
         assert "disable_print_identity" in appended
+        mock_remove_flag.assert_not_called()