Fixed incorrect type formats in SQL database by jaredmoo · Pull Request #1107 · Azure/azure-rest-api-specs

jaredmoo · 2017-04-10T20:23:36Z

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.

sameralameer · 2017-04-11T19:41:01Z

Apologies for the Delay @jaredmoo, we lately had a surge of PRs, someone will get on this shortly

jaredmoo · 2017-04-11T19:54:18Z

Thanks Samer :) there might be some validation warnings I need to fix, will look into those soon

salameer · 2017-04-11T22:33:24Z

Gotcha

balajikris · 2017-04-11T23:44:51Z

I ran the linter and semantic check tools and this change appears to be clean. I found no errors from the tools. Will sign off, after I do a manual review.

Tasks:

CI passes
Autorest linter passes (filed work items)
open-api-validation tool run passes
manual review

balajikris

LGTM.

balajikris · 2017-04-12T19:06:17Z

okay my bad here. I forgot to pass in the composite swagger entry point to the tool and the tool happily returned no errors. If I do however pass in the compositeSwagger entry point, it throws a bunch of errors.

I'll go through the list again. 👍

I submitted incorrectly

balajikris · 2017-04-13T19:24:51Z

Status as of 4/13: emailed @jaredmoo about the list of linter errors and examples errors that we should consider addressing.

balajikris

Emailed a set of linter errors and example errors that we need to address.

balajikris · 2017-04-13T19:26:55Z

arm-sql/2014-04-01/swagger/capabilities.json

                    "readOnly": true,
                    "type": "string",
-                    "description": "The unique ID of the service objective."
+                    "format": "uuid",


autorest linter tags this as

WARNING: Guid used at the #/Definitions/ServiceObjectiveCapability/.../id. Usage of Guid is not recommanded. If GUIDs are absolutely required in your service, please get sign off from the Azure API review board.

@ravbhatnagar : what's the recommended change here? I do not have enough context on why guids are bad and what's the right way to fix this. I'll appreciate it if you can fill me and Jared in here.

Unfortunately no change is possible. This is what the service is returning, and changing it at this point would be a REST API breaking change. We will have to consider removing this guid from the next api version.

Hmm. Thought as much. Thanks for confirming

balajikris · 2017-04-17T18:30:35Z

Summary of errors we will track by bugs:

ERROR: Please provide x-ms-examples describing minimum/maximum property set for response/request payloads for operations.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/paths
[Jared Moore] Let’s file a separate issue for this
ERROR: The tracked resource, 'Database', must have a list by resource group operation.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/Database
ERROR: The tracked resource, 'ElasticPool', must have a list by resource group operation.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/ElasticPool
ERROR: The tracked resource, 'Database', must have a list by subscriptions operation.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/Database
[Jared Moore] Let’s file a separate issue for this. It’s not implemented in the service right now.
ERROR: The tracked resource, 'ElasticPool', must have a list by subscriptions operation.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/ElasticPool
ERROR: Tracked resource 'Database' must have patch operation that at least supports the update of tags.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/Database
ERROR: Tracked resource 'Server' must have patch operation that at least supports the update of tags.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/Server
ERROR: Tracked resource 'ElasticPool' must have patch operation that at least supports the update of tags.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/ElasticPool
[Jared Moore] Let’s file a separate issue for this. It’s not implemented in the service right now.
ERROR: Model definition 'FirewallRule' must have the properties 'name', 'id' and 'type' in its hierarchy and these properties must be marked as readonly.
Path: file:///C:/workspace/azure-rest-api-specs/arm-sql/compositeSql.json#/definitions/FirewallRule
[Jared Moore] The service really doesn’t send ‘type’ for FirewallRule. This is a bug but not one that I can easily fix. Will file issue for this.

jaredmoo · 2017-04-17T22:59:20Z

Thanks, let me just double check there are no more changes needed

…ageMetric.

balajikris · 2017-04-19T20:30:39Z

Reviewed again after the last commit. things are still good 👍

AutorestCI · 2017-04-19T20:35:04Z

No modification for NodeJS

AutorestCI · 2017-04-19T20:35:10Z

Azure/azure-sdk-for-ruby#759

AutorestCI · 2017-04-19T20:35:27Z

Azure/azure-sdk-for-python#1094

…er-2020-01-01-preview Dev vnfmanager microsoft.vnf manager 2020 01 01 preview

jaredmoo added 2 commits April 10, 2017 13:16

Fixed databaseId format

ff1fe16

Fixed recoveryServicesRecoveryPointResourceId format

4a9a9f8

msftclas added the cla-not-required label Apr 10, 2017

azuresdkci assigned alvadb Apr 10, 2017

jaredmoo added 6 commits April 10, 2017 14:08

FIxed nullability of service objective properties

54fb52c

Fixed types of capabilities properties

1792796

Fixed format of ElasticPoolActivityProperties operationId

30b5386

Merge remote-tracking branch 'azure/master' into typefixes

6edc59c

Make activity operation id non-nullable

bcb426e

Use false instead of false for x-nullable

8307231

salameer assigned balajikris and unassigned alvadb Apr 11, 2017

balajikris previously approved these changes Apr 12, 2017

View reviewed changes

balajikris suggested changes Apr 13, 2017

View reviewed changes

jaredmoo mentioned this pull request Apr 14, 2017

Separate servers, operations, and backups from sql.core. #1122

Merged

10 tasks

Merge remote-tracking branch 'azure/master' into typefixes

04506ec

balajikris mentioned this pull request Apr 17, 2017

Linter errors in arm-sql spec that needs to be fixed #1128

Closed

balajikris approved these changes Apr 17, 2017

View reviewed changes

Fixed readonly/nullability for ServiceTierAdvisorProperties and SloUs…

af09c73

…ageMetric.

balajikris merged commit 4b0aa25 into Azure:master Apr 19, 2017

AutorestCI mentioned this pull request Apr 19, 2017

Automatic PR from RestAPI-PR1107 Azure/azure-sdk-for-ruby#759

Merged

AutorestCI mentioned this pull request Apr 19, 2017

Automatic PR from RestAPI-PR1107 Azure/azure-sdk-for-python#1094

Merged

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

Merge pull request #1107 from Azure/dev-vnfmanager-Microsoft.VnfManag…

3111cae

…er-2020-01-01-preview Dev vnfmanager microsoft.vnf manager 2020 01 01 preview

Conversation

jaredmoo commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

sameralameer commented Apr 11, 2017

Uh oh!

jaredmoo commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salameer commented Apr 11, 2017

Uh oh!

balajikris commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balajikris left a comment

Choose a reason for hiding this comment

Uh oh!

balajikris commented Apr 12, 2017

Uh oh!

balajikris commented Apr 13, 2017

Uh oh!

balajikris left a comment

Choose a reason for hiding this comment

Uh oh!

balajikris Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

jaredmoo Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

balajikris Apr 13, 2017

Choose a reason for hiding this comment

Uh oh!

balajikris commented Apr 17, 2017

Uh oh!

jaredmoo commented Apr 17, 2017

Uh oh!

balajikris commented Apr 19, 2017

Uh oh!

AutorestCI commented Apr 19, 2017

Uh oh!

AutorestCI commented Apr 19, 2017

Uh oh!

AutorestCI commented Apr 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jaredmoo commented Apr 10, 2017 •

edited

Loading

jaredmoo commented Apr 11, 2017 •

edited

Loading

balajikris commented Apr 11, 2017 •

edited

Loading