Add azure-mgmt-unified-design.md: TypedDict-driven Azure management SDK design document - #7
l0lawrence with Copilot wants to merge 4 commits into
Conversation
…tation Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com>
|
Hi @copilot. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
1 similar comment
|
Hi @copilot. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
There was a problem hiding this comment.
🟡 Changes recommended
Critical implementation issues and an unrelated file remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a TypedDict-driven design for unified Azure management resource managers, including CRUD, paging, LRO, factory, and usage examples.
Changes:
- Adds metadata schemas and VM/Storage Account specifications.
- Adds illustrative manager, factory, and usage implementations.
- Adds the unrelated root-level
agents.mdguide.
File summaries
| File | Findings |
|---|---|
doc/dev/mgmt/azure-mgmt-unified-design.md |
Critical: update is advertised but not implemented (3 votes); LRO polling passes an unsent request (3 votes); PipelineClient uses the wrong import path (1 vote). Moderate: required spec metadata is not enforced (2 votes); synchronous PUT does not wait for LRO completion (1 vote). |
agents.md |
Moderate: Unrelated root-level guide conflicts with the stated PR scope (3 votes). |
Review details
Suppressed comments (11)
doc/dev/mgmt/azure-mgmt-unified-design.md:450
- Although
create_or_updateaccepts anOperationSpec, this implementation only tests its truthiness and then hard-codes the generic URL, PUT method, andmodel_type. TheStorageAccountoverride's URL, parameters, andresponse_modelare therefore ignored; resolve and apply the selected operation specification instead.
url = self._get_base_url(resource_group_name, resource_name)
body = self._serialize.body(parameters, self._spec.get("model_type", ""))
request = self._build_request("PUT", url, json=body, **kwargs)
doc/dev/mgmt/azure-mgmt-unified-design.md:243
- The examples populate
custom_operationswithpower_off,start, andlist_keys, but neitherregisternorAutoResourceManagergenerates or exposes methods for these entries. As written, those advertised operations—and the laterrestartexample—cannot be called through the factory.
"custom_operations": [
{
"name": "power_off",
"http_method": "POST",
"url_template": (
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}"
"/providers/Microsoft.Compute/virtualMachines/{vmName}/powerOff"
),
doc/dev/mgmt/azure-mgmt-unified-design.md:400
- The
scopemetadata is never consulted: this URL builder always requires a resource-group path. A spec declaringsubscription,tenant, orresourcescope will still produce the resource-group URL and method signature, so the documented scope options do not work.
def _get_base_url(self, resource_group_name: str, resource_name: str) -> str:
"""Generate the base URL for a specific resource."""
provider = self._spec.get("provider_namespace", "")
resource_type = self._spec.get("resource_type", "")
return (
f"/subscriptions/{{subscriptionId}}/resourceGroups/{resource_group_name}"
f"/providers/{provider}/{resource_type}/{resource_name}"
doc/dev/mgmt/azure-mgmt-unified-design.md:606
- The request URL is still relative when it is passed directly to the pipeline. ARM operation implementations format requests with
self._client.format_url(...)first; without that step this code sends paths such as/subscriptions/...to the transport instead of an absolute management endpoint.
pipeline_response = self._client._pipeline.run(request, stream=False)
doc/dev/mgmt/azure-mgmt-unified-design.md:610
- Every non-success response is raised as the base
HttpResponseError, so a 404 is never converted toResourceNotFoundError; the error-handling example below therefore cannot catch a missing VM as shown. Apply an ARM error map (at least for 404/401/409/304) before raising.
if response.status_code >= 400:
from azure.core.exceptions import HttpResponseError
raise HttpResponseError(response=response)
doc/dev/mgmt/azure-mgmt-unified-design.md:478
- Only
polling_intervalis read fromLROSpec;polling_method,final_state_via, andtimeoutare never passed toARMPolling. Consequently the Storage Account specification's location final-state behavior and timeout setting have no effect.
lro_spec = self._spec.get("lro", {})
polling_interval = lro_spec.get("polling_interval", 30)
def get_long_running_output(response: HttpResponse) -> ModelType:
return self._deserialize(self._spec.get("model_type", ""), response)
doc/dev/mgmt/azure-mgmt-unified-design.md:536
- The delete LRO has the same initialization bug: the request is never sent and an
HttpRequestis passed whereLROPollerexpects aPipelineResponse, sobegin_deletefails immediately instead of starting the operation.
return LROPoller(
self._client,
request,
lambda _: None,
ARMPolling(polling_interval),
doc/dev/mgmt/azure-mgmt-unified-design.md:530
- This delete path also reads only
polling_interval; the declared polling strategy, final-state mode, and timeout are ignored just as inbegin_create_or_update. The metadata-driven LRO behavior is therefore not implemented for deletes.
lro_spec = self._spec.get("lro", {})
polling_interval = lro_spec.get("polling_interval", 30)
url = self._get_base_url(resource_group_name, resource_name)
request = self._build_request("DELETE", url, **kwargs)
doc/dev/mgmt/azure-mgmt-unified-design.md:772
ARMPipelineClientrequires an ARM configuration object or explicit policies; passingcredentialdirectly without either causes its constructor to raise before the factory is created. Build the service configuration with the credential and pass it asconfigto the client.
client = ARMPipelineClient(
base_url="https://management.azure.com",
credential=credential,
)
doc/dev/mgmt/azure-mgmt-unified-design.md:777
- The usage example passes a plain dictionary as
config, but_build_urldereferencesself._config.subscription_id. The first manager request therefore raisesAttributeError; use the same typed ARM configuration object for both the client and factory, or consistently support mapping access.
config={"subscription_id": subscription_id},
doc/dev/mgmt/azure-mgmt-unified-design.md:452
- Both example resources declare LRO settings, but this synchronous method sends the PUT once and immediately deserializes its initial response. For an ARM 202 response it returns before provisioning completes, so the documented
ModelTyperesult is not reliable; LRO resources should use the polling path or wait here.
url = self._get_base_url(resource_group_name, resource_name)
body = self._serialize.body(parameters, self._spec.get("model_type", ""))
request = self._build_request("PUT", url, json=body, **kwargs)
response = self._send_request(request)
return self._deserialize(self._spec.get("model_type", ""), response)
- Files reviewed: 2/2 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| update: OperationSpec | bool | ||
| """PATCH operation specification or True to auto-generate.""" |
| from typing import Any, Callable, Generic, Iterator, TypeVar | ||
|
|
||
| from azure.core.paging import ItemPaged | ||
| from azure.core.pipeline import PipelineClient |
| return LROPoller( | ||
| self._client, | ||
| request, | ||
| get_long_running_output, | ||
| ARMPolling(polling_interval), | ||
| ) |
| @@ -0,0 +1,379 @@ | |||
| # GitHub Copilot Agent Guide for Azure SDK for Python CI Fixes | |||
| class ResourceSpec(TypedDict, total=False): | ||
| """Complete specification for an Azure resource type.""" |
Description
Adds a design document showcasing a lightweight unified Azure management SDK approach using Python's
TypedDictfor declarative resource specifications with automated CRUD/paging/LRO operation mapping.Document contents:
ResourceSpec,OperationSpec,PagingSpec,LROSpec,CRUDSpec,ParameterSpecAutoResourceManagerbase class implementation with automatic operation generationResourceManagerFactoryfor resource registration and accessKey pattern:
Location:
doc/dev/mgmt/azure-mgmt-unified-design.mdAll SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines
N/A - documentation only, no executable code changes.
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.