Adding new API version for Microsoft.Web RP which fixes swagger errors #2802

ruslany · 2018-04-03T18:30:37Z

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

[X ] The title of the PR is clear and informative.
[ X] 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.
[ X] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
[ X] If applicable, the PR references the bug/issue that it fixes.
[ X] Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

[ X] I have read the contribution guidelines.
[ X] My spec meets the review criteria:
- [ X] The spec conforms to the Swagger 2.0 specification.
- [ X] The spec follows the guidelines described in the Swagger checklist document.
- [ X] Validation tools were run on swagger spec(s) and have all been fixed in this PR.

…as in ARM envelope

AutorestCI · 2018-04-03T18:31:15Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2065

AutorestCI · 2018-04-03T18:36:19Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#1573

AutorestCI · 2018-04-03T18:43:04Z

Automation for azure-libraries-for-java

The initial PR has been merged into your service PR:
AutorestCI/azure-libraries-for-java#4

AutorestCI · 2018-04-03T18:56:25Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2612

lmazuel · 2018-04-03T21:24:22Z

@AutorestCI rebuild azure-sdk-for-python

olydis

@ruslany generally cool, see comments. Anything starting with [informational] is more of a statement, not a request for action on your side, but please review real quick if those changes are expected 🙂

Note that github (STILL!!!???) has this annoying bug of showing comments on any line >2048 on line 2048... so for WebApps, I also tried annotating which type/operation I meant.

olydis · 2018-04-03T21:30:16Z

specification/web/resource-manager/Microsoft.DomainRegistration/stable/2018-02-01/Domains.json

