Skip to content

Updating App Config test resources - #48769

Open
Matthew Metcalf (mrm9084) wants to merge 10 commits into
Azure:mainfrom
mrm9084:FixArmTemplate
Open

Updating App Config test resources#48769
Matthew Metcalf (mrm9084) wants to merge 10 commits into
Azure:mainfrom
mrm9084:FixArmTemplate

Conversation

@mrm9084

Copy link
Copy Markdown
Member

Description

Updates the test structure of the python libraries.

  • Removed the unused connection string
  • Updated api version in template
  • Removed unused keys, we make them in the tests
  • Added a check at the start of testing to make sure the role has propagated to the resource before testing starts.

Copilot AI balanced review requested due to automatic review settings August 26, 2026 20:06
@github-actions github-actions Bot added the App Configuration Azure.ApplicationModel.Configuration label Aug 26, 2026
@azure-pipelines

Copy link
Copy Markdown
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copilot AI commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

One or more custom setup steps configured for this repository failed during this Copilot code review run:

Install azsdk mcp server

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 copilot-code-review.yml file. Read the docs for details.

Comment thread sdk/appconfiguration/azure-appconfiguration-provider/tests/conftest.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration-provider/tests/conftest.py Outdated
Copilot AI review requested due to automatic review settings August 28, 2026 17:36
Co-authored-by: Yuan Qu <yuanqu@microsoft.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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-20 enables sample execution, the sample runner executes every non-ignored .py file (eng/tools/azure-sdk-tools/azpysdk/samples.py:256-287), and samples such as azure-appconfiguration/samples/hello_world_sample.py:29 and azure-appconfiguration-provider/samples/connection_string_sample.py:11 access APPCONFIGURATION_CONNECTION_STRING directly. They will now exit with KeyError before 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": {

Comment thread sdk/appconfiguration/test-resources.json Outdated
Copilot AI review requested due to automatic review settings August 28, 2026 17:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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.py defines _wait_for_rbac_propagation, not _wait_for_data_plane_access, so this module raises ImportError during 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.py defines _wait_for_rbac_propagation, not _wait_for_data_plane_access, so this module raises ImportError during 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

Comment thread sdk/appconfiguration/azure-appconfiguration/tests/test_readiness.py
Comment thread sdk/appconfiguration/test-resources.json
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 31, 2026 17:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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.yml enables TestSamples, and unignored samples such as azure-appconfiguration-provider/samples/connection_string_sample.py and azure-appconfiguration/samples/hello_world_sample.py read APPCONFIGURATION_CONNECTION_STRING directly. Because deployment outputs are what populate these pipeline variables, those samples will now fail with KeyError; 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"

Comment thread sdk/appconfiguration/azure-appconfiguration/tests/test_readiness.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration-provider/tests/test_readiness.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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

Comment thread sdk/appconfiguration/azure-appconfiguration/samples/sync_token_sample_async.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration/samples/snapshot_sample_async.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration/samples/send_request_sample_async.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration/samples/read_only_sample_async.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration/samples/list_revision_sample_async.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration/samples/list_labels_sample_async.py Outdated
Comment thread sdk/appconfiguration/azure-appconfiguration/samples/hello_world_sample_async.py Outdated
Copilot AI review requested due to automatic review settings August 31, 2026 18:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 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.py does 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

Copilot AI review requested due to automatic review settings September 1, 2026 19:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 DefaultAzureCredential inline 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 DefaultAzureCredential in the async context manager.
    credential = DefaultAzureCredential()
    async with AzureAppConfigurationClient(endpoint, credential) as client:

sdk/appconfiguration/azure-appconfiguration/samples/snapshot_sample.py:33

  • Constructing DefaultAzureCredential inline 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; manage DefaultAzureCredential with an async context manager or try/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 DefaultAzureCredential inline 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 DefaultAzureCredential in 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 with for 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

Copilot AI review requested due to automatic review settings September 1, 2026 21:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment on lines +130 to +133
target = target.rstrip("/") + "/"
value = value.rstrip("/") + "/"
add_uri_string_sanitizer(target=target, value=value)
add_general_string_sanitizer(target=target, value=value)
Copilot AI review requested due to automatic review settings September 1, 2026 23:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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_configs stores those exact values in App Configuration. Mutating target to 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

Comment on lines 152 to 155
monkeypatch.setattr(
"azure.appconfiguration.provider._azureappconfigurationprovider.get_startup_backoff",
lambda *args, **kwargs: (0, False),
lambda *args, **kwargs: (float("inf"), False),
)
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.

3 participants