Skip to content

Commit 7400852

Browse files
shirasassoonShira Sassoon
andauthored
Ensure reset of feature flags and constants after config deployment (#871)
Resolves #872 This pull request introduces a robust and safer mechanism for handling feature flag and constant overrides during configuration-based deployments. The main improvement is the replacement of the previous global override function with a context manager, ensuring that any changes to feature flags and constants are always reverted after deployment, even if errors occur. This prevents state leakage between deployments and improves reliability. The pull request also updates validation logic, enhances logging, and adds comprehensive tests to verify correct override and restoration behavior. Key changes include: **Override Mechanism Improvements** - Replaced the global `apply_config_overrides` function with a `config_overrides_scope` context manager in `src/fabric_cicd/_common/_config_utils.py`. This ensures that feature flags and constants are overridden only for the duration of the deployment and are always restored to their original values afterward, even if exceptions occur. [[1]](diffhunk://#diff-a595a996e85e1b2ab0a30945c022432c66c2c63fdd87c97030327c412c713485L150-R197) [[2]](diffhunk://#diff-a8555a82747092e8d1f2ee36f31c947b4e5a264ed6c6d8de48d8daf850372704L15-R15) [[3]](diffhunk://#diff-a8555a82747092e8d1f2ee36f31c947b4e5a264ed6c6d8de48d8daf850372704L408-R408) - Updated all usages and tests to use the new context manager, removing references to the old function. [[1]](diffhunk://#diff-591b837760640c6d5c7b21d70575e6a262d56881f9ff3f993dbf10064c61d468L12-R14) [[2]](diffhunk://#diff-591b837760640c6d5c7b21d70575e6a262d56881f9ff3f993dbf10064c61d468L375-R531) **Validation and Logging Enhancements** - Improved validation logic for the `features` and `constants` sections, including more granular checks for environment-specific mappings and individual constant keys. [[1]](diffhunk://#diff-2b2dbdc5a0a864161ffbaf5211918409f6b2a5ea15a9e23d17ff803e6f9b2834L347-R347) [[2]](diffhunk://#diff-2b2dbdc5a0a864161ffbaf5211918409f6b2a5ea15a9e23d17ff803e6f9b2834L356-R365) [[3]](diffhunk://#diff-2b2dbdc5a0a864161ffbaf5211918409f6b2a5ea15a9e23d17ff803e6f9b2834L1012-R1047) - Changed the logging level for skipped `features` and `constants` to warnings instead of debug, making potential misconfigurations more visible. [[1]](diffhunk://#diff-1482256f9dd419ccd2b7281c95d3b1fa902a4af31630ad0e9dbca891241d5f2fL305-R306) [[2]](diffhunk://#diff-2b2dbdc5a0a864161ffbaf5211918409f6b2a5ea15a9e23d17ff803e6f9b2834L1084-L1097) **Testing Improvements** - Added extensive tests to verify that feature flags and constants are correctly overridden within the context and always restored after, including when exceptions are raised. Tests also check that user-set values are preserved and that environment-specific overrides work as intended. **Documentation** - Updated documentation to reflect the new override behavior and logging changes for `features` and `constants`. **Bug Fix** - Fixes issue #872: Ensures that override behavior for feature flags and constants in config deployment is correct and state-safe. (.changes/unreleased/fixed-20260311-110454.yaml) --------- Co-authored-by: Shira Sassoon <shirasassoon@microsoft.com>
1 parent 01cce8b commit 7400852

File tree

7 files changed

+740
-384
lines changed

7 files changed

+740
-384
lines changed
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
kind: fixed
2+
body: Fix override behavior of feature flags and constants in config deployment
3+
time: 2026-03-11T11:04:54.7911345+02:00
4+
custom:
5+
Author: shirasassoon
6+
AuthorLink: https://github.com/shirasassoon
7+
Issue: "872"
8+
IssueLink: https://github.com/microsoft/fabric-cicd/issues/872

docs/how_to/config_deployment.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -302,8 +302,8 @@ Fields are categorized as **required** or **optional**, which affects how missin
302302
| `unpublish.exclude_regex` | ❌ | Debug logged, setting skipped |
303303
| `unpublish.items_to_include` | ❌ | Debug logged, setting skipped |
304304
| `unpublish.skip` | ❌ | Defaults to `False` |
305-
| `features` | ❌ | Debug logged, setting skipped |
306-
| `constants` | ❌ | Debug logged, setting skipped |
305+
| `features` | ❌ | Warning logged, setting skipped |
306+
| `constants` | ❌ | Warning logged, setting skipped |
307307

308308
### Selective Environment Configuration
309309

src/fabric_cicd/_common/_config_utils.py

Lines changed: 41 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33

44
"""Utilities for YAML-based deployment configuration."""
55

6+
import contextlib
67
import logging
8+
from collections.abc import Generator
79
from typing import Optional, Union
810

911
from fabric_cicd import constants
@@ -147,30 +149,47 @@ def extract_unpublish_settings(config: dict, environment: str) -> dict:
147149
return settings
148150

149151

150-
def apply_config_overrides(config: dict, environment: str) -> None:
151-
"""Apply feature flags and constants overrides from config.
152+
@contextlib.contextmanager
153+
def config_overrides_scope(config: dict, environment: str) -> Generator[None, None, None]:
154+
"""
155+
Context manager that applies feature flags and constants
156+
overrides from config and guarantees cleanup.
157+
158+
Feature flags and constants are restored to their pre-call values on exit,
159+
ensuring no state leaks between deployments.
152160
153161
Args:
154162
config: Configuration dictionary
155163
environment: Target environment for deployment
156164
"""
157-
if "features" in config:
158-
features = config["features"]
159-
features_list = features.get(environment, []) if isinstance(features, dict) else features
160-
161-
for feature in features_list:
162-
constants.FEATURE_FLAG.add(feature)
163-
logger.info(f"Enabled feature flag: {feature}")
164-
165-
if "constants" in config:
166-
constants_section = config["constants"]
167-
# Check if it's an environment mapping (all values are dicts)
168-
if all(isinstance(v, dict) for v in constants_section.values()):
169-
constants_dict = constants_section.get(environment, {})
170-
else:
171-
constants_dict = constants_section
172-
173-
for key, value in constants_dict.items():
174-
if hasattr(constants, key):
175-
setattr(constants, key, value)
176-
logger.warning(f"Override constant {key} = {value}")
165+
# Snapshot current state before any changes
166+
original_feature_flags = constants.FEATURE_FLAG.copy()
167+
overridden_keys = {}
168+
169+
try:
170+
# Set feature flags
171+
if "features" in config:
172+
features = config["features"]
173+
features_list = features.get(environment, []) if isinstance(features, dict) else features
174+
for feature in features_list:
175+
constants.FEATURE_FLAG.add(feature)
176+
logger.info(f"Enabled feature flag: {feature}")
177+
178+
# Apply constants overrides
179+
if "constants" in config:
180+
constants_section = config["constants"]
181+
for key in list(constants_section.keys()):
182+
value = get_config_value(constants_section, key, environment)
183+
if value is not None and hasattr(constants, key):
184+
overridden_keys[key] = getattr(constants, key)
185+
setattr(constants, key, value)
186+
logger.warning(f"Override constant {key} = {value}")
187+
yield
188+
189+
finally:
190+
# Restore original state — guaranteed even if deployment raises
191+
constants.FEATURE_FLAG.clear()
192+
constants.FEATURE_FLAG.update(original_feature_flags)
193+
194+
for key, original_value in overridden_keys.items():
195+
setattr(constants, key, original_value)

src/fabric_cicd/_common/_config_validator.py

Lines changed: 44 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ def _validate_environment_exists(self) -> None:
344344
if any(
345345
field_name in section and isinstance(section[field_name], dict)
346346
for section, field_name, _, _, _ in _get_config_fields(self.config)
347-
if not (field_name == "constants" and _is_regular_constants_dict(section.get(field_name, {})))
347+
if field_name != "constants"
348348
):
349349
self.errors.append(constants.CONFIG_VALIDATION_MSGS["environment"]["no_env_with_mappings"])
350350
return
@@ -353,8 +353,16 @@ def _validate_environment_exists(self) -> None:
353353
for section, field_name, display_name, is_required, log_warning in _get_config_fields(self.config):
354354
if field_name in section:
355355
field_value = section[field_name]
356-
# Handle constants special case
357-
if field_name == "constants" and _is_regular_constants_dict(field_value):
356+
# Handle constants special case — check each constant's value individually
357+
if field_name == "constants":
358+
if isinstance(field_value, dict):
359+
for const_key, const_val in field_value.items():
360+
if isinstance(const_val, dict) and self.environment not in const_val:
361+
available_envs = list(const_val.keys())
362+
logger.warning(
363+
f"Environment '{self.environment}' not found in 'constants.{const_key}'. "
364+
f"Available environments: {available_envs}. This constant will be skipped."
365+
)
358366
continue
359367

360368
# If it's a dict (environment mapping), check if target environment exists
@@ -593,8 +601,8 @@ def _resolve_path_field(
593601

594602
for env_key, path_str in paths_to_resolve.items():
595603
try:
596-
path = Path(path_str)
597604
env_desc = f" for environment '{env_key}'" if env_key != "_default" else ""
605+
path = Path(path_str)
598606

599607
if path.is_absolute():
600608
resolved_path = path
@@ -652,13 +660,9 @@ def _resolve_path_field(
652660
if isinstance(field_value, str):
653661
if section_name:
654662
self.config[section_name][field_name] = str(resolved_path)
655-
else:
656-
self.config[field_name] = str(resolved_path)
657663
else:
658664
if section_name:
659665
self.config[section_name][field_name][env_key] = str(resolved_path)
660-
else:
661-
self.config[field_name][env_key] = str(resolved_path)
662666

663667
except (OSError, ValueError) as e:
664668
self.errors.append(
@@ -975,11 +979,6 @@ def _validate_features_section(self, features: any) -> None:
975979

976980
# Validate each environment's features list
977981
for env, features_list in features.items():
978-
if not features_list:
979-
self.errors.append(
980-
constants.CONFIG_VALIDATION_MSGS["operation"]["empty_section_env"].format("features", env)
981-
)
982-
continue
983982
self._validate_features_list(features_list, f"features.{env}")
984983
return
985984

@@ -1009,49 +1008,45 @@ def _validate_constants_section(self, constants_section: any) -> None:
10091008
)
10101009
return
10111010

1012-
# Check if all values are dictionaries (contains environment mapping)
1013-
if constants_section and all(isinstance(value, dict) for value in constants_section.values()):
1014-
# Validate environment mapping
1015-
if not self._validate_environment_mapping(constants_section, "constants", dict):
1016-
return
1017-
1018-
# Validate each environment's constants dictionary
1019-
for env, env_constants in constants_section.items():
1020-
if not env_constants:
1021-
self.errors.append(
1022-
constants.CONFIG_VALIDATION_MSGS["operation"]["empty_section_env"].format("constants", env)
1023-
)
1024-
continue
1025-
self._validate_constants_dict(env_constants, f"constants.{env}")
1026-
else:
1027-
# Simple constants dictionary
1028-
self._validate_constants_dict(constants_section, "constants")
1029-
1030-
def _validate_constants_dict(self, constants_dict: dict, context: str) -> None:
1031-
"""Validate a constants dictionary with proper context for error messages."""
1032-
for key, value in constants_dict.items():
1011+
# Validate each constant key individually — values can be flat or per-key env mappings
1012+
for key, value in constants_section.items():
10331013
if not isinstance(key, str) or not key.strip():
10341014
self.errors.append(
1035-
constants.CONFIG_VALIDATION_MSGS["operation"]["invalid_constant_key"].format(context, key)
1015+
constants.CONFIG_VALIDATION_MSGS["operation"]["invalid_constant_key"].format("constants", key)
10361016
)
10371017
continue
10381018

10391019
# Validate that the constant exists in the constants module
10401020
if not hasattr(constants, key):
10411021
self.errors.append(
1042-
constants.CONFIG_VALIDATION_MSGS["operation"]["unknown_constant"].format(key, context)
1022+
constants.CONFIG_VALIDATION_MSGS["operation"]["unknown_constant"].format(key, "constants")
10431023
)
1044-
continue
10451024

1046-
# Validate URL constants
1047-
if key in _URL_CONSTANTS:
1048-
if not isinstance(value, str):
1049-
self.errors.append(f"'{key}' in '{context}' must be a string URL, got {type(value).__name__}")
1050-
continue
1051-
try:
1052-
validate_api_url(value, f"{context}.{key}")
1053-
except InputError as e:
1054-
self.errors.append(str(e))
1025+
if isinstance(value, dict):
1026+
# Per-key environment mapping: { KEY: { dev: val, prod: val } }
1027+
for env, env_value in value.items():
1028+
if not isinstance(env, str) or not env.strip():
1029+
self.errors.append(
1030+
constants.CONFIG_VALIDATION_MSGS["environment"]["invalid_env_key"].format(
1031+
f"constants.{key}", type(env).__name__
1032+
)
1033+
)
1034+
continue
1035+
self._validate_single_constant(key, env_value, f"constants.{key}.{env}")
1036+
else:
1037+
# Flat value: { KEY: val }
1038+
self._validate_single_constant(key, value, f"constants.{key}")
1039+
1040+
def _validate_single_constant(self, key: str, value: any, context: str) -> None:
1041+
"""Validate a single constant value."""
1042+
if key in _URL_CONSTANTS:
1043+
if not isinstance(value, str):
1044+
self.errors.append(f"'{context}' must be a string URL, got {type(value).__name__}")
1045+
return
1046+
try:
1047+
validate_api_url(value, context)
1048+
except InputError as e:
1049+
self.errors.append(str(e))
10551050

10561051

10571052
def _get_config_fields(config: dict) -> list[tuple[dict, str, str, bool, bool]]:
@@ -1081,20 +1076,12 @@ def _get_config_fields(config: dict) -> list[tuple[dict, str, str, bool, bool]]:
10811076
(config.get("unpublish", {}), "exclude_regex", "unpublish.exclude_regex", False, False),
10821077
(config.get("unpublish", {}), "items_to_include", "unpublish.items_to_include", False, False),
10831078
(config.get("unpublish", {}), "skip", "unpublish.skip", False, False),
1084-
# Top-level sections - optional (debug if missing)
1085-
(config, "features", "features", False, False),
1086-
(config, "constants", "constants", False, False),
1079+
# Top-level sections - optional (warn if missing)
1080+
(config, "features", "features", False, True),
1081+
(config, "constants", "constants", False, True),
10871082
]
10881083

10891084

1090-
def _is_regular_constants_dict(constants_value: dict) -> bool:
1091-
"""Check if constants section is a regular dict (not environment mapping)."""
1092-
if not isinstance(constants_value, dict) or not constants_value:
1093-
return True
1094-
# Environment mapping if ALL values are dicts, regular dict otherwise
1095-
return not all(isinstance(value, dict) for value in constants_value.values())
1096-
1097-
10981085
def _find_git_root(path: Path) -> Optional[Path]:
10991086
"""Find the git repository root for a given path."""
11001087
current = path.resolve()

src/fabric_cicd/publish.py

Lines changed: 37 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
import fabric_cicd._items as items
1313
from fabric_cicd import constants
1414
from fabric_cicd._common._config_utils import (
15-
apply_config_overrides,
15+
config_overrides_scope,
1616
extract_publish_settings,
1717
extract_unpublish_settings,
1818
extract_workspace_settings,
@@ -405,42 +405,41 @@ def deploy_with_config(
405405
unpublish_settings = extract_unpublish_settings(config, environment)
406406

407407
# Apply feature flags and constants if specified
408-
apply_config_overrides(config, environment)
409-
410-
# Create FabricWorkspace object with extracted settings
411-
workspace = FabricWorkspace(
412-
repository_directory=workspace_settings["repository_directory"],
413-
item_type_in_scope=workspace_settings.get("item_types_in_scope"),
414-
environment=environment,
415-
workspace_id=workspace_settings.get("workspace_id"),
416-
workspace_name=workspace_settings.get("workspace_name"),
417-
token_credential=token_credential,
418-
parameter_file_path=workspace_settings.get("parameter_file_path"),
419-
)
420-
# Execute deployment operations based on skip settings
421-
if not publish_settings.get("skip", False):
422-
publish_all_items(
423-
workspace,
424-
item_name_exclude_regex=publish_settings.get("exclude_regex"),
425-
folder_path_exclude_regex=publish_settings.get("folder_exclude_regex"),
426-
folder_path_to_include=publish_settings.get("folder_path_to_include"),
427-
items_to_include=publish_settings.get("items_to_include"),
428-
shortcut_exclude_regex=publish_settings.get("shortcut_exclude_regex"),
408+
with config_overrides_scope(config, environment):
409+
# Create FabricWorkspace object with extracted settings
410+
workspace = FabricWorkspace(
411+
repository_directory=workspace_settings["repository_directory"],
412+
item_type_in_scope=workspace_settings.get("item_types_in_scope"),
413+
environment=environment,
414+
workspace_id=workspace_settings.get("workspace_id"),
415+
workspace_name=workspace_settings.get("workspace_name"),
416+
token_credential=token_credential,
417+
parameter_file_path=workspace_settings.get("parameter_file_path"),
429418
)
430-
else:
431-
logger.info(f"Skipping publish operation for environment '{environment}'")
432-
433-
if not unpublish_settings.get("skip", False):
434-
unpublish_all_orphan_items(
435-
workspace,
436-
item_name_exclude_regex=unpublish_settings.get("exclude_regex", "^$"),
437-
items_to_include=unpublish_settings.get("items_to_include"),
419+
# Execute deployment operations based on skip settings
420+
if not publish_settings.get("skip", False):
421+
publish_all_items(
422+
workspace,
423+
item_name_exclude_regex=publish_settings.get("exclude_regex"),
424+
folder_path_exclude_regex=publish_settings.get("folder_exclude_regex"),
425+
folder_path_to_include=publish_settings.get("folder_path_to_include"),
426+
items_to_include=publish_settings.get("items_to_include"),
427+
shortcut_exclude_regex=publish_settings.get("shortcut_exclude_regex"),
428+
)
429+
else:
430+
logger.info(f"Skipping publish operation for environment '{environment}'")
431+
432+
if not unpublish_settings.get("skip", False):
433+
unpublish_all_orphan_items(
434+
workspace,
435+
item_name_exclude_regex=unpublish_settings.get("exclude_regex", "^$"),
436+
items_to_include=unpublish_settings.get("items_to_include"),
437+
)
438+
else:
439+
logger.info(f"Skipping unpublish operation for environment '{environment}'")
440+
441+
logger.info("Config-based deployment completed successfully")
442+
return DeploymentResult(
443+
status=DeploymentStatus.COMPLETED,
444+
message="Deployment completed successfully",
438445
)
439-
else:
440-
logger.info(f"Skipping unpublish operation for environment '{environment}'")
441-
442-
logger.info("Config-based deployment completed successfully")
443-
return DeploymentResult(
444-
status=DeploymentStatus.COMPLETED,
445-
message="Deployment completed successfully",
446-
)

0 commit comments

Comments
 (0)