+        "summary": "Renew a domain.",
+        "description": "Renew a domain.",
+        "operationId": "Domains_Renew",
+        "parameters": [


[informational] example removed!?

olydis · 2018-04-03T21:31:11Z

specification/web/resource-manager/Microsoft.DomainRegistration/stable/2018-02-01/Domains.json

+          "204": {
+            "description": "Domain does not exist."
+          },
+          "400": {


[informational] changed from default to explicit status codes (note that this changes the SDK's behavior!)

olydis · 2018-04-03T21:32:25Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/AppServiceEnvironments.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/hostingEnvironments/{name}/changeVirtualNetwork": {


[note to self] new operation

olydis · 2018-04-03T21:33:40Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/AppServiceEnvironments.json

+        ],
+        "summary": "Resume an App Service Environment.",
+        "description": "Resume an App Service Environment.",
+        "operationId": "AppServiceEnvironments_SyncVirtualNetworkInfo",


[note to self] new operation

olydis · 2018-04-03T21:38:34Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/WebApps.json

+          "200": {
+            "description": "Succesfully deleted web app backup item.",
+            "schema": {
+              "$ref": "#/definitions/BackupItem"


[informational] added response schema (so you return the deleted item? cool!)

olydis · 2018-04-03T22:18:44Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/Certificates.json

+          "$ref": "./CommonDefinitions.json#/definitions/ProxyOnlyResource"
+        }
+      ],
+      "properties": {


[informational] geoRegion property removed

olydis · 2018-04-03T22:20:30Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/CommonDefinitions.json

+        "sourceWebAppId"
+      ],
+      "type": "object",
+      "properties": {


[informational] ignoreQuotas property removed

olydis · 2018-04-03T22:21:42Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/CommonDefinitions.json

+            "SystemAssigned",
+            "UserAssigned"
+          ],
+          "x-ms-enum": {


@ruslany type: string removed! On purpose?

@olydis ManagedServiceIdentity is now a flag enumeration. What is the correct swagger representation of such an enumeration?

@naveedaz how would that enumeration be sent over the wire? I suspect there is no good Swagger representation, but assuming that you want something like "SystemAssigned|UserAssigned" to be sent, that's still a string so it must be modeled like that 🙂

olydis · 2018-04-03T22:24:15Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/CommonDefinitions.json

+          },
+          "x-ms-client-flatten": true
+        },
+


@ruslany isLinux? 😄 I don't know the details of this, but sounds like a year from now there could be an isMacOS or similar - wouldn't it make more sense to introduce an enum property that just says platform or something? May well be that that makes no sense here, just dumping my brain 🙂

@olydis - while I agree that enum probably would be better, for now it is a bool. Should we add another platform later then we'll rework the API to be enum. As far as I know there are currently no plans for that.

sounds good to me 🙂

olydis · 2018-04-03T22:27:53Z

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/ClassicMobileServices.json

@@ -0,0 +1,264 @@
+{


@ruslany is this the first version of this Swagger? Or is there a previous API version? Examples for this presumably new API are missing.

@ruslany We should not expose the ClassicMobileServices.json. It is not really useful for customers and was only used during transition (Zumo -> ARM web apps).

olydis · 2018-04-03T23:47:52Z

@ravbhatnagar new API version, mostly resolved linter errors

naveedaz · 2018-04-05T20:56:02Z

@olydis The "underscore_separated_words" is for the KUDU APIs (https://github.com/projectkudu/kudu/wiki/REST-API). Due to a bug in the swagger generator, they came out as pascalCasing earlier, but the actual API follows the "underscore_separated_words" convention. So the swagger was incorrect before.

ruslany · 2018-04-05T22:07:04Z

@olydis @naveedaz - I removed ClassingMobileServices.json

ravbhatnagar · 2018-04-05T23:36:40Z

Signing off from ARM side since these are all existing APis already deployed.

olydis · 2018-04-06T19:11:10Z

...rce-manager/Microsoft.DomainRegistration/stable/2018-02-01/examples/ListTopLevelDomains.json

+                        "id": "/subscriptions/34adfa4f-cedf-4dc0-ba29-b6d1a69ab345/providers/Microsoft.DomainRegistration/topLevelDomains/com",
+                        "name": "com",
+                        "type": "Microsoft.DomainRegistration/topLevelDomains",
+                        "location": "global",


@ruslany @naveedaz This example (and that for TopLevelDomains_Get) is failing validation right now since the type here is modeled as a ProxyOnlyResource which doesn't have a location field. It this intended or do you think that
a) maybe there is a slight modeling bug or
b) these examples should not have location on them?

Apart from that, please acknowledge whether all changes I point out in my first review (removed example, property changes, etc.) are on purpose. Note that this is a real issue, please fix it 🙂

@olydis are you referring to the "ManagedServiceIdentity is now a flag enumeration"?

correct - just responded to the comment, depending on how that flag enum is supposed to be sent over the wire, it has to be documented accordingly. 🙂

olydis

Outstanding discussion: Adding new API version for Microsoft.Web RP which fixes swagger errors #2802 (comment)
Also, there seems to be a slight issue with example ListTopLevelDomainAgreements.json: The operation requires a agreementOption parameter, but none is provided in the example.

ruslany · 2018-04-11T18:50:31Z

@olydis - fixed the ListTopLevelDomainAgreements example

olydis · 2018-04-11T20:22:12Z

@ruslany Looking good, thanks! :)

#2802 (comment) remains - note that this is critical, no user-friendly code is generated for this as it is right now. In C#:

        /// <summary>
        /// Gets or sets type of managed service identity.
        /// </summary>
        [JsonProperty(PropertyName = "type")]
        public object Type { get; set; }

Cool. What can I set this to now?

ruslany · 2018-04-11T23:59:36Z

@olydis - fixed the type for "ManagedServiceIdentityType",

olydis

thanks for your efforts everyone! 🙂

ruslany · 2018-04-12T21:49:04Z

@olydis - thanks for the thorough review and feedback!

azuresdkciprbot · 2018-04-12T22:51:40Z

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-04-12T23:01:58Z

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

ruslany added 6 commits April 3, 2018 11:27

Adding new API version

14212dc

