Skip to content

Conversation

@mrm9084
Copy link
Member

@mrm9084 mrm9084 commented Nov 20, 2025

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

Copilot AI review requested due to automatic review settings November 20, 2025 17:11
@github-actions github-actions bot added the App Configuration Azure.ApplicationModel.Configuration label Nov 20, 2025
Copilot finished reviewing on behalf of mrm9084 November 20, 2025 17:15
Copy link
Contributor

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 pull request adds support for snapshot references in the Azure App Configuration Provider SDK for Python. Snapshot references allow configuration settings to reference entire snapshots, which are then resolved and merged into the configuration at load time. This feature enables better configuration management by allowing users to group related settings into snapshots and reference them dynamically.

Key Changes:

  • Introduced snapshot reference parsing and resolution functionality with recursive processing support
  • Added telemetry tracking for snapshot reference usage via the request tracing context
  • Extended both synchronous and asynchronous providers to handle snapshot reference resolution

Reviewed Changes

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

Show a summary per file
File Description
_constants.py Added constants for snapshot reference MIME profile, content type, and telemetry tag
_snapshot_reference_parser.py New parser class for validating and extracting snapshot names from reference JSON
_client_manager.py Added resolve_snapshot_reference method to resolve snapshot references to actual settings
aio/_async_client_manager.py Async version of snapshot reference resolution functionality
_azureappconfigurationprovider.py Modified _process_configurations to detect and resolve snapshot references recursively
aio/_azureappconfigurationproviderasync.py Async version of snapshot reference processing in configuration loading
_azureappconfigurationproviderbase.py Added telemetry tracking for snapshot reference content type detection
_request_tracing_context.py Added uses_snapshot_reference flag and correlation context header support
test_snapshot_references.py Unit tests for snapshot reference parser validation logic
test_snapshot_references_integration.py Integration tests for snapshot reference parsing and content type detection
aio/test_async_snapshot_references_integration.py Async integration tests for snapshot reference resolution in client manager and provider
test_request_tracing_context.py Tests for snapshot reference telemetry tracking in correlation context
refresh_sample.py Unrelated sample modifications that should be reverted

Comment on lines +470 to +473
# Recursively process the resolved snapshot settings to handle feature flags,
# configuration mapping
snapshot_configuration_list = list(snapshot_settings.values())
resolved_settings = await self._process_configurations(snapshot_configuration_list, client)
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The recursive call to _process_configurations on line 473 could lead to infinite recursion if a snapshot contains a snapshot reference that points back to itself or creates a circular reference chain. There's no depth tracking or cycle detection to prevent this.

Consider adding a depth limit parameter or cycle detection mechanism to prevent stack overflow from circular snapshot references.

Copilot uses AI. Check for mistakes.
APP_CONFIG_SNAPSHOT_REF_MIME_PROFILE = "https://azconfig.io/mime-profiles/snapshot-ref"

# Snapshot reference tracing key
SNAPSHOT_REFERENCE_TAG = "UsesSnapshotReference=true"
Copy link
Member

@avanigupta avanigupta Nov 20, 2025

Choose a reason for hiding this comment

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

Snapshot ref is part of the features string. Eg: "Features=AI+SnapshotRef"

# Mime profiles
APP_CONFIG_AI_MIME_PROFILE = "https://azconfig.io/mime-profiles/ai/"
APP_CONFIG_AICC_MIME_PROFILE = "https://azconfig.io/mime-profiles/ai/chat-completion"
APP_CONFIG_SNAPSHOT_REF_MIME_PROFILE = "https://azconfig.io/mime-profiles/snapshot-ref"
Copy link
Member

Choose a reason for hiding this comment

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

No need to track MIME profile - just compare with SNAPSHOT_REF_CONTENT_TYPE to check if it's a snapshot reference. We do the same in .net provider: https://github.com/Azure/AppConfiguration-DotnetProvider/blob/34576ef70d303a029b7ea7ef860f12885fb8f35d/src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs#L869C45-L869C57


if self._feature_flag_refresh_enabled:
self._watched_feature_flags[(settings.key, settings.label)] = settings.etag
elif settings.content_type and SNAPSHOT_REF_CONTENT_TYPE in settings.content_type:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elif settings.content_type and SNAPSHOT_REF_CONTENT_TYPE in settings.content_type:
elif settings.content_type and SNAPSHOT_REF_CONTENT_TYPE == settings.content_type:

APP_CONFIG_AICC_MIME_PROFILE = "https://azconfig.io/mime-profiles/ai/chat-completion"

# Snapshot reference tracing key
SNAPSHOT_REFERENCE_TAG = "UsesSnapshotReference"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SNAPSHOT_REFERENCE_TAG = "UsesSnapshotReference"
SNAPSHOT_REFERENCE_TAG = "SnapshotRef"

# Snapshot reference tracing key
SNAPSHOT_REFERENCE_TAG = "UsesSnapshotReference"

# Snapshot reference constants
Copy link
Member

Choose a reason for hiding this comment

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

This file contains all the constants. Could you organize them into sections so that related constants are grouped together? Eg tracing constants, content types, FM constants.

or empty/whitespace snapshot name
"""
if setting is None:
raise ValueError("Setting cannot be None")
Copy link
Member

Choose a reason for hiding this comment

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

Does ValueError get handled properly if the provider is optional?

if self.is_failover_request:
tags.append(FAILOVER_TAG)

if self.uses_snapshot_reference:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldnt this be part of features_string?

snapshot_selector = SettingSelector(snapshot_name=snapshot_name)

# Use existing load_configuration_settings to load from snapshot
configurations = self.load_configuration_settings([snapshot_selector], **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

If snapshot contains feature flags, will those be ignored?

if APP_CONFIG_AICC_MIME_PROFILE in config.content_type:
self._tracing_context.uses_aicc_configuration = True
if SNAPSHOT_REF_CONTENT_TYPE in config.content_type:
self._tracing_context.uses_snapshot_reference = True
Copy link
Member

Choose a reason for hiding this comment

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

This is already tracked in _process_configurations, do we still need to check content type here?

self._tracing_context.uses_snapshot_reference = True
try:
# Resolve the snapshot reference to actual settings
snapshot_settings = client.resolve_snapshot_reference(settings)
Copy link
Member

Choose a reason for hiding this comment

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

Snapshot references should be resolved before _configuration_mapper is run - i.e. _configuration_mapper runs on all the settings loaded by snapshot or snapshot reference

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

Labels

App Configuration Azure.ApplicationModel.Configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants