Fixes for code review comments from https://github.com/Azure/azure-rest-api-specs/pull/752 by naveedaz · Pull Request #761 · Azure/azure-rest-api-specs

naveedaz · 2016-11-24T02:09:22Z

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.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

amarzavery · 2016-11-24T02:19:10Z

arm-web/2015-04-01/Domains.json

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


@naveedaz: General comment: Since webapp has a big surface area for their APIs, there are lot of objects. It is hard for us to cover all the objects manually. It would be nice if the individual service teams would diligently mark the model properties as required or provide default values or mark them readOnly wherever necessary. The intent of the initial code review was to get this message across.
What are the required properties of the "Contact" object? Currently everything is optional. It is hard to believe that everything is optional. The service would definitely want some required properties in the "Contact" model
These are the following properties:

"properties": { "addressMailing": { "$ref": "#/definitions/Address", "description": "Mailing address." }, "email": { "description": "Email address.", "type": "string" }, "fax": { "description": "Fax number.", "type": "string" }, "jobTitle": { "description": "Job title.", "type": "string" }, "nameFirst": { "description": "First name.", "type": "string" }, "nameLast": { "description": "Last name.", "type": "string" }, "nameMiddle": { "description": "Middle name.", "type": "string" }, "organization": { "description": "Organization.", "type": "string" }, "phone": { "description": "Phone number.", "type": "string" } }

amarzavery · 2016-11-24T02:23:20Z

arm-web/2016-03-01/DeletedWebApps.json

            },
+            "advancedRoutingEnabled": {
+              "type": "boolean"
+            },


@naveedaz - missing description.

amarzavery · 2016-11-24T02:24:31Z

arm-web/2016-03-01/DeletedWebApps.json

