Fixing global parameters for siterecovery and resourcehealth #1753

amarzavery · 2017-09-27T18:10:47Z

No description provided.

… parameters and not as client properties

…Moreover it is not being used anywhere. So removing it.

veronicagg · 2017-09-27T21:01:15Z

...tion/resourcehealth/resource-manager/Microsoft.ResourceHealth/2015-01-01/resourcehealth.json

@@ -548,13 +548,6 @@
      "description": "The name of the resource group.",
      "x-ms-parameter-location": "method"
    },
-    "ResourceTypeParameter": {


it appears this wasn't been used in the spec or any others in the repo, right? so removing it shouldn't cause any harm.

it was not being used and having it in the global parameters section was causing this to be a client property.

veronicagg · 2017-09-27T21:02:26Z

...tion/resourcehealth/resource-manager/Microsoft.ResourceHealth/2017-07-01/resourcehealth.json

@@ -564,13 +564,6 @@
      "description": "The name of the resource group.",
      "x-ms-parameter-location": "method"
    },
-    "ResourceTypeParameter": {


same situation as above here?

veronicagg · 2017-09-27T21:04:32Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

@@ -14122,14 +14122,16 @@
      "in": "path",
      "description": "The name of the resource group where the recovery services vault is present.",
      "required": true,
-      "type": "string"
+      "type": "string",
+      "x-ms-parameter-location": "method"


what issue was not having this causing? what's the breaking change on generated code due to this?

new SiteRecoveryManagementClient(credentials, subscriptionId, resourceGroupName, resourceName);
This makes the client require resourceGroupName and resourceName in the client constructor. Both of these become client properties and that is incorrect. I had filed an issue in the linter repo about this. If it is management-plane swagger then the linter should warn if it sees anything else apart from subscriptionId and apiVersion and does not have x-ms-parameter-location set to method. Azure/azure-openapi-validator#84

As responded over the mail thread:

As per our scenarios, customers are more likely to work in the context of a single ResourceName, so, It makes more sense to have ResourceGroupName and ResourceName in global parameters section for recoveryservicessiterecovery specification. For more context we have:

https://github.com/Azure/azure-rest-api-specs/blob/current/specification/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json which defines CRUD on ResourceName and hence, ResourceGroupName and ResourceName are already method parameters.

However, specification at https://github.com/Azure/azure-rest-api-specs/tree/current/specification/recoveryservicessiterecovery/resource-manager is mostly sused by customer in the context of a single ResourceName. So, initializing ResourceGroupName and ResourceName at the time of client initialization aligns better to our customer scenario.

Moreover, any change as suggested in the thread would be a breaking change for our existing clients like PowerShell and Ibiza (requires passing ResourceGroupName and ResourceName explicitly as input to every call rather than just once at sdk client initialization) as I can understand, which we would not be able to take at this point.
Let us know if we are missing some point to understand here.

Thanks,
Avneesh

veronicagg · 2017-09-27T21:06:27Z

@mukum would you be ok with these updates?

…ters wherever applicable in service fabric management plane.

…the generated code.

veronicagg · 2017-09-28T20:20:04Z

@amarzavery is there a reason for adding Service Fabric changes to this PR? It'd be good to split them in another PR, since we'd want to have someone from that team take a look.

amarzavery · 2017-09-28T20:24:15Z

All of them are fixing the same thing. Hence grouped them together.

veronicagg · 2017-09-28T20:25:19Z

@sankalpsoni for ResourceHealth spec update, any concerns with the update?

amarzavery · 2017-09-28T20:26:45Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

-        "2016-09-01"
-      ],
-      "default": "2016-09-01"
+      "type": "string"


@samedder - Can you please take a look at changes I made in the service fabric resource-manager spec.

Some of the global parameters missed the "x-ms-parameter-location": "method" extension.

The enum array is not required in the first place and it had an incorrect value as well. If the value has to be present then it should be in sync with the api-version in the file path that is "2017-07-01-preview" in this case.

