TypeSpec conversion for resources/policy#38509
Conversation
…anagementGroupName}/providers/microsoft.Authorization
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.Comment generated by summarize-checks workflow run. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
| * The parameter values for the assigned policy rule. The keys are the parameter names. | ||
| */ | ||
| #suppress "@azure-tools/typespec-azure-resource-manager/arm-no-record" "For backward compatibility" | ||
| parameters?: ParameterValues; |
There was a problem hiding this comment.
https://github.com/Azure/azure-rest-api-specs-pr/blob/main/specification/resources/resource-manager/Microsoft.Authorization/policy/stable/2025-03-01/policyAssignments.json#L775
https://github.com/Azure/azure-rest-api-specs-pr/blob/main/specification/resources/resource-manager/Microsoft.Authorization/policy/stable/2025-03-01/policyAssignments.json#L881

It is not consistent with the definition in swagger, and this cause go breaking change like - Type of AssignmentProperties.Parametershas been changed frommap[string]*ParameterValuesValueto*ParameterValues``,and there are several similar parameters changes.
Is this designed?
There was a problem hiding this comment.
I assume that
"ParameterValues": {
"type": "object",
"additionalProperties": {
"$ref": "#/definitions/ParameterValuesValue"
},
"description": "The parameter values for the policy rule. The keys are the parameter names."
},
means ParameterValues is a dict of ParameterValuesValue.
Hence in TypeSpec
model ParameterValues is Record<ParameterValuesValue>;
then
parameters?: ParameterValues;
@jliusan
Do you prefer we write it as
parameters?: Record<ParameterValuesValue>;
(without the ParameterValues model)?
There was a problem hiding this comment.
From the Swagger, I think the current way of writing be same (that is the reason why I wrote it this way, with a ParameterValues).
The Swagger generated from TypeSpec
There was a problem hiding this comment.
I assume that
"ParameterValues": { "type": "object", "additionalProperties": { "$ref": "#/definitions/ParameterValuesValue" }, "description": "The parameter values for the policy rule. The keys are the parameter names." },means
ParameterValuesis a dict ofParameterValuesValue.Hence in TypeSpec
model ParameterValues is Record<ParameterValuesValue>;then
parameters?: ParameterValues;@jliusan Do you prefer we write it as
parameters?: Record<ParameterValuesValue>;(without the
ParameterValuesmodel)?
Yes, go prefers parameters?: Record<ParameterValuesValue>;
There was a problem hiding this comment.
Changed in bc62110
SDK likely won't have break from the diff on Swagger (caused by this change). As least for Java from m4, there is no ParameterValues class generated from Swagger.
|
/azp run SDK Validation - Go |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| @armResourceOperations | ||
| interface PolicyAssignments { | ||
| /** |
There was a problem hiding this comment.
Go has breaking change like these, and there are no ops named as these in tsp file but exist in swagger on main branch, are these funcs removed by designed ?
- Function `*AssignmentsClient.CreateByID` has been removed
- Function `*AssignmentsClient.DeleteByID` has been removed
- Function `*AssignmentsClient.GetByID` has been removed
- Function `*AssignmentsClient.UpdateByID` has been removed
There was a problem hiding this comment.
They are moved into client.tsp, because these ops are not the genuine "CRUD" on the resource model, but describing another equivalent route to it (e.g. route {endpoint}/{policyAssignmentId}). Also see PR description.
Chenjie seems fine to remove them from Go SDK.
If you'd like to have them, you can add "go" scope to these ops in client.tsp (currently it be @scope("java"))
Also note for #38509 (comment) on the get op
Difference in Swagger
/{policyAssignmentId}is not included <- it is OKed by serviceFor backward compatible on SDK, they are added in "client.tsp" with
@scopex-ms-skip-url-encoding=trueon$filterquery parameter is not includedtruein Swagger. User of the API is unliked to escape it before calling the API.valueof paged response changed to "required"{ "type: "object" }changed to{}(due to TypeSpecunknown)PolicyAssignment,PolicyDefinition,PolicyDefinitionVersion,PolicySetDefinition,PolicySetDefinitionVersionnow has base modelProxyResourceARM (Control Plane) API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.
Additional information
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.
Getting help
Purpose of this PRandDue diligence checklist.write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.