-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fixing Feature Flag Snapshot #44904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fixing Feature Flag Snapshot #44904
Conversation
There was a problem hiding this comment.
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 fixes two bugs in the Azure App Configuration Provider related to feature flags and snapshots:
- Handling
Nonevalues forfeature_flag_selectorsparameter - Adding snapshot support to the
load_feature_flagsmethod
Changes:
- Fixed
feature_flag_selectors=Nonecausing exceptions by adding a null check in initialization - Added snapshot support to the sync
load_feature_flagsmethod in_client_manager.py - Added comprehensive test coverage for snapshot feature flag handling
- Added helper functions for test resource management and snapshot creation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationproviderbase.py | Added null check for feature_flag_selectors to prevent exceptions when set to None |
| sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_client_manager.py | Added snapshot support to load_feature_flags method and added configurations variable initialization |
| sdk/appconfiguration/azure-appconfiguration-provider/tests/testcase.py | Added helper functions for test cleanup, settings setup, and snapshot creation |
| sdk/appconfiguration/azure-appconfiguration-provider/tests/asynctestcase.py | Added async versions of test helper functions |
| sdk/appconfiguration/azure-appconfiguration-provider/tests/test_snapshots.py | Enhanced tests for snapshot functionality with feature flags and refactored cleanup code |
| sdk/appconfiguration/azure-appconfiguration-provider/tests/aio/test_async_snapshots.py | Enhanced async tests for snapshot functionality with feature flags |
| sdk/appconfiguration/azure-appconfiguration-provider/samples/aad_sample.py | Demonstrated usage of feature_flag_selectors=None parameter |
sdk/appconfiguration/azure-appconfiguration-provider/samples/aad_sample.py
Outdated
Show resolved
Hide resolved
…ad_sample.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
sdk/appconfiguration/azure-appconfiguration-provider/tests/aio/test_async_snapshots.py
Outdated
Show resolved
Hide resolved
...iguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_client_manager.py
Outdated
Show resolved
Hide resolved
| if self._feature_flag_selectors is None: | ||
| self._feature_flag_selectors = [SettingSelector(key_filter="*")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesnt the previous line cover this case already? How can self._feature_flag_selectors be None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if someone passes in kwargs and it's kwargs['feature_flag_selectors']=None it results in an error without this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesnt kwargs.pop("feature_flag_selectors", [SettingSelector(key_filter="*")]) handle the case where if feature_flag_selectors is None, then we use a default list of SettingSelector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that only handles the case where the item doesn't exist in the dictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, defaulting to None when the key isn’t present in kwargs makes the intent clearer, since the next line already checks for None. It also ensures the default value is defined in only one place.
self._feature_flag_selectors = kwargs.pop("feature_flag_selectors", None)
if self._feature_flag_selectors is None:
self._feature_flag_selectors = [SettingSelector(key_filter="*")]
...iguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_client_manager.py
Show resolved
Hide resolved
Co-authored-by: Avani Gupta <avanigupta@users.noreply.github.com>
| async for feature_flag in feature_flags: | ||
| if isinstance(feature_flag, FeatureFlagConfigurationSetting): | ||
| loaded_feature_flags.append(feature_flag) | ||
| if not isinstance(feature_flag, FeatureFlagConfigurationSetting): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if block is not needed
| for feature_flag in feature_flags: | ||
| if isinstance(feature_flag, FeatureFlagConfigurationSetting): | ||
| loaded_feature_flags.append(feature_flag) | ||
| if not isinstance(feature_flag, FeatureFlagConfigurationSetting): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, not needed
Description
Fixes for feature Flags:
Nonewhich would cause an exceptionfeature_flag_selects, it actually resulted in loading all feature flags with the null label.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines