-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Batch Mangement Plane 2025-06-01 #35105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Next Steps to Merge✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge. |
PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
Hi @rkmanda , thanks for your notification. In the implementation of Batch, we will create DiskEncryptionSet resources from DiskRP to support the feature of CustomerManagedKey. The REST API of creating DiskEncryptionSet is: https://learn.microsoft.com/en-us/rest/api/compute/disk-encryption-sets/create-or-update?view=rest-compute-2025-02-01&tabs=HTTP. We receive the keyUrl and rotationToLatestKeyVersionEnabled properties from user. After some processing, we will use them with some other values to create DiskEncryptionSet. To keep align, we used almost the same API definition as creating DiskEncryptionSet in our API. As for the structures between our current API and the recommended API definition, I believe they are very similar:
So their structures are very similar, Batch just removed some unnecessary properties. In addition, there are some differences in naming, some of which are to maintain consistency with DiskEncryptionSet. |
Looks like this comment is still applicable. Please address. In reply to: 2954203732 Refers to: specification/batch/resource-manager/Microsoft.Batch/stable/2025-06-01/BatchManagement.json:6543 in 13f5ea6. [](commit_id = 13f5ea6, deletion_comment = False) |
There are few new properties being introduced in this PR. The examples do not reflect the new properties being added. Please update examples also , to have the new properties from this json/apiversion Refers to: specification/batch/resource-manager/Microsoft.Batch/stable/2025-06-01/BatchManagement.json:4757 in 13f5ea6. [](commit_id = 13f5ea6, deletion_comment = False) |
Author added comments in PR In reply to: 3009476962 Refers to: specification/batch/resource-manager/Microsoft.Batch/stable/2025-06-01/BatchManagement.json:6543 in 13f5ea6. [](commit_id = 13f5ea6, deletion_comment = False) |
Hi @ramoka178 , for this comment, please refer to my explanation at #35105 (comment). We think our structures are same as the guidance, and we are following the naming from under layer (DiskRP, https://learn.microsoft.com/en-us/rest/api/compute/disk-encryption-sets/create-or-update?view=rest-compute-2025-02-01&tabs=HTTP). |
Hi team, We are working on the comments currently. But by the way, could you please check our reply 2995366145 on the comment 2954203732? So that we could confirm if we need to change our definitions and avoid reworking. Thanks! |
Hi @ramoka178 , the eanbled parameter is follow Azure VMSS API about proxyagentsettings https://learn.microsoft.com/en-us/rest/api/compute/virtual-machine-scale-sets/create-or-update?view=rest-compute-2025-02-01&tabs=HTTP#proxyagentsettings , and I think for this parameter there is no need to change boolean to enum because no more choice for it. and use enum may make customer misunderstand the mean of |
In this case, I don't think it should be. Some Batch properties are passed through to Virtual Machine Scale Sets, and in these cases, we try to align to their API. ipTagType is one of these properties (https://learn.microsoft.com/en-us/rest/api/compute/virtual-machine-scale-sets/create-or-update?view=rest-compute-2025-02-01&tabs=HTTP#virtualmachinescalesetiptag), and it defines the type as 'string'. Instead of having to track whenever VMSS adds a new type and needing to duplicate the type in Batch before it's usable to customers, we allow this field to be set to any string and delegate the validation to VMSS. |
Just wondering, do you ever interact with this field? Can you actually be sure you'll really support every value that the VMSS RP supports, without adding logic for it in the future? Or is it better to explicitly enumerate the list of values you actually definitely support for this api version, so that customers don't try and send you values you don't definitely support? |
Assuming you can flow parameters to other RPs sounds fine in theory, but in practice it may stop working for you if other RPs ever make breaking changes in their APIs... Not to mention, worries like what is the potential security impact of allowing values to flow through, unchecked and unvalidated? In enum cases, its much more obvious that what can go wrong is limited, than with a string! Bottom line for me: Maybe it is better not to expose the underlying implementation in every case. Maybe you should expose a simplified or more limited API capturing the pit of success. Are you really being strategic about this? |
You removed the documented Default value of 'None', but added "If not specified, the default is none.". Is this intended as a bugfix, a breaking change, or what exactly? Refers to: specification/batch/resource-manager/Microsoft.Batch/stable/2025-06-01/BatchManagement.json:4996 in af615ca. [](commit_id = af615ca, deletion_comment = True) |
doesn't seem like the right title for the mode property. Do you want something like "Mode: audit vs enforce"? Refers to: specification/batch/resource-manager/Microsoft.Batch/stable/2025-06-01/BatchManagement.json:6736 in 13f5ea6. [](commit_id = 13f5ea6, deletion_comment = False) |
Nothing mandatory to fix at this point for me, but please review remaining feedback/questions. |
You should fix or suppress the remaining lintdiff warnings introduced in this PR: |
Since this is a new version, consider adding a provisioningState to your model to fix this historic linter warning
|
Since this is a new version, you should consider fixing this historic issue: ❌ PutRequestResponseSchemeArm | A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'BatchAccount_Create' Request Model: 'parameters[2].schema' Response Model: 'responses[200].schema'Location: Microsoft.Batch/stable/2025-06-01/BatchManagement.json#L38 -- | -- ❌ PutRequestResponseSchemeArm | A PUT operation request body schema should be the same as its 200 response schema, to allow reusing the same entity between GET and PUT. If the schema of the PUT request body is a superset of the GET response body, make sure you have a PATCH operation to make the resource updatable. Operation: 'Certificate_Create' Request Model: 'parameters[3].schema' Response Model: 'responses[200].schema'Location: Microsoft.Batch/stable/2025-06-01/BatchManagement.json#L1262 -- | -- |
Since this is a new api version, consider relaxing some of the 'required' constraints for PATCH requests: ❌ PatchBodyParametersSchema | Properties of a PATCH request body must not be required, property:storageAccountId.Location: Microsoft.Batch/stable/2025-06-01/BatchManagement.json#L146 -- | -- ❌ PatchBodyParametersSchema | Properties of a PATCH request body must not have default value, property:authenticationMode.Location: Microsoft.Batch/stable/2025-06-01/BatchManagement.json#L146 ❌ PatchBodyParametersSchema | Properties of a PATCH request body must not have default value, property:publicNetworkAccess.Location: Microsoft.Batch/stable/2025-06-01/BatchManagement.json#L146 ❌ PatchBodyParametersSchema | Properties of a PATCH request body must not be required, property:defaultAction.Location: Microsoft.Batch/stable/2025-06-01/BatchManagement.json#L146 |
ARM (Control Plane) API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting help
section 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 ApiView
comment 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 PR
andDue diligence checklist
.write access
per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Merge
comment. 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.
queued
state, please add a comment with contents/azp run
.This should result in a new comment denoting a
PR validation pipeline
has started and the checks should be updated after few minutes.