Add CheckNameAvailability operation to 2017-05-01 Swagger (management plane) for Azure Batch #1432

kiwidev · 2017-07-17T23:18:16Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

Note for reviewers

This method follows the RP guidelines from https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/proxy-api-reference.md#check-name-availability-requests

msftclas · 2017-07-17T23:18:20Z

@kiwidev,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

matthchr · 2017-07-18T00:09:53Z

This looks good to me @kiwidev

vishrutshah · 2017-07-18T22:58:32Z

Thanks @kiwidev for the PR. I'll be reviewing the PR by tomorrow. Feel free to ping me anytime if you need something meanwhile :)

ravbhatnagar · 2017-07-19T18:10:55Z

#ARMSignedOff

vishrutshah

@kiwidev Left some questions & comments. Let me know what you think. Thanks!

vishrutshah · 2017-07-19T20:25:33Z

BatchManagement.json

+        "tags": [
+          "Application"
+        ],
+        "operationId": "Application_List",


Should we name it as Application_ListByBatchAccount being applications as tracked resource?

vishrutshah · 2017-07-19T20:28:15Z

BatchManagement.json

+            "in": "body",
+            "required": true,
+            "schema": {
+              "$ref": "#/definitions/BatchAccountCreateParameters"


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: 'BatchAccountCreateParameters' Response Model: 'BatchAccount'

Can you please look why is this not the case?

vishrutshah · 2017-07-19T20:30:00Z

BatchManagement.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Batch/batchAccounts/{accountName}/applications/{applicationId}/versions/{version}": {


Do we not have ListByApplication operation for PATH

/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Batch/batchAccounts/{accountName}/applications/{applicationId}/versions

Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/254658613#L673

vishrutshah · 2017-07-19T20:33:00Z

BatchManagement.json

+        "tags": [
+          "BatchAccount"
+        ],
+        "operationId": "BatchAccount_Create",


As we use the NOUN part of NOUN_VERB pattern in operationId in the grouping of operation. Currently, we have a model named as NOUN that'll be auto resolved by autorest to resolve naming conflicts.

Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/254658613#L685

Please make sure that is what you expect in SDK or we need to make them differ here.

vishrutshah · 2017-07-19T20:35:35Z

BatchManagement.json

+        "tags": [
+          "ApplicationPackage"
+        ],
+        "operationId": "ApplicationPackage_Activate",


As we use the NOUN part of NOUN_VERB pattern in operationId in the grouping of operation. Currently, we have a model named as NOUN that'll be auto resolved by autorest to resolve naming conflicts.

Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/254658613#L916

Please review 18 of these [OperationIdNounConflictingModelNames] warning in the build here

Please make sure that is what you expect in SDK or we need to make them differ here.

vishrutshah · 2017-07-19T20:38:32Z

BatchManagement.json

+      },
+      "description": "Parameters for adding an Application."
+    },
+    "Application": {


line 1284: @ravbhatnagar Can you please validate that this is expected?

Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Model definition 'Application' has extra properties ['displayName,packages,allowUpdates,defaultVersion']Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Model definition 'Application' has extra properties ['displayName,packages,allowUpdates,defaultVersion']

Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/254658613#L1010

vishrutshah · 2017-07-19T20:39:51Z

BatchManagement.json

+      },
+      "description": "Contains information about an application in a Batch account."
+    },
+    "ApplicationPackage": {


line 1312: @ravbhatnagar same as above. can you please validate?

Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Model definition 'ApplicationPackage' has extra properties ['version,state,format,storageUrl,storageUrlExpiry,lastActivationTime']

Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/254658613#L1021

This type doesn't match the ARM proxy specification and will be reworked in a future API version.

vishrutshah · 2017-07-19T20:42:08Z

BatchManagement.json

+      },
+      "description": "Parameters for adding an Application."
+    },
+    "Application": {


line 1284: looks like this is an ARM resource. This should have id, name & type as the readOnly.

Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/254658613#L1045

This type doesn't match the ARM proxy specification and will be reworked in a future API version.

vishrutshah · 2017-07-19T20:43:54Z

BatchManagement.json

+    },
+    "ApplicationPackage": {
+      "properties": {
+        "id": {


If this is a resource then are we missing name & type as readOnly ?

This type doesn't match the ARM proxy specification and will be reworked in a future API version.

vishrutshah · 2017-07-19T20:48:54Z

BatchManagement.json

@@ -0,0 +1,1618 @@
+{
+  "swagger": "2.0",


This file is placed at the wrong location. Please move it inside the Batch specifications.

Oops, this file should not have been included, and has been removed. The diff was only on the BatchManagement.json file inside 2017-05-01. (Only change should be the checkNameAvailability changes).

kiwidev · 2017-07-19T21:11:35Z

@vishrutshah apologies, there was an extra file included in this commit which has now been removed. All of the issues you've pointed out are not changing as part of the PR, and they are mostly all known issues.
As part of the next API version we are planning to revisit the API surface, which will address all / most of these issues. Unfortunately, as these APIs have been in production for a while (in the case of applications since before the ARM spec was well defined for proxy resources) we're stuck with them in these API versions.

The intention of this PR was just to review the "checkNameAvailability" changes.

vishrutshah · 2017-07-19T21:33:03Z

hahaah... thanks for removing that file. Let me review the smaller chunk now :)

vishrutshah · 2017-07-19T21:34:13Z

Closing and re-opening this PR to have the latest CI changes to include CI bug fix.

msftclas · 2017-07-19T21:34:19Z

@kiwidev,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

vishrutshah

@kiwidev Left couple of minor comments. Let me know what you think? Thanks!

vishrutshah · 2017-07-19T21:37:50Z

specification/batch/resource-manager/Microsoft.Batch/2017-05-01/BatchManagement.json

+        },
+        "type": {
+          "type": "string",
+          "description": "The resource type. Must be set to Microsoft.Batch/batchAccounts"


Should this be modeled as enum with one default value? like: https://github.com/Azure/azure-rest-api-specs/blob/current/specification/storage/resource-manager/Microsoft.Storage/2017-06-01/storage.json#L689 ?

Changed, thanks.

vishrutshah · 2017-07-19T21:38:40Z

specification/batch/resource-manager/Microsoft.Batch/2017-05-01/BatchManagement.json

+    "CheckNameAvailabilityResult": {
+      "properties": {
+        "nameAvailable": {
+          "type": "boolean",


Should this be readOnly: true as seems like only settable by server

vishrutshah · 2017-07-19T21:39:03Z

specification/batch/resource-manager/Microsoft.Batch/2017-05-01/BatchManagement.json

+          },
+          "description": "Gets the reason that a Batch account name could not be used. The Reason element is only returned if NameAvailable is false."
+        },
+        "message": {


readOnly: true here as well ?

kiwidev · 2017-07-19T22:09:26Z

Thanks @vishrutshah, changes made as noted.

azuresdkciprbot · 2017-07-19T22:17:54Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/batch/resource-manager/readme.md
Before the PR: Warning(s): 26 Error(s): 14
After the PR: Warning(s): 27 Error(s): 14

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

kiwidev · 2017-07-19T22:20:36Z

@vishrutshah is the build error on the ruby build: ValueError: Autorest call ended with 0, but no files were generated actually a build error or a bug in the CI system that we can work around? Thanks.

vishrutshah · 2017-07-19T22:42:21Z

@kiwidev It's because of the wrong swagger_to_sdk config file in Ruby repo. We should be good to move forward here.

Issue Azure/azure-sdk-for-ruby#853

vishrutshah · 2017-07-19T22:51:11Z

looks like model validation failing but seems like that already existed.

BatchAccount_Delete: RESPONSE_STATUS_CODE_NOT_IN_EXAMPLE Reference: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/255449011#L662

Would you mind fixing this by adding 204 in example or we can open an issue for the service team to follow up. I am fine both the ways. Let me know. Thanks!

kiwidev · 2017-07-19T22:52:15Z

I'll make the change as part of this PR.

azuresdkciprbot · 2017-07-19T23:07:35Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/batch/resource-manager/readme.md
Before the PR: Warning(s): 26 Error(s): 14
After the PR: Warning(s): 27 Error(s): 14

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

kiwidev · 2017-07-19T23:28:01Z

@vishrutshah Change made to add 204 to the example, model validation now passing.

vishrutshah · 2017-07-20T00:18:27Z

Thanks @kiwidev for the quick turn-around! 👍

AutorestCI · 2017-07-20T00:20:52Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-07-20T00:21:22Z

Azure/azure-sdk-for-python#1311

msftclas added the cla-not-required label Jul 17, 2017

azuresdkci assigned vishrutshah Jul 17, 2017

kiwidev force-pushed the current branch from b1b3aa4 to 43f4f1a Compare July 17, 2017 23:36

vishrutshah suggested changes Jul 19, 2017

View reviewed changes

vishrutshah added ARM Batch labels Jul 19, 2017

kiwidev force-pushed the current branch from 43f4f1a to 321d156 Compare July 19, 2017 20:57

vishrutshah closed this Jul 19, 2017

vishrutshah reopened this Jul 19, 2017

msftclas added the cla-not-required label Jul 19, 2017

vishrutshah suggested changes Jul 19, 2017

View reviewed changes

Add Azure Batch Resource Manager CheckNameAvailability operation

d9d12c6

kiwidev force-pushed the current branch from 321d156 to d9d12c6 Compare July 19, 2017 22:08

Adding 204 response to BatchAccountDelete example

18f4d21

vishrutshah approved these changes Jul 20, 2017

View reviewed changes

vishrutshah merged commit 1183158 into Azure:current Jul 20, 2017

AutorestCI mentioned this pull request Jul 20, 2017

[BatchMgmt] Add CheckNameAvailability operation to 2017-05-01 Swagger Azure/azure-sdk-for-python#1311

Merged

matthchr mentioned this pull request Jul 25, 2017

Update Azure Batch to 5.1.0 Azure/azure-sdk-for-net#3513

Merged

8 tasks

Add CheckNameAvailability operation to 2017-05-01 Swagger (management plane) for Azure Batch #1432

Add CheckNameAvailability operation to 2017-05-01 Swagger (management plane) for Azure Batch #1432

Conversation

kiwidev commented Jul 17, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

Note for reviewers

msftclas commented Jul 17, 2017

matthchr commented Jul 18, 2017

vishrutshah commented Jul 18, 2017

ravbhatnagar commented Jul 19, 2017

vishrutshah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiwidev commented Jul 19, 2017

vishrutshah commented Jul 19, 2017

vishrutshah commented Jul 19, 2017

msftclas commented Jul 19, 2017

vishrutshah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiwidev commented Jul 19, 2017

azuresdkciprbot commented Jul 19, 2017

kiwidev commented Jul 19, 2017

vishrutshah commented Jul 19, 2017

vishrutshah commented Jul 19, 2017

kiwidev commented Jul 19, 2017 • edited Loading

azuresdkciprbot commented Jul 19, 2017

kiwidev commented Jul 19, 2017

vishrutshah commented Jul 20, 2017

AutorestCI commented Jul 20, 2017

AutorestCI commented Jul 20, 2017

kiwidev commented Jul 17, 2017 •

edited by azuresdkci

Loading

kiwidev commented Jul 19, 2017 •

edited

Loading