Updating App Config test resources - #48769
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Updates App Configuration test resources to use RBAC-only authentication and tolerate role-assignment propagation delays.
Changes:
- Modernizes the ARM template and removes unused connection-string/key resources.
- Adds live-test readiness checks with bounded exponential backoff.
- Adds unit coverage for success, retry, error, and timeout paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
sdk/appconfiguration/test-resources.json |
Updates resource provisioning and outputs. |
sdk/appconfiguration/azure-appconfiguration/tests/conftest.py |
Adds RBAC readiness fixture. |
sdk/appconfiguration/azure-appconfiguration/tests/test_readiness.py |
Tests readiness behavior. |
sdk/appconfiguration/azure-appconfiguration-provider/tests/conftest.py |
Orders setup after readiness and uses test credentials. |
sdk/appconfiguration/azure-appconfiguration-provider/tests/test_readiness.py |
Tests provider readiness behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
Co-authored-by: Yuan Qu <yuanqu@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
sdk/appconfiguration/test-resources.json:143
- Removing this output breaks the service's live sample job.
sdk/appconfiguration/tests.yml:16-20enables sample execution, the sample runner executes every non-ignored.pyfile (eng/tools/azure-sdk-tools/azpysdk/samples.py:256-287), and samples such asazure-appconfiguration/samples/hello_world_sample.py:29andazure-appconfiguration-provider/samples/connection_string_sample.py:11accessAPPCONFIGURATION_CONNECTION_STRINGdirectly. They will now exit withKeyErrorbefore testing anything. Either continue provisioning a usable connection string (and local-auth access) or migrate/skip all connection-string samples in the live job.
"APPCONFIGURATION_ENDPOINT_STRING": {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
sdk/appconfiguration/azure-appconfiguration/tests/test_readiness.py:6
conftest.pydefines_wait_for_rbac_propagation, not_wait_for_data_plane_access, so this module raisesImportErrorduring collection and none of these tests run. Import the actual helper (the alias keeps the existing test call sites intact).
from conftest import _wait_for_data_plane_access
sdk/appconfiguration/azure-appconfiguration-provider/tests/test_readiness.py:6
conftest.pydefines_wait_for_rbac_propagation, not_wait_for_data_plane_access, so this module raisesImportErrorduring collection and none of these tests run. Import the actual helper (the alias keeps the existing test call sites intact).
from conftest import _wait_for_data_plane_access
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
sdk/appconfiguration/test-resources.json:80
- The removed connection-string output is still required by live sample validation:
sdk/appconfiguration/tests.ymlenablesTestSamples, and unignored samples such asazure-appconfiguration-provider/samples/connection_string_sample.pyandazure-appconfiguration/samples/hello_world_sample.pyreadAPPCONFIGURATION_CONNECTION_STRINGdirectly. Because deployment outputs are what populate these pipeline variables, those samples will now fail withKeyError; retain a usable connection-string resource/output or migrate/exclude every affected sample in the same change.
"disableLocalAuth": true,
"dataPlaneProxy": {
"authenticationMode": "Pass-through",
"privateLinkDelegation": "Disabled"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 9 comments.
Suppressed comments (2)
sdk/appconfiguration/azure-appconfiguration/tests/test_readiness.py:11
- This import references no symbol in this package's
conftest.py(the helper is named_wait_for_rbac_propagation), so pytest will fail while collecting this new test module. Import the actual helper under the test's local name.
from conftest import _wait_for_data_plane_access
sdk/appconfiguration/azure-appconfiguration-provider/tests/test_readiness.py:11
- This import references no symbol in this package's
conftest.py(the helper is named_wait_for_rbac_propagation), so pytest will fail while collecting this new test module. Import the actual helper under the test's local name.
from conftest import _wait_for_data_plane_access
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sdk/appconfiguration/azure-appconfiguration-provider/tests/test_readiness.py:11
- This imports a symbol that
conftest.pydoes not define, so pytest fails while collecting this new test module and none of these readiness tests run. Import the actual helper (renaming the test references would also work).
from conftest import _wait_for_data_plane_access
There was a problem hiding this comment.
🔵 Needs a closer look
Newly introduced credentials and clients are not consistently cleaned up or protected against exception paths.
Review details
Suppressed comments (22)
Previously missed (22) — in code that hasn't changed since the last review.
sdk/appconfiguration/azure-appconfiguration-provider/tests/conftest.py:70
- The credential returned here is discarded, so closing the App Configuration client does not close the credential's own transport. In live runs this can leave the identity HTTP session open; manage both objects with context managers.
client = AzureAppConfigurationClient(endpoint, get_credential())
sdk/appconfiguration/azure-appconfiguration/samples/conditional_operation_sample.py:32
- The newly created credential cannot be closed because no reference is retained, and this client is also never closed. Manage both with context managers so authentication and client transports are released even when an operation raises.
client = AzureAppConfigurationClient(endpoint, DefaultAzureCredential())
sdk/appconfiguration/azure-appconfiguration/samples/conditional_operation_sample_async.py:35
- Credential cleanup occurs only after every service call succeeds, and this client is never closed. Any return or exception (including the early return below) leaves aio transports open; manage both the credential and client with async context managers or a
try/finally.
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
sdk/appconfiguration/azure-appconfiguration/samples/hello_world_sample.py:33
- The newly created credential cannot be closed because no reference is retained, and this client is also never closed. Manage both with context managers so authentication and client transports are released even when an operation raises.
client = AzureAppConfigurationClient(endpoint, DefaultAzureCredential())
sdk/appconfiguration/azure-appconfiguration/samples/hello_world_sample_async.py:35
- Credential cleanup occurs only after every service call succeeds, and this client is never closed. An exception leaves both aio transports open; manage the credential and client with async context managers or a
try/finally.
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
sdk/appconfiguration/azure-appconfiguration/samples/list_configuration_settings_sample.py:30
- The newly created credential cannot be closed because no reference is retained, and this client is also never closed. Manage both with context managers so authentication and client transports are released even when an operation raises.
client = AzureAppConfigurationClient(endpoint, DefaultAzureCredential())
sdk/appconfiguration/azure-appconfiguration/samples/list_configuration_settings_sample_async.py:33
- Credential cleanup occurs only after every service call succeeds, and this client is never closed. An exception leaves both aio transports open; manage the credential and client with async context managers or a
try/finally.
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
sdk/appconfiguration/azure-appconfiguration/samples/list_labels_sample.py:30
- The newly created credential cannot be closed because no reference is retained, and this client is also never closed. Manage both with context managers so authentication and client transports are released even when an operation raises.
client = AzureAppConfigurationClient(endpoint, DefaultAzureCredential())
sdk/appconfiguration/azure-appconfiguration/samples/list_labels_sample_async.py:33
- Credential cleanup occurs only after every service call succeeds, and this client is never closed. An exception leaves both aio transports open; manage the credential and client with async context managers or a
try/finally.
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
sdk/appconfiguration/azure-appconfiguration/samples/list_revision_sample.py:30
- The newly created credential cannot be closed because no reference is retained, and this client is also never closed. Manage both with context managers so authentication and client transports are released even when an operation raises.
client = AzureAppConfigurationClient(endpoint, DefaultAzureCredential())
sdk/appconfiguration/azure-appconfiguration/samples/list_revision_sample_async.py:33
- Credential cleanup occurs only after every service call succeeds, and this client is never closed. An exception leaves both aio transports open; manage the credential and client with async context managers or a
try/finally.
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
sdk/appconfiguration/azure-appconfiguration/samples/read_only_sample.py:30
- The newly created credential cannot be closed because no reference is retained, and this client is also never closed. Manage both with context managers so authentication and client transports are released even when an operation raises.
client = AzureAppConfigurationClient(endpoint, DefaultAzureCredential())
sdk/appconfiguration/azure-appconfiguration/samples/read_only_sample_async.py:33
- Credential cleanup occurs only after every service call succeeds, and this client is never closed. An exception leaves both aio transports open; manage the credential and client with async context managers or a
try/finally.
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
sdk/appconfiguration/azure-appconfiguration/samples/send_request_sample.py:30
- Constructing
DefaultAzureCredentialinline means its transport can never be closed; closing the App Configuration client does not close a separately supplied credential. Include the credential in the context manager so both resources are released.
with AzureAppConfigurationClient(endpoint, DefaultAzureCredential()) as client:
sdk/appconfiguration/azure-appconfiguration/samples/send_request_sample_async.py:39
- The credential is closed only after the request succeeds. If client construction, authentication, or the request raises, the aio credential transport remains open; include
DefaultAzureCredentialin the async context manager.
credential = DefaultAzureCredential()
async with AzureAppConfigurationClient(endpoint, credential) as client:
sdk/appconfiguration/azure-appconfiguration/samples/snapshot_sample.py:33
- Constructing
DefaultAzureCredentialinline means its transport can never be closed; closing the App Configuration client does not close a separately supplied credential. Include the credential in the context manager so both resources are released.
with AzureAppConfigurationClient(endpoint, DefaultAzureCredential()) as client:
sdk/appconfiguration/azure-appconfiguration/samples/snapshot_sample_async.py:36
- The credential is closed only after every snapshot operation succeeds. Any exception inside the client context skips
credential.close()and leaves its aio transport open; manageDefaultAzureCredentialwith an async context manager ortry/finally.
credential = DefaultAzureCredential()
config_setting1 = ConfigurationSetting(key="my_key1", label="my_label1")
config_setting2 = ConfigurationSetting(key="my_key1", label="my_label2")
snapshot_name = str(uuid4())
async with AzureAppConfigurationClient(endpoint, credential) as client:
sdk/appconfiguration/azure-appconfiguration/samples/sync_token_sample.py:32
- Constructing
DefaultAzureCredentialinline means its transport can never be closed; closing the App Configuration client does not close a separately supplied credential. Include the credential in the context manager so both resources are released.
with AzureAppConfigurationClient(endpoint, DefaultAzureCredential()) as client:
sdk/appconfiguration/azure-appconfiguration/samples/sync_token_sample_async.py:33
- The credential is closed only after the entire event loop body succeeds. If token acquisition or a service request raises, its aio transport remains open; include
DefaultAzureCredentialin the async context manager so cleanup is exception-safe.
credential = DefaultAzureCredential()
all_keys = []
async with AzureAppConfigurationClient(endpoint, credential) as client:
sdk/appconfiguration/azure-appconfiguration/tests/conftest.py:86
- The credential returned here is discarded, so closing the App Configuration client does not close the credential's own transport. In live runs this can leave the identity HTTP session open; manage both objects with context managers.
client = AzureAppConfigurationClient(endpoint, get_credential())
sdk/appconfiguration/azure-appconfiguration-provider/README.md:26
- This recommended authentication example creates a credential without ever closing it. Since the provider does not own a separately supplied credential, demonstrate a context manager or explicit
credential.close()so readers do not leak the identity transport.
endpoint = os.environ["APPCONFIGURATION_ENDPOINT_STRING"]
credential = DefaultAzureCredential()
# Connecting to Azure App Configuration using Entra ID
config = load(endpoint=endpoint, credential=credential, **kwargs)
sdk/appconfiguration/azure-appconfiguration/README.md:387
- This new authentication example creates both an identity credential and an async client without showing either being closed. Because async credentials own transport sessions, copying this standalone snippet can produce unclosed-session warnings; demonstrate
async withfor both resources or include explicit cleanup.
endpoint = os.environ["APPCONFIGURATION_ENDPOINT_STRING"]
credential = DefaultAzureCredential()
# Create an app config client
client = AzureAppConfigurationClient(endpoint, credential)
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The revised sanitizer can leave a live Key Vault URL unsanitized in recorded response bodies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 1
- Review effort level: Balanced
| target = target.rstrip("/") + "/" | ||
| value = value.rstrip("/") + "/" | ||
| add_uri_string_sanitizer(target=target, value=value) | ||
| add_general_string_sanitizer(target=target, value=value) |
There was a problem hiding this comment.
🟡 Changes recommended
Key Vault URLs may remain unsanitized, and async playback retries are not patched to fail fast.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
sdk/appconfiguration/azure-appconfiguration-provider/tests/conftest.py:133
- The template outputs Key Vault secret URLs without a trailing slash, and
setup_configsstores those exact values in App Configuration. Mutatingtargetto add/before registering the general string sanitizer means the URL embedded in recorded request/response bodies no longer matches, so the real vault and secret URL can remain in the recording. Keep the original target for the general sanitizer and normalize only the URI sanitizer target.
for target, value in key_vault_references:
target = target.rstrip("/") + "/"
value = value.rstrip("/") + "/"
add_uri_string_sanitizer(target=target, value=value)
add_general_string_sanitizer(target=target, value=value)
- Files reviewed: 31/31 changed files
- Comments generated: 1
- Review effort level: Balanced
| monkeypatch.setattr( | ||
| "azure.appconfiguration.provider._azureappconfigurationprovider.get_startup_backoff", | ||
| lambda *args, **kwargs: (0, False), | ||
| lambda *args, **kwargs: (float("inf"), False), | ||
| ) |
Description
Updates the test structure of the python libraries.