Expose Patch operations. Expose missing provider level operations #1816

naveedaz · 2017-10-06T18:47:03Z

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.

…from swagger

…vice and gateway API

…operties

…tatus object

…arameters

ravbhatnagar · 2017-10-13T21:00:11Z

@sergey-shandar - Can you confirm the behavior on #1 above and if SDK team is fine with this?
for #2, I chatted with @naveedaz and the RP behavior is that it does not update properties which are null or not sent on the subsequent PUT. So unintentional update of goal state due to SDK not understanding and ignoring some properties will not happen.
For #3, yes please check.

naveedaz · 2017-10-13T21:34:49Z

@sergey-shandar, @ravbhatnagar I generated the SDK and indeed x-ms-mutability does not affect whether property is required. So for now the location will not be required. This is not necessarily breaking but calls from client without location could make it to service, where they will fail with the appropriate message. The validation would move to service instead of client.
Resource models specifically for patch verb would be a fair bit of work. In the interest of time, I say it is fine to live with the validation moving to service temporarily (customers already specify location when creating so it should not matter much). In a follow up PR I will consolidate common definitions in a single file instead of having it duplicated in all per collection files. It will be much easier to tackle the patching specific resource models then. Discussed this with @ravbhatnagar offline. @sergey-shandar What do you think?

ravbhatnagar · 2017-10-14T00:53:57Z

The proposal by @naveedaz sounds good to me. @sergey-shandar - please confirm fro SDK side.

sergey-shandar · 2017-10-17T18:17:05Z

@shawns1 another PR where CI is not shown. Could you have a look?

sergey-shandar · 2017-10-17T18:27:06Z

...source-manager/Microsoft.CertificateRegistration/2015-08-01/AppServiceCertificateOrders.json