@chingchenmsft needs to sign off on this. I believe this may break our clients so he should verify.

@juhacket - I believe these changes were made by you and merged in the PR #1717. Can you please take a look?

Can you fix the description as well? "This is a required parameter and it's value must be "2016-09-01"."

amarzavery · 2017-09-28T20:28:15Z

specification/servicefabric/data-plane/Microsoft.ServiceFabric/5.6/servicefabric.json

@@ -7399,7 +7399,7 @@
    },
    "ServicePlacementPreferPrimaryDomainPolicyDescription": {
      "x-ms-discriminator-value": "PreferPrimaryDomain",
-      "description": "Describes the policy to be used for placement of a Service Fabric service where the service's Primary replicas should optimally be placed in a particular domain.\n\nThis placement policy is usually used with fault domains in scenarios where the Service Fabric cluster is geographically distributed in order to indicate that a service�s primary replica should be located in a particular fault domain, which in geo-distributed scenarios usually aligns with regional or datacenter boundaries. Note that since this is an optimization it is possible that the Primary replica may not end up located in this domain due to failures, capacity limits, or other constraints.\n",
+      "description": "Describes the policy to be used for placement of a Service Fabric service where the service's Primary replicas should optimally be placed in a particular domain.\n\nThis placement policy is usually used with fault domains in scenarios where the Service Fabric cluster is geographically distributed in order to indicate that a service's primary replica should be located in a particular fault domain, which in geo-distributed scenarios usually aligns with regional or datacenter boundaries. Note that since this is an optimization it is possible that the Primary replica may not end up located in this domain due to failures, capacity limits, or other constraints.\n",


@samedder - In this commit I have replaced the MS Word style single quote with simple single quote as it generates wonky characters in the generated client documentation.

samedder · 2017-09-28T22:21:23Z

...servicefabric/resource-manager/Microsoft.ServiceFabric/2017-07-01-preview/servicefabric.json

-        "2016-09-01"
-      ],
-      "default": "2016-09-01"
+      "type": "string"


@chingchenmsft needs to sign off on this. I believe this may break our clients so he should verify.

QingChenmsft · 2017-09-28T22:25:54Z

@juhacket is the owner now, need his sign off.

azuresdkciprbot · 2017-09-29T17:08:48Z

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): 23

File: specification/resourcehealth/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 1

File: specification/servicefabric/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 652
After the PR: Warning(s): 0 Error(s): 652

File: specification/servicefabric/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 40
After the PR: Warning(s): 0 Error(s): 40

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

amarzavery · 2017-09-29T20:11:52Z

@lmazuel - Can you help take a look at the python generator issue over here.

Traceback (most recent call last):
  File "/SwaggerToSdkMain.py", line 142, in <module>
    main()
  File "/SwaggerToSdkMain.py", line 139, in main
    args.autorest_bin)
  File "/SwaggerToSdkMain.py", line 36, in generate_sdk
    config = read_config(sdk_repo.working_tree_dir, config_path)
  File "/SwaggerToSdkCore.py", line 435, in read_config
    return json.loads(config_fd.read())
  File "/usr/lib/python3.6/json/__init__.py", line 354, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python3.6/json/decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python3.6/json/decoder.py", line 355, in raw_decode
    obj, end = self.scan_once(s, idx)
json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 45 column 5 (char 1387)

azuresdkciprbot · 2017-09-29T21:32:11Z

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): 23

File: specification/resourcehealth/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 1

File: specification/servicefabric/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 652
After the PR: Warning(s): 0 Error(s): 652

File: specification/servicefabric/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 40
After the PR: Warning(s): 0 Error(s): 40

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

lmazuel · 2017-10-02T23:15:16Z

@amarzavery CI fixed

veronicagg · 2017-10-03T23:00:19Z

