Skip to content

App Config Provider - Load Order Change #42905

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 38 commits into from
Sep 26, 2025
Merged

App Config Provider - Load Order Change #42905

merged 38 commits into from
Sep 26, 2025

Conversation

mrm9084
Copy link
Member

@mrm9084 mrm9084 commented Sep 8, 2025 •
edited
Loading

Description

This should change nothing in how the provider operates.

  • Moves from loading configurations, then processing them, loading feature flags, then processing them. To loading Configs then feature flags, then processing everything all at once. This is needed mainly for two other future changes with Snapshots and Configuration Mapping.
  • Moves Async tests into their own folder.
  • Refactors the code a bit so there is more code shared between the asynchronous and sync versions.

@Copilot Copilot AI review requested due to automatic review settings September 8, 2025 21:27
Copilot

This comment was marked as outdated.

Sign in to view

mrm9084 and others added 6 commits September 8, 2025 14:31
@mrm9084 @Copilot
Update sdk/appconfiguration/azure-appconfiguration-provider/azure/app…
499064f 
…configuration/provider/_azureappconfigurationproviderbase.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@mrm9084
Refresh re-work
eebae5f
@mrm9084
Updating kv processing
cfa65cb
@mrm9084
reducing code reuse
c35b225
@mrm9084
small updates
6f1fbff
@mrm9084
Update assets.json
7ad361d
@mrm9084 mrm9084 requested a review from Copilot September 9, 2025 21:00
Copilot
Copilot AI reviewed Sep 9, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a 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 refactors the Azure App Configuration Provider to change the loading order from processing configurations and feature flags separately to loading everything first, then processing all at once. This architectural change supports future features like Snapshots and Configuration Mapping.

  • Changes loading order to batch configurations and feature flags before processing
  • Moves async tests into a dedicated folder structure
  • Consolidates shared code between sync and async versions by extracting common functionality to the base class

Reviewed Changes

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

Show a summary per file
File Description
_azureappconfigurationproviderasync.py Main async provider implementation with refactored loading and refresh logic
_async_client_manager.py Async client manager with simplified feature flag handling and fixed dataclass initialization
_client_manager_base.py Moved shared functionality to base class, simplified interface
_client_manager.py Sync client manager mirroring async changes
_azureappconfigurationproviderbase.py Enhanced base class with shared utility functions and feature flag processing
_azureappconfigurationprovider.py Sync provider implementation using shared base functionality
assets.json Updated asset tag reference

jimmyca15
jimmyca15 reviewed Sep 26, 2025
View reviewed changes
Copy link
Member

@jimmyca15 jimmyca15 left a comment

Choose a reason for hiding this comment

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

.

@mrm9084 mrm9084 requested a review from Copilot September 26, 2025 22:36
Copilot
Copilot AI reviewed Sep 26, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

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 9 out of 9 changed files in this pull request and generated 4 comments.

@mrm9084 mrm9084 enabled auto-merge (squash) September 26, 2025 23:13
@mrm9084 mrm9084 merged commit 2227417 into Azure:main Sep 26, 2025
19 checks passed
@mrm9084 mrm9084 deleted the LoadAllThenProcess branch September 26, 2025 23:30
albertofori
albertofori reviewed Oct 1, 2025
View reviewed changes
...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py
for config in configuration_settings:
if isinstance(config, FeatureFlagConfigurationSetting):
feature_flags_processed = []
for settings in configuration_settings:
Copy link
Member

Choose a reason for hiding this comment

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

nit: setting instead of settings

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py
configuration_settings_processed = {}
for config in configuration_settings:
if isinstance(config, FeatureFlagConfigurationSetting):
feature_flags_processed = []
Copy link
Member

Choose a reason for hiding this comment

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

feature_flags_processed is populated but not used anywhere. Does this also mean we ignore all feature flags that are fetched with key-value setting selectors are ignored/filtered out?

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py
feature_flags_processed = []
for settings in configuration_settings:
if isinstance(settings, FeatureFlagConfigurationSetting):
# Feature flags are not processed like other settings
Copy link
Member

Choose a reason for hiding this comment

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

The comment says feature flags are not processed yet they seem to be processed though not used?

...-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py
)
settings_refreshed = True

if self._feature_flag_refresh_enabled and self._feature_flag_refresh_timer.needs_refresh():
Copy link
Member

Choose a reason for hiding this comment

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

Should refresh be happening if self._Feature_flag_enabled is false?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rossgrambo rossgrambo rossgrambo approved these changes

@albertofori albertofori albertofori left review comments

Copilot code review Copilot Copilot left review comments

@jimmyca15 jimmyca15 jimmyca15 approved these changes

@avanigupta avanigupta Awaiting requested review from avanigupta avanigupta is a code owner

Assignees
No one assigned
Labels
App Configuration Provider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mrm9084 @jimmyca15 @rossgrambo @albertofori