-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add CheckNameAvailability operation to 2017-05-01 Swagger (management plane) for Azure Batch #1432
Conversation
@kiwidev, |
This looks good to me @kiwidev |
Thanks @kiwidev for the PR. I'll be reviewing the PR by tomorrow. Feel free to ping me anytime if you need something meanwhile :) |
#ARMSignedOff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiwidev Left some questions & comments. Let me know what you think. Thanks!
BatchManagement.json
Outdated
"tags": [ | ||
"Application" | ||
], | ||
"operationId": "Application_List", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we name it as Application_ListByBatchAccount
being applications
as tracked resource?
BatchManagement.json
Outdated
"in": "body", | ||
"required": true, | ||
"schema": { | ||
"$ref": "#/definitions/BatchAccountCreateParameters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
BatchManagement.json
Outdated
} | ||
} | ||
}, | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Batch/batchAccounts/{accountName}/applications/{applicationId}/versions/{version}": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
BatchManagement.json
Outdated
"tags": [ | ||
"BatchAccount" | ||
], | ||
"operationId": "BatchAccount_Create", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
BatchManagement.json
Outdated
"tags": [ | ||
"ApplicationPackage" | ||
], | ||
"operationId": "ApplicationPackage_Activate", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
BatchManagement.json
Outdated
}, | ||
"description": "Parameters for adding an Application." | ||
}, | ||
"Application": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
BatchManagement.json
Outdated
}, | ||
"description": "Contains information about an application in a Batch account." | ||
}, | ||
"ApplicationPackage": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type doesn't match the ARM proxy specification and will be reworked in a future API version.
BatchManagement.json
Outdated
}, | ||
"description": "Parameters for adding an Application." | ||
}, | ||
"Application": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type doesn't match the ARM proxy specification and will be reworked in a future API version.
BatchManagement.json
Outdated
}, | ||
"ApplicationPackage": { | ||
"properties": { | ||
"id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a resource then are we missing name
& type
as readOnly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type doesn't match the ARM proxy specification and will be reworked in a future API version.
BatchManagement.json
Outdated
@@ -0,0 +1,1618 @@ | |||
{ | |||
"swagger": "2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is placed at the wrong location. Please move it inside the Batch specifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@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. The intention of this PR was just to review the "checkNameAvailability" changes. |
hahaah... thanks for removing that file. Let me review the smaller chunk now :) |
Closing and re-opening this PR to have the latest CI changes to include CI bug fix. |
@kiwidev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiwidev Left couple of minor comments. Let me know what you think? Thanks!
}, | ||
"type": { | ||
"type": "string", | ||
"description": "The resource type. Must be set to Microsoft.Batch/batchAccounts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, thanks.
"CheckNameAvailabilityResult": { | ||
"properties": { | ||
"nameAvailable": { | ||
"type": "boolean", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be readOnly: true
as seems like only settable by server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
}, | ||
"description": "Gets the reason that a Batch account name could not be used. The Reason element is only returned if NameAvailable is false." | ||
}, | ||
"message": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readOnly: true
here as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Thanks @vishrutshah, changes made as noted. |
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: AutoRest Linter Guidelines | AutoRest Linter Issues Send feedback and make AutoRest Linter Azure Bot smarter day by day! Thanks for your co-operation. |
@vishrutshah is the build error on the ruby build: |
@kiwidev It's because of the wrong |
looks like model validation failing but seems like that already existed. BatchAccount_Delete: Would you mind fixing this by adding |
I'll make the change as part of this PR. |
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: AutoRest Linter Guidelines | AutoRest Linter Issues Send feedback and make AutoRest Linter Azure Bot smarter day by day! Thanks for your co-operation. |
@vishrutshah Change made to add 204 to the example, model validation now passing. |
Thanks @kiwidev for the quick turn-around! 👍 |
No modification for AutorestCI/azure-sdk-for-node |
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
api-version
in the path should match theapi-version
in the spec).Quality of Swagger
Note for reviewers