Adding new API version 2018-01-10 fixing swagger violations. by avneeshrai · Pull Request #2419 · Azure/azure-rest-api-specs

avneeshrai · 2018-02-05T14:10:29Z

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.

AutorestCI · 2018-02-05T14:11:28Z

This commit was treated and no generation was made for Azure/azure-sdk-for-python

AutorestCI · 2018-02-05T14:12:18Z

This commit was treated and no generation was made for Azure/azure-sdk-for-go

avneeshrai · 2018-02-05T15:18:13Z

This PR introduces new API version with following changes as compared to last version:

Fixes for swagger violations: a) Camel casing. b) Unwanted top level properties in Job model.
Introducing few new APIs like (TargetComputeSizes_ListByReplicationProtectedItem)
Having operations and models in sorted order in the spec.

avneeshrai · 2018-02-05T15:21:22Z

Note:

Testing for the changes is in progress, PRs would be updated with the fixes if required.
We will soon update the swagger PR with right API examples (instances which are currently being flagged by model validator).

jianghaolu

Please add the new api version in your readme.

avneeshrai · 2018-02-06T13:09:19Z

Updated the readme

AutorestCI · 2018-02-06T13:09:46Z

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

AutorestCI · 2018-02-06T13:13:52Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

jianghaolu · 2018-02-06T19:28:57Z

@avneeshrai Can you please add the new api version to the README? Without that our validation tool will not be able to run correctly. After that I'll be able to review based on the validation results. Thanks!

avneeshrai · 2018-02-07T05:42:45Z

@jianghaolu I have already updated the readme in commit 2cad803 . Did i miss something there?

AutorestCI · 2018-02-07T11:45:48Z

Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

Command: profileBuilder -s preview -name preview
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

avneeshrai · 2018-02-07T16:22:40Z

And please help with the error: Swagger to SDK encountered a Subprocess error: (Azure/azure-sdk-for-go)

/bin/sh: 1: profileBuilder: not found

what needs to be done here?

fearthecowboy · 2018-02-07T18:21:19Z

@mozehgir -- Why was this reassigned?

mozehgir · 2018-02-07T18:24:44Z

Hey Garrett(@fearthecowboy ), I believe Jianghao is OOF for a week and if this PR can be reviewed before the next fundamentals meeting (Feb 13th) - it would be great.
Ideally if we could have it merged this week, we can represent the team accordingly in the meeting.

fearthecowboy · 2018-02-08T19:26:15Z

Hey @avneeshrai , @mozehgir

The new service.json file for the new API version has everything in a completely different order. This significantly increases the effort to review, since the diffs are useless to correlate against.

Is it possible that you can leave the file in the same order the original file was in?

…ier review.

avneeshrai · 2018-02-09T08:35:36Z

@fearthecowboy I have reverted alphabetical sorting of path and definitions for most of the cases so that review efforts are saved.
We will update the PR again just by sorting definitions and paths
Let me know if there are any other action item on us w.r.t PR.

AutorestCI · 2018-02-09T08:36:15Z

Automation for azure-sdk-for-go

Encountered a Subprocess error: (azure-sdk-for-go)

Command: dep ensure
Finished with return code 127
and output:

/bin/sh: 1: dep: not found

azuresdkciprbot · 2018-02-09T08:36:45Z

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/recoveryservicessiterecovery/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 23
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

fearthecowboy · 2018-02-09T14:46:54Z

+    "application/json"
+  ],
+  "paths": {
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{resourceName}/replicationFabrics/{fabricName}/replicationProtectionContainers/{protectionContainerName}/replicationProtectedItems/{replicatedProtectedItemName}/targetComputeSizes": {


FYI - new API

fearthecowboy · 2018-02-09T14:48:24Z

+        }
+      }
+    },
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{resourceName}/replicationVaultHealth/default/refresh": {


FYI - New API

fearthecowboy · 2018-02-09T14:50:07Z

+            "in": "path",
+            "description": "Unique fabric name.",
+            "required": true,
+            "


New member -- can we provide an enum of the possible values for this?

fearthecowboy · 2018-02-09T14:54:50Z

+            "in": "path",
+            "description": "Unique fabric name.",
+            "required": true,
+            "


instanceType and multiVmGroupCreateOption seem like they could have enum to indicate the possible values.

If you use x-ms-enum and modelAsString:true you can add more values later without a breaking change.

fearthecowboy · 2018-02-09T15:00:06Z

+            "in": "path",
+            "description": "Unique fabric name.",
+            "required": true,
+            "


You're adding enum values (which is a breaking change, but ok in a new API version). You might think about making this modelAsString:true so that it can be expanded without making a breaking change later.

fearthecowboy · 2018-02-09T15:02:07Z

+            "in": "path",
+            "description": "Unique fabric name.",
+            "required": true,
+            "


I'd recommend that enums are generally modelAsString true to avoid breaking changes in the future.

fearthecowboy

Much Better! at least I was able to go thru it and see whats new much easier.

You have a few enums, which you might want to consider using x-ms-enum/modelAsString:true so that values can be changed in the future.

You also have a few things that end in 'type' or 'option' which makes me wonder if there are enums that describe valid values for those.

fearthecowboy · 2018-02-09T15:07:11Z

Tagging @ravbhatnagar for ARM review/feedback.

avneeshrai · 2018-02-12T15:38:36Z

@fearthecowboy are we talking about just one instance for multiVmGroupCreateOption to be represented as enum here? or you are referring to some other fields as well? Sorry couldn't get it from the comments.