@@ -1341,7 +1465,11 @@
        },
        "location": {
          "description": "Resource Location.",
-          "type": "string"
+          "type": "string",
+          "x-ms-mutability": [


I know, we recommend to use x-ms-mutability. AutoRest SDK code generation doesn't use the extension for now but it may use it in the future. It's not an SDK breaking change. In the same time, removing required field may change an order of constructor parameters.

sergey-shandar · 2017-10-17T18:27:37Z

specification/web/resource-manager/Microsoft.DomainRegistration/2015-04-01/Domains.json

@@ -1130,9 +1191,6 @@
    },
    "Resource": {
      "description": "Azure resource. This resource is tracked in Azure Resource Manager",
-      "required": [
-        "location"


Same here, it's a potential SDK breaking change.

sergey-shandar · 2017-10-17T18:28:05Z

specification/web/resource-manager/Microsoft.Web/2016-03-01/Certificates.json

@@ -451,9 +452,6 @@
    },
    "Resource": {
      "description": "Azure resource. This resource is tracked in Azure Resource Manager",
-      "required": [
-        "location"


A potential SDK breaking change.

sergey-shandar · 2017-10-17T18:28:33Z

specification/web/resource-manager/Microsoft.Web/2016-03-01/Provider.json

@@ -32,7 +32,7 @@
          "200": {
            "description": "OK",
            "schema": {
-              "$ref": "#/definitions/Object"
+              "type": "object"


an SDK breaking change.

Change from #/definitions/Object is SDK but recommended by Azure SDK/ARM

sergey-shandar · 2017-10-17T18:28:39Z

specification/web/resource-manager/Microsoft.Web/2016-03-01/Provider.json

@@ -92,7 +92,7 @@
          "200": {
            "description": "OK",
            "schema": {
-              "$ref": "#/definitions/Object"
+              "type": "object"


an SDK breaking change.

Change from #/definitions/Object is SDK but recommended by Azure SDK/ARM

sergey-shandar · 2017-10-17T18:35:25Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

@@ -18256,11 +18558,6 @@
          "format": "double",
          "description": "Value of counter at a certain time.",
          "type": "number"
-        },
-        "coreCount": {
-          "format": "int32",


The property was removed.

sergey-shandar · 2017-10-17T18:35:42Z

specification/web/resource-manager/Microsoft.Web/2016-08-01/WebApps.json

@@ -18958,9 +19256,6 @@
    },
    "Resource": {
      "description": "Azure resource. This resource is tracked in Azure Resource Manager",
-      "required": [
-        "location"


potential SDK breaking change.

sergey-shandar · 2017-10-17T18:37:32Z

specification/web/resource-manager/Microsoft.Web/2016-09-01/AppServiceEnvironments.json

-        "x-ms-pageable": {
-          "nextLinkName": "nextLink"
-        }
+ "swagger": "2.0",


Changing formatting of the document is unnecessary changes and it's really hard to review. Could you format the document as it was before?

sergey-shandar · 2017-10-17T18:40:05Z

specification/web/resource-manager/Microsoft.Web/2016-09-01/AppServicePlans.json

@@ -3214,7 +3433,8 @@
              "readOnly": true
            },
            "certBlob": {
-              "description": "A certificate file (.cer) blob containing the public key of the private key used to authenticate a \n Point-To-Site VPN connection.",
+              "format": "byte",
+              "description": "A certificate file (.cer) blob containing the public key of the private key used to authenticate a \nPoint-To-Site VPN connection.",


A potential SDK breaking changes.

Client validation for (uuid, byte ) will cause SDK breaks, but is acceptable and actually a good change.

sergey-shandar · 2017-10-17T18:40:27Z

specification/web/resource-manager/Microsoft.Web/2016-09-01/AppServicePlans.json

@@ -2074,9 +2288,6 @@
    },
    "Resource": {
      "description": "Azure resource. This resource is tracked in Azure Resource Manager",
-      "required": [
-        "location"
-      ],


A potential SDK breaking changes.

azuresdkciprbot · 2017-10-20T17:25:25Z

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): 105
After the PR: Warning(s): 0 Error(s): 88

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2017-10-20T17:53:52Z

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): 105
After the PR: Warning(s): 0 Error(s): 88

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

naveedaz · 2017-10-20T18:03:08Z

consumes/produces is now declared only at the collection level unless it is different for a specific operation.
WebJobId was a false query parameter, that should not have existed. The service does not use it.
Client validation for (uuid, byte ) will cause SDK breaks, but is acceptable and actually a good change.
ErrorEntity was incorrectly under properties property. ARM expects it in the envelope.
Change from #/definitions/Object is SDK but recommended by Azure SDK/ARM
CommonDefinitions.json is now added. The generator tool just spits the common models in this file. References to this file are not yet used, since it was complicating the PR too much. I will add that as a separate PR.
Resource model does not have identity anymore. Instead it is on the only tracked resource 'Site' that actually uses it. SDK wise the optional property will be removed for all the tracked resources that do not use a managed service identity. (AppServiceCertificateResource, AppServiceCertificateOrder, Certificate
Domai, AppServiceEnvironmentResource, AppServicePlan).
DomainOwnershipIdentifier is not a tracked resource. Marked it as a proxyOnlyResource. SDK change wise, location is no longer a required property for this resource.

sergey-shandar

There a lot of SDK breaking changes but otherwise the changes are really good.

AutorestCI · 2017-10-23T19:33:07Z

No modification for AutorestCI/azure-sdk-for-node

AutorestCI · 2017-10-23T19:33:12Z

Azure/azure-sdk-for-python#1561

AutorestCI · 2017-10-23T19:35:52Z

Azure/azure-sdk-for-ruby#1082

naveedaz · 2017-10-23T22:28:10Z

MSI is supported only for the site resource. It was intentional to remove it from all tracked resources except for Site.

Comments from above
7. Resource model does not have identity anymore. Instead it is on the only tracked resource 'Site' that actually uses it. SDK wise the optional property will be removed for all the tracked resources that do not use a managed service identity. (AppServiceCertificateResource, AppServiceCertificateOrder, Certificate
Domai, AppServiceEnvironmentResource, AppServicePlan).

lmazuel · 2017-10-23T22:32:00Z

@naveedaz Yes, I saw afterward and removed my comment (not fast enough :)). Thanks for the confirmation :)

I see ContinuousWebJob.settings has changed type, I guess it's on purpose as well.

updated email regex

Naveed Aziz added 26 commits May 8, 2017 17:09

Fix regression where Get source control API was accidentally removed …

7a491a8

…from swagger

Merge remote-tracking branch 'upstream/master'

c211666

Add PublicCertificates API, MSDeploy API and remove obsolete microSer…

5d1d2ec

…vice and gateway API

Add public certificates, Functions Admin Token and MSDeploy APIs

dc0352d

Revert resource definition changes and description change for site pr…

549a9de

…operties

Merge remote-tracking branch 'upstream/current' into current

39ceb0d

Make type and name readonly properties. Add ARM envelope to MSDeployS…

ba5ad4f

…tatus object

Merge remote-tracking branch 'upstream/current' into current

f9084ca

Fix missing quotation issue.

a82a87c

Merge remote-tracking branch 'upstream/current' into current

7ed3428

Add long running operation to MS deploy

e0fd751

Merge remote-tracking branch 'upstream/current' into current

7fa0073

Make MsDeploy and MsDeployLog Azure resources

4dab1f8

Merge remote-tracking branch 'upstream/current' into current

2101e70

Fix some AutoRest validation issues. Use dictionary for msdeploy.setp…

d4abb2f

…arameters

Ensure models are same across all schemas in Microsoft.Web

b47c0d5

Fix azure resource type across all models

26c40f9

Merge remote-tracking branch 'upstream/current' into current

d068b39

Remove conflicting ListOperations. It is not used for public Azure.

49b1967

Merge remote-tracking branch 'upstream/current' into current

39cbd9e

Merge remote-tracking branch 'upstream/current' into current

462f730

Merge conflicts

5f7e16e

Merge remote-tracking branch 'upstream/current' into current

e4e2990

Expose Patch operations. Expose missing provider level operations

0937c56

Merge remote-tracking branch 'upstream/current' into current

c01b88b

Fix static validation issues.

98f90e5

azuresdkci assigned sergey-shandar Oct 6, 2017

sergey-shandar added the potential-sdk-breaking-change label Oct 9, 2017

Navy Aziz added 2 commits October 9, 2017 11:06

Remove required for location on Azure resource

512ff6a

Merge remote-tracking branch 'upstream/current' into current

6eb2e27

ravbhatnagar added the ReadyForSDKReview label Oct 12, 2017

Navy Aziz added 2 commits October 16, 2017 14:05

Merge upstream changes

c8b9172

Merge remote-tracking branch 'upstream/current' into current

20c083c

sergey-shandar suggested changes Oct 17, 2017

View reviewed changes

Navy Aziz added 7 commits October 19, 2017 17:08

Move common definitions to single location.

ea96df9

Fix spurious references to commonDefinitions

3680787

Merge remote-tracking branch 'upstream/current' into current

3883327

Address CR issues.

f9bffdc

Revert references to common definitions

4866660

Merge remote-tracking branch 'upstream/current' into current

e7d6ee8

Revert Model Resource to have required location.

9478357

Fixing a few straglers

e1592df

sergey-shandar approved these changes Oct 23, 2017

View reviewed changes

sergey-shandar merged commit 54ffab4 into Azure:current Oct 23, 2017

AutorestCI mentioned this pull request Oct 23, 2017

[Web] Update Azure/azure-sdk-for-python#1561

Closed

AutorestCI mentioned this pull request Oct 23, 2017

Automatic PR from RestAPI-PR1816 Azure/azure-sdk-for-ruby#1082

Closed

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

Merge pull request #1816 from ankisho/emailregexupdate

9b404ab

updated email regex

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose Patch operations. Expose missing provider level operations #1816

Expose Patch operations. Expose missing provider level operations #1816

naveedaz commented Oct 6, 2017 •

edited by azuresdkci

Loading

ravbhatnagar commented Oct 13, 2017

naveedaz commented Oct 13, 2017

ravbhatnagar commented Oct 14, 2017

sergey-shandar commented Oct 17, 2017

sergey-shandar Oct 17, 2017

sergey-shandar Oct 17, 2017

sergey-shandar Oct 17, 2017

sergey-shandar Oct 17, 2017

naveedaz Oct 20, 2017

sergey-shandar Oct 17, 2017

naveedaz Oct 20, 2017

sergey-shandar Oct 17, 2017

sergey-shandar Oct 17, 2017

sergey-shandar Oct 17, 2017

sergey-shandar Oct 17, 2017

naveedaz Oct 20, 2017

sergey-shandar Oct 17, 2017

azuresdkciprbot commented Oct 20, 2017

azuresdkciprbot commented Oct 20, 2017

naveedaz commented Oct 20, 2017 •

edited

Loading

sergey-shandar left a comment

AutorestCI commented Oct 23, 2017

AutorestCI commented Oct 23, 2017

AutorestCI commented Oct 23, 2017

naveedaz commented Oct 23, 2017

lmazuel commented Oct 23, 2017

Expose Patch operations. Expose missing provider level operations #1816

Expose Patch operations. Expose missing provider level operations #1816

Conversation

naveedaz commented Oct 6, 2017 • edited by azuresdkci Loading

PR information

Quality of Swagger

ravbhatnagar commented Oct 13, 2017

naveedaz commented Oct 13, 2017

ravbhatnagar commented Oct 14, 2017

sergey-shandar commented Oct 17, 2017

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

azuresdkciprbot commented Oct 20, 2017

azuresdkciprbot commented Oct 20, 2017

naveedaz commented Oct 20, 2017 • edited Loading

sergey-shandar left a comment

Choose a reason for hiding this comment

AutorestCI commented Oct 23, 2017

AutorestCI commented Oct 23, 2017

AutorestCI commented Oct 23, 2017

naveedaz commented Oct 23, 2017

lmazuel commented Oct 23, 2017

naveedaz commented Oct 6, 2017 •

edited by azuresdkci

Loading

naveedaz commented Oct 20, 2017 •

edited

Loading