@amarzavery I believe we're missing response from ResourceHealth on the change. SiteRecovery changes would need to be updated based on the response from the team. Are we signed off from Service Fabric side?

amarzavery · 2017-10-04T17:13:58Z

closing this in favor of #1793 and #1794

amarzavery added 2 commits September 27, 2017 11:08

siterecovery should have resourcegroupName and resourcename as method…

4a49385

… parameters and not as client properties

resourceType should be a method parameter and not a client property. …

7b69658

…Moreover it is not being used anywhere. So removing it.

amarzavery added the potential-sdk-breaking-change label Sep 27, 2017

azuresdkci assigned veronicagg Sep 27, 2017

veronicagg suggested changes Sep 27, 2017

View reviewed changes

veronicagg added the Site Recovery label Sep 27, 2017

adding "x-ms-parameter-location": "method" extension to global parame…

8983664

…ters wherever applicable in service fabric management plane.

amarzavery force-pushed the fixglobalParams branch from 4931eba to 8983664 Compare September 28, 2017 02:32

replacing special character � with ' as this causes jshint issues in …

024fd4f

…the generated code.

amarzavery commented Sep 28, 2017

View reviewed changes

samedder requested changes Sep 28, 2017

View reviewed changes

Updating documentation as per review feedback.

a80c2da

juhacket approved these changes Sep 29, 2017

View reviewed changes

This was referenced Oct 4, 2017

Fix global parameters with x-ms-parameter-location for sf. #1794

Merged

removing resourcetype parameter from global prameters as it is not referenced anywhere. #1795

Merged

amarzavery closed this Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing global parameters for siterecovery and resourcehealth #1753

Fixing global parameters for siterecovery and resourcehealth #1753

amarzavery commented Sep 27, 2017 •

edited by azuresdkci

Loading

veronicagg Sep 27, 2017

amarzavery Sep 27, 2017 •

edited

Loading

veronicagg Sep 27, 2017

amarzavery Sep 27, 2017

veronicagg Sep 27, 2017

amarzavery Sep 27, 2017

avneeshrai Oct 2, 2017

veronicagg commented Sep 27, 2017

veronicagg commented Sep 28, 2017

amarzavery commented Sep 28, 2017

veronicagg commented Sep 28, 2017

amarzavery Sep 28, 2017 •

edited

Loading

samedder Sep 28, 2017

amarzavery Sep 28, 2017 •

edited

Loading

juhacket Sep 29, 2017

amarzavery Sep 28, 2017

samedder Sep 28, 2017

QingChenmsft commented Sep 28, 2017

azuresdkciprbot commented Sep 29, 2017

amarzavery commented Sep 29, 2017

azuresdkciprbot commented Sep 29, 2017

lmazuel commented Oct 2, 2017

veronicagg commented Oct 3, 2017

amarzavery commented Oct 4, 2017

Fixing global parameters for siterecovery and resourcehealth #1753

Fixing global parameters for siterecovery and resourcehealth #1753

Conversation

amarzavery commented Sep 27, 2017 • edited by azuresdkci Loading

Choose a reason for hiding this comment

amarzavery Sep 27, 2017 • 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

veronicagg commented Sep 27, 2017

veronicagg commented Sep 28, 2017

amarzavery commented Sep 28, 2017

veronicagg commented Sep 28, 2017

amarzavery Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarzavery Sep 28, 2017 • 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

QingChenmsft commented Sep 28, 2017

azuresdkciprbot commented Sep 29, 2017

amarzavery commented Sep 29, 2017

azuresdkciprbot commented Sep 29, 2017

lmazuel commented Oct 2, 2017

veronicagg commented Oct 3, 2017

amarzavery commented Oct 4, 2017

amarzavery commented Sep 27, 2017 •

edited by azuresdkci

Loading

amarzavery Sep 27, 2017 •

edited

Loading

amarzavery Sep 28, 2017 •

edited

Loading

amarzavery Sep 28, 2017 •

edited

Loading