Adding new API version that hides properties that have the same name …

cb1b093

…as in ARM envelope

Latest updates

33b9a42

Fixing the error definition reference

3cec378

Fixing description for default response

e58c8ad

Add suppression for one RPC 3019 error

818a227

azuresdkci assigned olydis Apr 3, 2018

azuresdkci requested a review from olydis April 3, 2018 18:31

Adding examples for the resource health metadata

b8fffef

AutorestCI mentioned this pull request Apr 3, 2018

[AutoPR web/resource-manager] Adding new API version for Microsoft.Web RP which fixes swagger errors Azure/azure-sdk-for-node#2650

Merged

Fixing API version in the examples

574e458

Azure deleted a comment from AutorestCI Apr 3, 2018

AutorestCI mentioned this pull request Apr 3, 2018

[AutoPR web/resource-manager] Adding new API version for Microsoft.Web RP which fixes swagger errors Azure/azure-sdk-for-python#2340

Merged

Azure deleted a comment from AutorestCI Apr 3, 2018

olydis reviewed Apr 3, 2018

View reviewed changes

olydis added potential-sdk-breaking-change WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Apr 3, 2018

Removing ClassicMobileServices.json

ef4aa89

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Apr 5, 2018

ravbhatnagar removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Apr 5, 2018

olydis reviewed Apr 6, 2018

View reviewed changes

ruslany added 3 commits April 10, 2018 11:08

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

60c482e

Removed location element for top level domain examples

8957477

Removing deprecated method

e07d7dc

olydis reviewed Apr 11, 2018

View reviewed changes

ruslany added 2 commits April 11, 2018 11:49

Fixing the sample for ListTopLevelDomainAgreements API

44bc8e6

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs

e459dad

AutorestCI mentioned this pull request Apr 11, 2018

[AutoPR web/resource-manager] Adding new API version for Microsoft.Web RP which fixes swagger errors Azure/azure-sdk-for-go#1558

Merged

Fixing the type for "ManagedServiceIdentityType",

2860a13

olydis approved these changes Apr 12, 2018

View reviewed changes

olydis merged commit 67edb3f into Azure:master Apr 12, 2018

anuchandy mentioned this pull request Jun 27, 2018

Add new backup discovery API. Fix error codes for source control API,… #3195

Merged

10 tasks

nking-1 mentioned this pull request Aug 10, 2018

WebApps - Add BackupName to backup API models #3560

Merged

10 tasks

Adding new API version for Microsoft.Web RP which fixes swagger errors #2802

Adding new API version for Microsoft.Web RP which fixes swagger errors #2802

Conversation

ruslany commented Apr 3, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

AutorestCI commented Apr 3, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Apr 3, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Apr 3, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Apr 3, 2018 • edited Loading

Automation for azure-sdk-for-node

lmazuel commented Apr 3, 2018

olydis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olydis Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olydis commented Apr 3, 2018 • edited Loading

naveedaz commented Apr 5, 2018

ruslany commented Apr 5, 2018

ravbhatnagar commented Apr 5, 2018

olydis Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olydis left a comment

Choose a reason for hiding this comment

ruslany commented Apr 11, 2018

olydis commented Apr 11, 2018

ruslany commented Apr 11, 2018

olydis left a comment

Choose a reason for hiding this comment

ruslany commented Apr 12, 2018

azuresdkciprbot commented Apr 12, 2018

azuresdkciprbot commented Apr 12, 2018

ruslany commented Apr 3, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Apr 3, 2018 •

edited

Loading

AutorestCI commented Apr 3, 2018 •

edited

Loading

AutorestCI commented Apr 3, 2018 •

edited

Loading

AutorestCI commented Apr 3, 2018 •

edited

Loading

olydis left a comment •

edited

Loading

olydis Apr 3, 2018 •

edited

Loading

olydis commented Apr 3, 2018 •

edited

Loading

olydis Apr 6, 2018 •

edited

Loading