fearthecowboy · 2018-02-12T17:59:24Z

Hey @avneeshrai

I was looking at multiVmGroupCreateOption when I then noticed the same thing throughout the file. Any time you have a string field that ends in ..option or ..type it piques my curiosity to see if there is an enum that contains the acceptable values.

If you don't have an enum and there are specific values that mean something, then we're really doing consumers a disservice by not listing them.

using x-ms-enum and specifying modelAsString: true will make it so you can add more values later without making it a breaking change (since, we store the value even if it's not known, and older clients won't die when they see an unknown value.)

Given that you have a new API version here, I strongly suggest you take the opportunity to fill in details on types like multiVmGroupCreateOption to make sure we've got the best possible documentation for your service, and enable us to generate the best possible SDK we can. 😄

… NetworkMappingFabricSpecificSettings

azuresdkciprbot · 2018-02-13T14:32:50Z

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/recoveryservicessiterecovery/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 23
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

avneeshrai · 2018-02-13T14:43:52Z

@fearthecowboy we have made multiVmGroupCreateOption as enum as suggested however kept modelAsString: false, to keep it consistent with other enum fields in our APIs.
Just a question here: multiVmGroupCreateOption is also used for odata query filtering in one of the case, hope that works?

avneeshrai · 2018-02-13T14:45:36Z

@fearthecowboy and yeah let us know if there is any other action item on us :)

fearthecowboy · 2018-02-13T17:21:42Z

I guess I'm ok with this; we're just waiting for ARM signoff from @ravbhatnagar

ravbhatnagar · 2018-02-15T22:13:54Z

Signing off! most of these, if not all, are existing APis which have already been reviewed. It looks like they are getting added to swagger now. @avneeshrai - please correct if I am wrong here.

avneeshrai · 2018-02-17T14:43:52Z

Right @ravbhatnagar, as per my understanding approval have been taken by ASR team for most of these APIs.

@fearthecowboy I will now update the PR after sorting API paths and definitions as discussed in the thread. Please merge (if you dont see any issue there).

avneeshrai · 2018-02-19T21:30:53Z

@fearthecowboy could you please review and merge this :)

not needed

Adding new API version 2018-01-10 fixing swagger voilations

7c01529

azuresdkci assigned jianghaolu Feb 5, 2018

jianghaolu previously requested changes Feb 5, 2018

View reviewed changes

jianghaolu added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Feb 5, 2018

Updating readme file for new API:2018-01-10

2cad803

Updating api examples

8091f0a

mozehgir unassigned jianghaolu Feb 7, 2018

mozehgir added the Reassign label Feb 7, 2018

azuresdkci removed the Reassign label Feb 7, 2018

azuresdkci assigned fearthecowboy Feb 7, 2018

Reverting alphabetical ordering of operations and definitions for eas…

345e327

…ier review.

fearthecowboy reviewed Feb 9, 2018

View reviewed changes

fearthecowboy suggested changes Feb 9, 2018

View reviewed changes

fearthecowboy assigned ravbhatnagar Feb 9, 2018

Taking review comments and introducing missing polymorphic fields for…

27c648b

… NetworkMappingFabricSpecificSettings

fearthecowboy approved these changes Feb 13, 2018

View reviewed changes

avneeshrai mentioned this pull request Feb 14, 2018

Changes corresponding to new swagger PR for API:2018-01-10 Azure/azure-sdk-for-net#4075

Merged

9 tasks

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Feb 15, 2018

Alphabetically sorting definitions and API paths.

bbebc30

fearthecowboy merged commit a98fa16 into Azure:master Feb 20, 2018

Conversation

avneeshrai commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Feb 5, 2018

Uh oh!

AutorestCI commented Feb 5, 2018

Uh oh!

avneeshrai commented Feb 5, 2018

Uh oh!

avneeshrai commented Feb 5, 2018

Uh oh!

jianghaolu left a comment

Choose a reason for hiding this comment

Uh oh!

avneeshrai commented Feb 6, 2018

Uh oh!

AutorestCI commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Feb 6, 2018

Uh oh!

jianghaolu commented Feb 6, 2018

Uh oh!

avneeshrai commented Feb 7, 2018

Uh oh!

AutorestCI commented Feb 7, 2018

Uh oh!

avneeshrai commented Feb 7, 2018

Uh oh!

fearthecowboy commented Feb 7, 2018

Uh oh!

mozehgir commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fearthecowboy commented Feb 8, 2018

Uh oh!

avneeshrai commented Feb 9, 2018

Uh oh!

AutorestCI commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

azuresdkciprbot commented Feb 9, 2018

Uh oh!

fearthecowboy Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Feb 9, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy left a comment

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Feb 9, 2018

Uh oh!

avneeshrai commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fearthecowboy commented Feb 12, 2018

Uh oh!

azuresdkciprbot commented Feb 13, 2018

Uh oh!

avneeshrai commented Feb 13, 2018

avneeshrai commented Feb 5, 2018 •

edited

Loading

AutorestCI commented Feb 6, 2018 •

edited

Loading

mozehgir commented Feb 7, 2018 •

edited

Loading

AutorestCI commented Feb 9, 2018 •

edited

Loading

fearthecowboy Feb 9, 2018 •

edited

Loading

avneeshrai commented Feb 12, 2018 •

edited

Loading

avneeshrai commented Feb 17, 2018 •

edited

Loading