Swagger json file for Azure Automation accounts#950
Merged
fearthecowboy merged 13 commits intoAzure:masterfrom Mar 13, 2017
Merged
Swagger json file for Azure Automation accounts#950fearthecowboy merged 13 commits intoAzure:masterfrom
fearthecowboy merged 13 commits intoAzure:masterfrom
Conversation
Starting with these 4 resources. json files are split by each resource. rest of resources to come subsequently.
|
Hi @vivlingaiah, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
viveklingaiah
changed the title
Contributor
|
Hey @vivlingaiah Can I get you to install node.js run the following commands: # Install the new autorest command line
npm install -g autorest
# run autorest on the swagger
autorest -input arm-automation/compositeAutomation.json -CodeGenerator Azure.CSharp -Modeler CompositeSwaggerWhen I do that, I'm finding a lot of issues: |
Contributor
|
(posted from @vivlingaiah 's email ) I am getting the below error messages when running AutoRest using 4 separate json files in PR. Note that the PR does not have json files for all resources. The plan is to create json files in two batches.
I believe this message is saying that the operation group should be called "Streams" not "JobStreams" -- still checking on this--it's a new rule. C:\Users\vivekl\Documents\GitHub\azure-rest-api-specs\arm-automation>autorest -input compositeAutomation.json -CodeGenerator Azure.CSharp -Modeler CompositeSwagger > Log.txt
ERROR: Operation JobStreams_List must be one of: streams_listbysubscriptionid, streams_list, streams_listbyresourcegroup, streams_listbyjobs
Path: compositeAutomation.json#/paths
ERROR: Operation TypeFields_List must be one of: fields_listbysubscriptionid, fields_list, fields_listbyresourcegroup, fields_listbytypes
Path: compositeAutomation.json#/paths
ERROR: Operation TestJobStreams_List must be one of: streams_listbysubscriptionid, streams_list, streams_listbyresourcegroup, streams_listbytestjob
Path: compositeAutomation.json#/pathsThis one is telling you that you have to have an "Operations API" that returns the methods for your whole API. It's telling you that the name of the operation must be called In the object at: the property It's telling you that you need to also supply a response that is "200 or 201 for Put/Patch. 200, 201 or 204 for Post. 200 or 204 or both for Delete." the This is telling you that you must use the word |
viveklingaiah
changed the title
fearthecowboy
suggested changes
Mar 3, 2017
| "tags": [ | ||
| "AutomationAccounts" | ||
| ], | ||
| "operationId": "AutomationAccounts_Patch", |
Contributor
There was a problem hiding this comment.
can not have two operationIds in the same operation.
This isn't even valid JSON, and when I run it in AutoRest, it throws an error. (which isn't helpful yet, but is still a red flag!)
You can use tools like http://jsonlint.com/ to validate your JSON
Please correct it and update.
Contributor
Author
|
committed the right account.json. I had the duplicate OperationId removed in my local file. I had not pushed it. |
Contributor
|
There are still a few things left that should be fixed -- descriptions for certain. > autorest -input https://github.com/AzureAutomationTeam/azure-rest-api-specs/blob/ac72df3b75765089756673c3bff43a2e5cafc044/arm-automation/compositeAutomation.json -CodeGenerator none -Modeler CompositeSwagger
Ignore these two For certain, fix the descriptions: These are false-positives. Don't worry about these. |
Contributor
Author
|
Garrett, qq. what version of Autorest are you using ? You seem to get more warnings and errors. I'm using AutoRest 1.0.1.0 . Installed from https://github.com/Azure/autorest/#installing-autorest |
| { | ||
| "swagger": "2.0", | ||
| "info": { | ||
| "title": "AutomationManagementClient", |
Contributor
There was a problem hiding this comment.
We don't put the word Client in the title anymore, since this describes the service not the client
| ], | ||
| "consumes": [ | ||
| "application/json", | ||
| "text/json" |
Contributor
There was a problem hiding this comment.
in 'consumes' it's not really valuable to add text/json -- application/json is sufficient.
| "application/json", | ||
| "text/json" | ||
| ], | ||
| "produces": [ |
Contributor
There was a problem hiding this comment.
in produces it's not really valuable to add text/json -- application/json is sufficient.
Plus, do you actually ever produce text/json ? This is an advertisement of what your service will output.
There was a problem hiding this comment.
Can this be at resource level? We have only one resource that will output test/json
| "operationId": "AutomationAccounts_Update", | ||
| "description": "Patch an automation account.", | ||
| "externalDocs": { | ||
| "url": "http://aka.ms/azureautomationsdk/automationaccountoperations" |
Contributor
There was a problem hiding this comment.
The url http://aka.ms/azureautomationsdk/automationaccountoperations isn't currently valid -- are you expecting this to be created later?
There was a problem hiding this comment.
yes, our doc team will point to the destination documentation home page
| "x-ms-client-flatten": true, | ||
| "description": "Gets or sets account update properties." | ||
| }, | ||
| "name": { |
Contributor
There was a problem hiding this comment.
Shouldn't name and/or location be read-only?
Contributor
Author
There was a problem hiding this comment.
they are not read-only. They are used to delete accounts
Contributor
Author
There was a problem hiding this comment.
they are not read-only. They are used to update accounts
fearthecowboy
suggested changes
Mar 7, 2017
Contributor
fearthecowboy
left a comment
fearthecowboy
left a comment
There was a problem hiding this comment.
I've left a few comments, but they apply all over the doc.
things like gets or sets.. should not be there -- instead document what the property does or how it affects things.
You should have some validations (size, range, regexes) on properties and parameters when they are coming from the user.
You should mark things as read only or required as necessary.
| "$ref": "#/parameters/ApiVersionParameter" | ||
| } | ||
| ], | ||
| "responses": { |
Contributor
There was a problem hiding this comment.
Every operation MUST have a default status code in the responses section. The default corresponds to the error returned by the operation.
Contributor
Author
There was a problem hiding this comment.
added default response
| "$ref": "#/parameters/ApiVersionParameter" | ||
| } | ||
| ], | ||
| "responses": { |
Contributor
There was a problem hiding this comment.
Every operation MUST have a default status code in the responses section. The default corresponds to the error returned by the operation.
Contributor
Author
There was a problem hiding this comment.
added default response
| "$ref": "#/parameters/ApiVersionParameter" | ||
| } | ||
| ], | ||
| "responses": { |
Contributor
There was a problem hiding this comment.
Every operation MUST have a default status code in the responses section. The default corresponds to the error returned by the operation.
Contributor
Author
There was a problem hiding this comment.
added default response
| "$ref": "#/parameters/ApiVersionParameter" | ||
| } | ||
| ], | ||
| "responses": { |
Contributor
There was a problem hiding this comment.
Every operation MUST have a default status code in the responses section. The default corresponds to the error returned by the operation.
Contributor
There was a problem hiding this comment.
... for all operations ...
Contributor
Author
There was a problem hiding this comment.
added default response
| }, | ||
| "description": "The parameters supplied to the patch account properties." | ||
| }, | ||
| "AutomationAccountUpdateParameters": { |
Contributor
There was a problem hiding this comment.
The documentation of model properties MUST NOT start with the phrase Gets or sets .., Gets .., Sets ..
(for all properties).
There was a problem hiding this comment.
Is this change from Hyak? For Hyak it was requirement to have Gets or Sets in description so that user knows if it is only gettable and settable property. Here is the MSDN documentation for released SDK
Since this swagger is generation from Hyak we have gets or sets in description.
| }, | ||
| "description": "The parameters supplied to the create or update account properties." | ||
| }, | ||
| "AutomationAccountCreateOrUpdateParameters": { |
Contributor
There was a problem hiding this comment.
Are any of these required fields?
they should be marked with "required" : true
Contributor
Author
There was a problem hiding this comment.
automationAccountName and AutomationAccountCreateOrUpdateParameters both have required true. those are the only 2 candidates in this file
| "description": "Gets or sets the name of the resource." | ||
| }, | ||
| "location": { | ||
| "type": "string", |
Contributor
There was a problem hiding this comment.
are there any validation rules that should be applied on these parameters?
things like :
"maxLength": 256,
"minLength": 1,
"pattern": "^[a-zA-Z0-9][a-zA-Z0-9_.-]*$"
Worth reading: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameter-object-examples
There was a problem hiding this comment.
Most of our parameter validations are done in b/e web service. The reason for this is, at times we have requirements from customer to increase Max length of parameter etc., for such cases its easy to fix and deploy b/e is production. Customer needs not update the SDK for changes. For this particular case ARM validates location and return appropriate localized error if location is incorrect.
| }, | ||
| "description": "The response model for the list account operation." | ||
| }, | ||
| "Resource": { |
Contributor
There was a problem hiding this comment.
You have inconsistent spacing in this file-- please ensure that it is properly formatted at 2 space indents.
Contributor
Author
There was a problem hiding this comment.
is there a way to autocorrect the indentation ?
fearthecowboy
suggested changes
Mar 9, 2017
Contributor
fearthecowboy
left a comment
fearthecowboy
left a comment
There was a problem hiding this comment.
Mostly just some pretty weak descriptions, and please run the code thru a formatter to get 2-space indentation on your JSON.
| "$ref": "#/definitions/AutomationAccount" | ||
| } | ||
| }, | ||
| "default": { |
Contributor
There was a problem hiding this comment.
You've still got indentation problems. (hint: paste it into http://jsonprettyprint.com/ and copy the output )
Contributor
Author
There was a problem hiding this comment.
jsonprettyprint adds escape backslashes. I do not think that's the right thing to do. could you confirm ?
Contributor
Author
There was a problem hiding this comment.
jsonprettyprint adds escape backslashes. I do not think that's the right thing to do. could you confirm ?
| "minLength": 6, | ||
| "maxLength": 50, | ||
| "pattern": "^[A-Za-z][-A-Za-z0-9]{4,48}[A-Za-z0-9]$", | ||
| "description": "Parameters supplied to the create or update automation account." |
Contributor
There was a problem hiding this comment.
"description": "Parameters supplied to the create or update automation account." -- this should be something about the automationAccountName
| }, | ||
| "family": { | ||
| "type": "string", | ||
| "description": "sku family." |
| "capacity": { | ||
| "type": "integer", | ||
| "format": "int32", | ||
| "description": "sku capcity." |
| "properties": { | ||
| "sku": { | ||
| "$ref": "#/definitions/Sku", | ||
| "description": "account sku." |
Contributor
There was a problem hiding this comment.
Remember, these documentation strings are used to generated the online API documentation and the inline code documentation that end users will consume.
You might want to make the content at least a readable sentence (ie, The account SKU )
| "description": "account sku." | ||
| } | ||
| }, | ||
| "description": "The parameters supplied to the update account properties." |
Contributor
There was a problem hiding this comment.
remove the word properties -- ie, The parameters supplied to the update account.
| "properties": { | ||
| "$ref": "#/definitions/AutomationAccountUpdateProperties", | ||
| "x-ms-client-flatten": true, | ||
| "description": "account update properties." |
Contributor
There was a problem hiding this comment.
Capitalization of sentence.
| }, | ||
| "description": { | ||
| "type": "string", | ||
| "description": "description." |
Contributor
There was a problem hiding this comment.
description is not a sufficient description of the description field. I suggest a description that better articulates what the description field is describing.
| "type": "string" | ||
| } | ||
| }, | ||
| "description": "display properties" |
Contributor
There was a problem hiding this comment.
description is a little brief.
| "type": "string", | ||
| "minLength": 6, | ||
| "maxLength": 50, | ||
| "pattern": "^[A-Za-z][-A-Za-z0-9]{4,48}[A-Za-z0-9]$", |
There was a problem hiding this comment.
Most of our parameter validations are done in b/e web service. The reason for this is, at times we have requirements from customer to increase Max length of parameter etc., for such cases its easy to fix and deploy b/e is production. Customer needs not update the SDK for changes. For this particular case ARM validates location and return appropriate localized error if location is incorrect.
| "type": "string", | ||
| "minLength": 6, | ||
| "maxLength": 50, | ||
| "pattern": "^[A-Za-z][-A-Za-z0-9]{4,48}[A-Za-z0-9]$", |
fearthecowboy
approved these changes
Mar 13, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Starting with these 4 resources. json files are split by each resource.
rest of resources to come subsequently.
-Linter is not accessible at the below link by the way (I already have access from aka.ms/azuregithub)
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-versionin the path should match theapi-versionin the spec).Quality of Swagger