Skip to content

Conversation

@nathannfan
Copy link
Contributor

@nathannfan nathannfan commented Jun 6, 2017

Add database and elastic pool metric swagger coverage and examples. Move usages to usages.json.

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

@olydis
Copy link
Contributor

olydis commented Jun 6, 2017

@nathannfan Please add security definitions as described here (see https://github.com/Azure/azure-rest-api-specs/search?q=securityDefinitions for typical structure within Swagger) to all of your Swaggers 🙂

@olydis
Copy link
Contributor

olydis commented Jun 6, 2017

not sure about your SDK/CLI release state, but be aware of the breaking changes (at least due to renaming of types) introduced here

@nathannfan
Copy link
Contributor Author

@olydis Jared added them to all of them already, so I added them to my new file.

@azuresdkci
Copy link
Contributor

Hi There,

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

File: arm-sql/2014-04-01/swagger/metrics.json
Number of linter errors/warnings before the PR: 0
Number of linter errors/warnings after the PR: 1

File: arm-sql/2014-04-01/swagger/sql.core.json
Number of linter errors/warnings before the PR: 24
Number of linter errors/warnings after the PR: 24

File: arm-sql/2014-04-01/swagger/usages.json
Number of linter errors/warnings before the PR: 1
Number of linter errors/warnings after the PR: 1

Thanks for your co-operation.

@nathannfan
Copy link
Contributor Author

@olydis not sure why I'm failing validation for ruby. Any ideas?

@jaredmoo
Copy link
Contributor

jaredmoo commented Jun 6, 2017

I think the ruby build is broken

@olydis
Copy link
Contributor

olydis commented Jun 6, 2017

@jaredmoo seems so :( will check with the Ruby people, but we can go on here independently for the time being

"in": "query",
"required": true,
"type": "string",
"description": "An OData filter expression that describes a subset of metrics to return. Required for getting metrics."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General guideline: don't repeat things like requiredness in the description. For generated SDKs, imagine a method ListMetrics with a regular (non-optional) parameter filter with a description Required for getting metrics.. You would probably think "Yeah, obviously..."

"in": "query",
"required": true,
"type": "string",
"description": "An OData filter expression that describes a subset of metrics to return. Required for getting metrics."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above, remove Required for getting metrics.

}
},
"name": {
"$ref": "#/definitions/MetricName",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think you wanna tag those $refed properties as readOnly too :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathannfan did you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I thought you meant tagging the properties in the $ref'd definitions "required," not the $ref themselves

Copy link
Contributor

@olydis olydis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments. as always, very high quality! 👍

},
"required": ["value"],
"description": "Represents the response to a list server metrics request."
"description": "The Impact of an operation, both in absolute and relative terms."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: impact

@azuresdkci
Copy link
Contributor

Hi There,

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

File: arm-sql/2014-04-01/swagger/metrics.json
Number of linter errors/warnings before the PR: 0
Number of linter errors/warnings after the PR: 0

File: arm-sql/2014-04-01/swagger/sql.core.json
Number of linter errors/warnings before the PR: 0
Number of linter errors/warnings after the PR: 0

File: arm-sql/2014-04-01/swagger/usages.json
Number of linter errors/warnings before the PR: 0
Number of linter errors/warnings after the PR: 0

Thanks for your co-operation.

@olydis olydis merged commit b80af62 into Azure:master Jun 8, 2017
@nathannfan
Copy link
Contributor Author

@olydis thanks, Johannes!

@AutorestCI
Copy link

No modification for NodeJS

@AutorestCI
Copy link

@AutorestCI
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants