Move subscriptionId to global #2670

vivsriaus · 2018-03-15T20:44: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.
- 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-03-15T20:45:45Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-03-15T21:02:50Z

Automation for azure-libraries-for-java

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-libraries-for-java#293

AutorestCI · 2018-03-15T21:37:12Z

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#2029

AutorestCI · 2018-03-15T22:07:48Z

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#1334

sergey-shandar · 2018-03-16T18:32:13Z

@vivsriaus is there a good reason to introduce SDK breaking changes in the old stable API version?

sergey-shandar · 2018-03-16T18:42:41Z

...fication/resources/resource-manager/Microsoft.Resources/stable/2016-06-01/subscriptions.json

-            "required": true,
-            "type": "string",
-            "description": "The ID of the target subscription."
+            "$ref": "#/parameters/ApiVersionParameter"
          },


@vivsriaus the order of parameters does matter for some SDKs. Please, revert back to

the subscription is should be the first parameter,

the api version should be the second parameter.

You may change the order in the next api version.

@sergey-shandar Fixed

vivsriaus · 2018-03-16T19:12:16Z

@sergey-shandar With regards to your comment about introducing breaking changes, we're doing this because our file was flagged as non compliant per Mohammed Zehgir. (non compliant because we were not using subId as a global parameter). Also, can you explain how this is considered a breaking change? The subscriptionId parameter is still required in the modified operations, right?

sergey-shandar · 2018-03-16T20:32:25Z

@vivsriaus it's breaking changes for SDKs because the parameters and their order are changed. We generate Azure SDK using the swagger files. So imagine Azure SDK C# DLL, one version of this DLL has a function with one parameter type and the new version of this DLL has a function with different parameter types and order of the parameters is changed.

sergey-shandar · 2018-03-16T20:39:01Z

@vivsriaus thanks for changing the order back. It's not an SDK breaking change anymore.

sergey-shandar · 2018-03-16T20:58:00Z

...fication/resources/resource-manager/Microsoft.Resources/stable/2016-06-01/subscriptions.json

+      "required": true,
+      "type": "string",
+      "description": "The ID of the target subscription."
+    },


please, add "x-ms-parameter-location": "client"

What does this give us?

Never mind, I get it now

sergey-shandar · 2018-03-16T20:58:55Z

...fication/resources/resource-manager/Microsoft.Resources/stable/2016-06-01/subscriptions.json

+      "required": true,
+      "type": "string",
+      "description": "The ID of the target subscription."
+    },
    "ApiVersionParameter": {
      "name": "api-version",
      "in": "query",


please, add "x-ms-parameter-location": "client" to the ApiVersionParameter as well.

…eters

lmazuel · 2018-03-20T15:34:20Z

@vivsriaus @sergey-shandar This is a massive breaking change :(. This is moving from a local parameter to a client parameter :(

lmazuel · 2018-03-20T15:35:40Z

But yeah, it's written client, so what the hell Python is generating out of this :(
............

lmazuel · 2018-03-20T15:37:25Z

@AutorestCI regenerate azure-sdk-for-python

lmazuel · 2018-03-20T16:15:45Z

Ok, this is definitely a massive breaking change....

Move subscriptionId to global

81560dc

azuresdkci assigned sergey-shandar Mar 15, 2018

AutorestCI mentioned this pull request Mar 15, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-libraries-for-java#304

Merged

mozehgir requested a review from tfitzmac March 15, 2018 21:08

sergey-shandar removed the request for review from tfitzmac March 15, 2018 21:17

AutorestCI mentioned this pull request Mar 15, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-sdk-for-python#2188

Merged

AutorestCI mentioned this pull request Mar 15, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-sdk-for-go#1274

Closed

sergey-shandar added the potential-sdk-breaking-change label Mar 16, 2018

sergey-shandar suggested changes Mar 16, 2018

View reviewed changes

Move subId as the first parameter

8f5f588

vivsriaus requested a review from a user March 16, 2018 19:02

vivsriaus requested review from tbombach and vladbarosan as code owners March 16, 2018 19:02

AutorestCI mentioned this pull request Mar 16, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-sdk-for-go#1298

Closed

sergey-shandar removed the potential-sdk-breaking-change label Mar 16, 2018

sergey-shandar reviewed Mar 16, 2018

View reviewed changes

sergey-shandar added the potential-sdk-breaking-change label Mar 16, 2018

Add "x-ms-parameter-location": "client" to subId and apiVersion param…

f9d0f61

…eters

AutorestCI mentioned this pull request Mar 16, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-sdk-for-go#1309

Closed

sergey-shandar approved these changes Mar 19, 2018

View reviewed changes

sergey-shandar merged commit 6aeb05a into Azure:master Mar 19, 2018

AutorestCI mentioned this pull request Mar 19, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-sdk-for-go#1334

Merged

AutorestCI mentioned this pull request Mar 20, 2018

[AutoPR resources/resource-manager] Move subscriptionId to global Azure/azure-sdk-for-python#2234

Merged

lmazuel mentioned this pull request Mar 20, 2018

Fix subscription paramter in SubscriptionClient #2710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move subscriptionId to global #2670

Move subscriptionId to global #2670

vivsriaus commented Mar 15, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

sergey-shandar commented Mar 16, 2018

sergey-shandar Mar 16, 2018 •

edited

Loading

vivsriaus Mar 16, 2018

vivsriaus commented Mar 16, 2018

sergey-shandar commented Mar 16, 2018 •

edited

Loading

sergey-shandar commented Mar 16, 2018 •

edited

Loading

sergey-shandar Mar 16, 2018

vivsriaus Mar 16, 2018

vivsriaus Mar 16, 2018

sergey-shandar Mar 16, 2018

lmazuel commented Mar 20, 2018

lmazuel commented Mar 20, 2018 •

edited

Loading

lmazuel commented Mar 20, 2018

lmazuel commented Mar 20, 2018

Move subscriptionId to global #2670

Move subscriptionId to global #2670

Conversation

vivsriaus commented Mar 15, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

AutorestCI commented Mar 15, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Mar 15, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Mar 15, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Mar 15, 2018 • edited Loading

Automation for azure-sdk-for-go

sergey-shandar commented Mar 16, 2018

sergey-shandar Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

vivsriaus Mar 16, 2018

Choose a reason for hiding this comment

vivsriaus commented Mar 16, 2018

sergey-shandar commented Mar 16, 2018 • edited Loading

sergey-shandar commented Mar 16, 2018 • edited Loading

sergey-shandar Mar 16, 2018

Choose a reason for hiding this comment

vivsriaus Mar 16, 2018

Choose a reason for hiding this comment

vivsriaus Mar 16, 2018

Choose a reason for hiding this comment

sergey-shandar Mar 16, 2018

Choose a reason for hiding this comment

lmazuel commented Mar 20, 2018

lmazuel commented Mar 20, 2018 • edited Loading

lmazuel commented Mar 20, 2018

lmazuel commented Mar 20, 2018

vivsriaus commented Mar 15, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

AutorestCI commented Mar 15, 2018 •

edited

Loading

sergey-shandar Mar 16, 2018 •

edited

Loading

sergey-shandar commented Mar 16, 2018 •

edited

Loading

sergey-shandar commented Mar 16, 2018 •

edited

Loading

lmazuel commented Mar 20, 2018 •

edited

Loading