Rechen batch microsoft.batch 2024 07 01

[cRui861]

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!

• New resource provider.
• New API version for an existing resource provider. (If API spec is not defined in TypeSpec, the PR should have been created in adherence to OpenAPI specs PR creation guidance).
• Update existing version for a new feature. (This is applicable only when you are revising a private preview API version.)
• Update existing version to fix OpenAPI spec quality issues in S360.
• Convert existing OpenAPI spec to TypeSpec spec (do not combine this with implementing changes for a new API version).
• Other, please clarify:
  • edit this with your clarification

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:

• I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data plane related specifications.
• I have reviewed following Resource Provider guidelines, including
  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

• First, please carefully read through this PR description, from top to bottom. Please fill out the Purpose of this PR and Due diligence checklist.
• If you don't have permissions to remove or add labels to the PR, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories
• To understand what you must do next to merge this PR, see the Next 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.
• For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure
  and https://aka.ms/ci-fix.
• For help with ARM review (PR workflow diagram Step 2), see https://aka.ms/azsdk/pr-arm-review.
• If the PR CI checks appear to be stuck in 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.
• If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

[openapi-pipeline-app]

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

[openapi-pipeline-app]

Generated ApiView

Language	Package Name	ApiView Link
Go	sdk/resourcemanager/batch/armbatch	https://apiview.dev/Assemblies/Review/5d7090769fc1444da42a31413d67c9ac?revisionId=2afd37e40a1045a6998b6c7c093cf389
Java	azure-resourcemanager-batch	https://apiview.dev/Assemblies/Review/899c727c2d1a4256b847a475f8259c6d?revisionId=ea776f36453e4774912bde9c4552a46d
JavaScript	@azure/arm-batch	https://apiview.dev/Assemblies/Review/9751edb49ede484ab29d90906184f1ca?revisionId=2e33a5c6fba74bde9ac987ee0ae42eaa
Swagger	Microsoft.Batch	https://apiview.dev/Assemblies/Review/85a2658fa59441d2bfe325d29d1d9b90?revisionId=3dcfd49fda7e4888b255dd39176b98bc

[mentat9]

    "isReadOnly": {

ARM recommends enums over booleans for future proof APIs. Standard guidance is: replace boolean/switch properties with a more meaningful enum whenever possible.
A boolean will forever have two valid values (true or false). A string enum type is always preferred. Also, properties should always provide better values just than True and False. For example two switches "isTypeA" and "isTypeB" should be replaced with one enum "type": [A, B, DefaultType]. Enums are always a more flexible and future proof option because they allow additional values to be added in the future in a non-breaking way, e.g. [Enabled, Disabled, Suspended, Deallocated].
Note: do NOT define a 'boolean enum' with two values 'True and False'. This might be easier to 'extend' in terms of types, but semantically its confusing, and no better than a boolean.
Refers to: specification/batch/resource-manager/Microsoft.Batch/stable/2024-07-01/BatchManagement.json:5469 in a1d0d19. [](commit_id = a1d0d19, deletion_comment = False)

Spoke with the engineer who worked on this for our service. isReadOnly, in this context, can only be isReadOnly = True or False (no more choices). So it seems unnecessary to change it to an enum. Same goes for the other boolean properties like EnableXXX etc.

It's not a question of "necessary" or not. It's a question of quality of the APIs. It's common for cloud APIs to get hacked up with booleans over time and become complicated for clients to use. It can become difficult to select which vector of bits they need to set in order to make the API do what they want. Often there are combinations that are meaningless (true, true, false, true, is OK, but true, false, false, true doesn't even give a result). It's generally cleaner to more carefully design the options in the API to select amongst a set of only valid combinations that each have meaningful names.

For example, "isReadOnly" is named that because it's boolean, not because that's the clearest way to specify it. We would recommend something like access: { ReadOnly, ReadWrite }. It's just better. You could even add a value for linux containers indicating that access is modified by the VM path access, e.g. access: { ReadOnly, ReadWrite, WriteControlledByVM } (I'm not saying your APIs should support that, it's just an example).

And once your APIs are on the path of adding booleans for everything, it's self-fulfilling, nudging future API designs toward continuing the practice going forward: EnableXXX, EnableXXY, EnableXXZ, etc.

These are some additional reasons we give this feedback. Please consider in future API designs that 2 values != boolean.

[mentat9]

@cRui861 - I'm not blocking signoff, but please consider our feedback for future APIs. We want to raise the quality and consistency of Azure across the whole API surface area. This is where the feedback comes from: it's not just about a single property in one API version of a single API.

[msyyc]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Alancere]

Hi @raych1, please approve BreakingChange-Go-Sdk-Suppression, the Go SDK BreakingChange is caused by the current PR.

[msyyc]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[cRui861]

@cRui861 - I'm not blocking signoff, but please consider our feedback for future APIs. We want to raise the quality and consistency of Azure across the whole API surface area. This is where the feedback comes from: it's not just about a single property in one API version of a single API.

Thank you! We definitely understand the concern here and will keep this feedback in mind for future APIs.