Conversation
d07e3d1 to
a047276
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds "Snapshot Reference" support to the Azure App Configuration Go provider. Snapshot references allow a key-value setting stored in App Configuration to point to a named snapshot — when loaded, the settings from that snapshot are merged into the configuration. The implementation refactors the existing snapshot loading into a shared helper function, adds content-type detection for snapshot references, and emits a tracing tag when they are used.
Changes:
- Extracts snapshot loading into a reusable
loadSnapshotSettingsfunction and addssnapshotSettingsLoaderfunction type to support snapshot reference resolution with dependency injection. - Adds detection of
snapshotReferenceContentTypeinloadKeyValues, collects snapshot references, and resolves them via the newloadSettingsFromSnapshotRefsmethod. - Adds
UseSnapshotReferencetracing option andSnapshotReferenceTagcorrelation context tag.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
azureappconfiguration/azureappconfiguration.go |
Core implementation: detection of snapshot reference content type, loadSettingsFromSnapshotRefs, and parseSnapshotReference |
azureappconfiguration/settings_client.go |
Extracts snapshot loading into loadSnapshotSettings helper; adds snapshotSettingsLoader type |
azureappconfiguration/internal/tracing/tracing.go |
Adds SnapshotReferenceTag and UseSnapshotReference option to correlation context header |
azureappconfiguration/constants.go |
Adds snapshotReferenceContentType constant |
azureappconfiguration/options.go |
Minor whitespace fix |
azureappconfiguration/snapshot_test.go |
Adds unit tests for parseSnapshotReference and loadSettingsFromSnapshotRefs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| var respErr *azcore.ResponseError | ||
| if errors.As(err, &respErr) && respErr.StatusCode == 404 { | ||
| return settings, nil // treat non-existing snapshot as empty |
There was a problem hiding this comment.
I just realized that we introduced a breaking change when supporting snapshot reference for .net provider. This applies for all providers since they all use .net provider as blueprint.
In PR, we added a helper method to load configuration settings from snapshot and we will swallow the 404 if snapshot is not found. There are two code paths now: 1. select/load a snapshot (hard-coded in the code) 2. use a snapshot reference
Previously, if user selected a snapshot and that snapshot is not found, we will throw. Now this behavior change.
@jimmyca15 @avanigupta Is this behavior change intended?
There was a problem hiding this comment.
@zhiyuanliang-ms I believe this was intentional. We discussed this in my python PR as originally, I didn't catch the error: Azure/azure-sdk-for-python#44116 (comment)
There was a problem hiding this comment.
Is this behavior change intended?
Yes.
I just realized that we introduced a breaking change
While I agree there was a change, I'm not sure I would qualify it as breaking. A consumer would have had to depend on exceptions for missing snapshots.
ref