+            },
+            "advancedRoutingEnabled": {
+              "description": "Advanced routing enabled",
+              "type": "boolean"


Can the description be more sensible? What does advanced routing mean?

jianghaolu · 2016-11-24T02:14:05Z

arm-web/2015-04-01/Domains.json

            "createdTime": {
-              "format": "date-time",
              "description": "Domain creation timestamp.",
              "type": "string",


Why is this removed?

fearthecowboy · 2016-11-30T19:42:17Z

  "pattern": "^[-\\\\w\\\\._\\\\(\\\\)]+[^\\\\.]$"

We believe this is over-escaped.

The intent would be to be left with
^[-\w._()]+[^\.]$

which means the literal in JSON should be

^[-\w\._]+[^\\.]$

Refers to: arm-web/2016-03-01/DeletedWebApps.json:1078 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T19:44:11Z

  "description": "Custom action to be executed\r\n            when an auto heal rule is triggered.",

Still seeing some \r in descriptions.

Refers to: arm-web/2016-03-01/DeletedWebApps.json:121 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T19:44:34Z

  "description": "Custom action to be executed\r\n            when an auto heal rule is triggered.",

and collapse extra spaces to single space.

Refers to: arm-web/2016-03-01/DeletedWebApps.json:121 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T19:45:25Z

arm-web/2016-03-01/DeletedWebApps.json

        },
        "privateBytesInKB": {
-          "format": "int32",
          "description": "A rule based on private bytes.",


"format": "int32", [](start = 10, length = 18)

Why did this get removed? This should still be there.

fearthecowboy · 2016-11-30T19:48:21Z

arm-web/2016-03-01/DeletedWebApps.json

          "properties": {
            "deletedTimestamp": {
-              "format": "date-time",
              "description": "Time when the app was deleted.",


"format": "date-time", [](start = 14, length = 22)

This shouldn't have been removed. Generation problem?

fearthecowboy · 2016-11-30T19:48:37Z

arm-web/2016-03-01/DeletedWebApps.json

            "deletedTimestamp": {
-              "format": "date-time",
              "description": "Time when the app was deleted.",
              "type": "string",


"Time when the app was delete [](start = 29, length = 29)

Time zone? UTC?

fearthecowboy · 2016-11-30T19:49:22Z

          "description": "Current state of the app. Read-only.",

Saying this in the description is not advised. In Some languages this isn't even correct in the generated code.

Refers to: arm-web/2016-03-01/DeletedWebApps.json:256 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T19:49:33Z

          "description": "Hostnames associated with the app. Read-only.",

See above.

Refers to: arm-web/2016-03-01/DeletedWebApps.json:261 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T19:49:51Z

          "description": "Name of the repository site. Read-only.",

See above. (and below!) :D

Refers to: arm-web/2016-03-01/DeletedWebApps.json:269 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T19:50:46Z

arm-web/2016-03-01/DeletedWebApps.json

            },
            "lastModifiedTimeUtc": {
-              "format": "date-time",
              "description": "Last time the app was modified, in UTC. Read-only.",


"date-time" [](start = 24, length = 11)

Lost format again. :(

fearthecowboy · 2016-11-30T20:00:27Z

      "description": "Gets or sets a JSON string containing a list of dynamic tags that will be evaluated from user claims in the push registration endpoint.",

Please remove "Gets or sets" from descriptions - the language generator will make appropriate text regarding property accessors.

Refers to: arm-web/2016-03-01/DeletedWebApps.json:602 in c26c550. [](commit_id = c26c550, deletion_comment = False)

fearthecowboy · 2016-11-30T20:00:47Z

arm-web/2016-03-01/DeletedWebApps.json

        },
        "reroutePercentage": {
-          "format": "double",
          "description": "Percentage of the traffic which will be redirected to <code>ActionHostName</code>.",


rmat": "double", [](start = 13, length = 16)

more lost formats!

fearthecowboy

Hey @naveedaz We've done a cursory run over a couple files; we're seeing a few issues with missing format and a couple other things. Could you fix them up and re-generate and push a new set of files?

Thanks!

yugangw-msft · 2016-12-05T22:32:39Z

Please port the support to create linux plan, particularly this commit
/cc: @LukaszStem

yugangw-msft · 2016-12-05T22:42:57Z

Also it appears we still have property names using serverFarm, like here

naveedaz · 2016-12-06T01:34:59Z

@yugangw-msft Changing the property serverFarmId will break a lot of things. The resources in ARM themselves are serverfarms. Is this really needed right now?

yugangw-msft · 2016-12-06T17:43:26Z

@yugangw-msft Changing the property serverFarmId will break a lot of things. The resources in ARM themselves are serverfarms. Is this really needed right now?

@naveedaz Not critical, you can do it later

amarzavery · 2016-12-06T22:08:30Z

@naveedaz - We ran the semantic validator and found some issues. Can you please resolve them

TopLevelDomains.json has a global parameter named "resourceGroupNameParameter" but it is not being used anywhere. Can you please remove it?

Semantically validating https://raw.githubusercontent.com/naveedaz/azure-rest-api-specs/master/arm-web/2015-04-01/TopLevelDomains.json:

Warnings

[ { code: 'UNUSED_PARAMETER',
message: 'Parameter is defined but is not used: #/parameters/resourceGroupNameParameter',
path: [ 'parameters', 'resourceGroupNameParameter' ] } ]

Recommendations.json has an unused model definition "Resource". Can you please remove it from that spec?

Semantically validating https://raw.githubusercontent.com/naveedaz/azure-rest-api-specs/master/arm-web/2016-03-01/Recommendations.json:

Warnings

[ { code: 'UNUSED_DEFINITION',
message: 'Definition is defined but is not used: #/definitions/Resource',
path: [ 'definitions', 'Resource' ] } ]

fearthecowboy · 2016-12-06T22:35:06Z

@naveedaz if you can correct the issue brought up by @amarzavery , we're prepared to merge this PR; and then we can move you on to creating SDK and Unit Tests.

jianghaolu

https://github.com/Azure/azure-rest-api-specs/blame/master/arm-web/2016-08-01/WebApps.json#L13491
scmType should be writable.

jianghaolu · 2016-12-08T02:15:47Z

arm-web/2016-08-01/WebApps.json

@@ -10324,9 +9947,6 @@
            "$ref": "#/parameters/subscriptionIdParameter"
          },
          {
-            "$ref": "#/parameters/resourceGroupNameParameter"
-          },
-          {


Server returns 200 as well. Same for stop(), restart(), startSlot(), restartSot(), stopSlot().

fearthecowboy · 2017-01-11T22:56:45Z

@jianghaolu Are the changes you requested completed?

jianghaolu · 2017-01-11T23:11:44Z

No. The 2 fixes I'm still waiting are:

scmType should be read-write
start(), stop(), restart(), startSlot(), restartSot(), stopSlot() should accept 200

jianghaolu · 2017-01-14T00:18:05Z

Another fix required:
CertitificateDetails shouldn't have an extra layer of properties.

…. Make SCM type read/write. Add Linux App Fx version.

naveedaz · 2017-01-17T22:59:09Z

I have made the changes requested by @jianghaolu. Thanks

jianghaolu · 2017-01-17T23:08:45Z

Thanks Navy, do you know about why CertificateDetails's definition here is different from the actual payload I received from server?

jianghaolu · 2017-01-18T00:43:31Z

Looks like the issue for CertificateDetails is fixed.

naveedaz · 2017-01-18T02:51:07Z

Can we get this merged please?

AutorestCI · 2017-01-18T16:22:56Z

No modification for NodeJS

AutorestCI · 2017-01-18T16:23:01Z

No modification for Python

AutorestCI · 2017-01-18T16:23:02Z

No modification for Ruby

…es for SCMType. (#878) * Make resourcegroup the first parameter if applicable. * Address CR comments from #761 * Fix HTTP status codes for start/stop/restart app and deployment slots. Make SCM type read/write. Add Linux App Fx version. * Rename operation parameters with name options to avoid conflict with Node CLI SDK. Add config snapshots API. Add valid values for SCMType. * Remove obsolete siteAuthSettings properties

…s API (#925) * Fixes for code review comments * Make resourcegroup the first parameter if applicable. * Address CR comments from #761 * Address more comments on PR * Fix HTTP status codes for start/stop/restart app and deployment slots. Make SCM type read/write. Add Linux App Fx version. * Rename operation parameters with name options to avoid conflict with Node CLI SDK. Add config snapshots API. Add valid values for SCMType. * Fix CR recommendations. Remove obsolete siteAuthSettings properties * Surface the correct publishing user API * Expose hosttype for hostnames * Fix folder structure according to convention * Fix paths in composite swagger json * CR comment fix: Changed parameter name to userDetails.

change cache listing and skus

Naveed Aziz added 2 commits November 23, 2016 17:57

Fixes for code review comments

c8391a5

Make resourcegroup the first parameter if applicable.

c26c550

azurecla added the cla-not-required label Nov 24, 2016

amarzavery reviewed Nov 24, 2016

View reviewed changes

jianghaolu suggested changes Nov 28, 2016

View reviewed changes

fearthecowboy reviewed Nov 30, 2016

View reviewed changes

fearthecowboy suggested changes Nov 30, 2016

View reviewed changes

fearthecowboy added the Reviewed-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review label Nov 30, 2016

Naveed Aziz added 2 commits November 30, 2016 19:32

Address CR comments from #761

66457e2

Merge remote-tracking branch 'upstream/master'

8f3244d

jianghaolu approved these changes Dec 5, 2016

View reviewed changes

jianghaolu suggested changes Dec 8, 2016

View reviewed changes

Naveed Aziz added 2 commits December 12, 2016 18:36

Merge remote-tracking branch 'upstream/master'

794b910

Address more comments on PR

8012acb

lmazuel mentioned this pull request Jan 11, 2017

Web app Mgmt new version Azure/azure-sdk-for-python#936

Merged

fearthecowboy approved these changes Jan 11, 2017

View reviewed changes

Naveed Aziz added 2 commits January 17, 2017 14:30

Merge remote-tracking branch 'upstream/master'

7b662e5

Fix HTTP status codes for start/stop/restart app and deployment slots…

68306ef

…. Make SCM type read/write. Add Linux App Fx version.

jianghaolu approved these changes Jan 18, 2017

View reviewed changes

fearthecowboy merged commit fa45501 into Azure:master Jan 18, 2017

romahamu pushed a commit to romahamu/azure-rest-api-specs that referenced this pull request Aug 26, 2019

Merge pull request Azure#761 from waffe7/tmp

4d8f9eb

change cache listing and skus

Conversation

naveedaz commented Nov 24, 2016

PR information

Quality of Swagger

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy left a comment

Choose a reason for hiding this comment

Uh oh!

yugangw-msft commented Dec 5, 2016

Uh oh!

yugangw-msft commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naveedaz commented Dec 6, 2016

Uh oh!

yugangw-msft commented Dec 6, 2016

Uh oh!

amarzavery commented Dec 6, 2016

Warnings

Warnings

Uh oh!

fearthecowboy commented Dec 6, 2016

Uh oh!

jianghaolu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Jan 11, 2017

Uh oh!

jianghaolu commented Jan 11, 2017

Uh oh!

jianghaolu commented Jan 14, 2017

Uh oh!

naveedaz commented Jan 17, 2017

Uh oh!

jianghaolu commented Jan 17, 2017

Uh oh!

jianghaolu commented Jan 18, 2017

Uh oh!

naveedaz commented Jan 18, 2017

Uh oh!

AutorestCI commented Jan 18, 2017

Uh oh!

AutorestCI commented Jan 18, 2017

Uh oh!

AutorestCI commented Jan 18, 2017

Uh oh!

yugangw-msft commented Dec 5, 2016 •

edited

Loading