Adding Management Groups RP Swagger. #1671

muazmian · 2017-09-12T22:45:07Z

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.

Moving Swagger under Resources

msftclas · 2017-09-12T22:45:11Z

@muazmian,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

sergey-shandar · 2017-09-13T00:58:38Z

@ravbhatnagar new spec: resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

ravbhatnagar

Just a couple of comments around read-only attribute for id, name and type properties. This has already been reviewed in person. So ARM signs off.

ravbhatnagar · 2017-09-13T05:34:59Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

+  "ManagementGroupInfo": {
+   "description": "The management group.",
+   "properties": {
+    "id": {


id, name and type properties should be marked as read-only.

ravbhatnagar · 2017-09-13T05:35:26Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

+  "ManagementGroup": {
+   "description": "The management group details.",
+   "properties": {
+    "id": {


id, name and type as read-only

azuresdkciprbot · 2017-09-13T19:57:23Z

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

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

anuchandy

@muazmian really appreciate the fact that you are providing example for each model properties, in-addition to the x-ms-example. Just a suggestion, do you think you can keep only description and merge example content with it? That way IDE intellisense can show more details and api doc would be more cleaner. Just a thought.

I mean something like The ID of the management group for e.g. /providers/Microsoft.Management/managementGroups/20000000-0000-0000-0000-000000000000

anuchandy · 2017-09-14T18:26:01Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

+     }
+    },
+    "nextLink": {
+     "description": "The link (URL) to the next page of results.",


nextLink should be readonly

anuchandy · 2017-09-14T18:48:07Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

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


What exactly the content of nextLink property?

is it (1) fully qualified url (2) relative url (3) something else (e.g. guid)..

Asking because this operation also has a query parameter PageTokenParameter and it description says it will needs to be populated with the value of nextLink present in the current page in-order to get the next page.

Does your service requires user to follow below flow to enumerate all pages?

Get the first page GET /providers/Microsoft.Management/managementGroups. The response can contain value for nextLink e.g. abcd (just a token not a url)

Get the next page by using the original operation url /providers/Microsoft.Management/managementGroups and a query parameter token which is populated with the value of nextLink from the previous page

"x-ms-pageable": { "nextLinkName": "nextLink" }

I am seeing same property defined in three other swaggers.
https://github.com/Azure/azure-rest-api-specs/blob/current/specification/billing/resource-manager/Microsoft.Billing/2017-04-24-preview/billing.json

I don't think there is anything wrong with this.

@muazmian there is nothing wrong here w.r.t the usage of x-ms-pagable. My question is about relationship between PageTokenParameter and nextLink and how service is expecting user to use these properties together.

@anuchandy ..The RP has not yet implemented anything regarding generating NextLink or PageTokenParameter but the plan is that nextLink will be a relative url with $skiptoken.

I am renaming the PageTokenParameter to SkipToken make it more clear.

@muazmian, if an operation is marked as x-ms-pagable then the code generator emits two methods, in your case it generates following two methods:

ManagementGroups { Page<ManagementGroupInfo> List(String skipToken); Page<ManagementGroupInfo> ListNext(String nextLinkUrl, String skipToken); }

So user will use it as

Page<ManagementGroupInfo> firstPage = client.List( /* what should be value for skipToken param?*/); Page<ManagementGroupInfo> nextPage = client.ListNext(firstPage.nextLink, /* what should be value for skipToken param?*/);

Iam trying to understand - If odata $skipToken is already present in the nextLink property when why do we need separate skipToken argument for the method?

@anuchandy

You are right, this is wrong. I have removed "x-ms-parameter-location" from $skipToken. Hopefully this will resolve the issue.

In case this does not fix this, please let me know the actual fix.

I am just helping the team in publishing the Swagger and Azure PS.

removal of "x-ms-parameter-location" will simply make skipTokenParameter global, it goes to client from list method. Should we just remove this property given that it is not used anywhere?

What do you think about moving the $SkipToken out of parameters and only define it for that 1 api that will use it? The reason I am asking is that so skipToken is shown as a query paramter on swagger

sure sounds good..

anuchandy · 2017-09-14T19:31:52Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

+   "in": "query",
+   "required": false,
+   "type": "boolean",
+   "description": "The $recurse=true query string parameter allows clients to request inclusion of entire hierarchy in the response payload."


specify x-ms-parameter-location: method

see above comment

anuchandy · 2017-09-14T19:32:10Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

+   "in": "query",
+   "required": false,
+   "type": "string",
+   "description": "Page continuation token is only used if a previous operation returned a partial result. \nIf a previous response contains a nextLink element, the value of the nextLink element will include a token parameter that specifies a starting point to use for subsequent calls.\n"


please specify x-ms-parameter-location: method, refer https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-parameter-location

Is this a new requirement? None of our older RPs has this property defined.

this is not a new requirement - if you don't apply this attribute then these become properties of the client (ManagementGroups.cs), only the global properties (e.g. apiVersion, subscriptionId) which are applicable across all operations are supposed to be in the client level.

anuchandy · 2017-09-14T19:32:25Z

...ification/resources/resource-manager/Microsoft.Management/2017-08-31-preview/management.json

+   "enum": [
+    "children"
+   ],
+   "description": "The $expand=children query string parameter allows clients to request inclusion of children in the response payload."


specify x-ms-parameter-location: method

see above comment

azuresdkciprbot · 2017-09-19T02:05:41Z

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

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

AutorestCI · 2017-09-19T20:14:07Z

Azure/azure-sdk-for-python#1451

AutorestCI · 2017-09-19T20:14:13Z

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

AutorestCI · 2017-09-19T20:14:25Z

Azure/azure-sdk-for-ruby#983

muazmian added 2 commits September 8, 2017 12:09

Adding Microsoft.Management swagger with examples

4e13e54

Updating Swagger to ARM standards.

d69565b

Moving Swagger under Resources

msftclas added the cla-not-required label Sep 12, 2017

azuresdkci assigned sergey-shandar Sep 12, 2017

sergey-shandar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 13, 2017

ravbhatnagar approved these changes Sep 13, 2017

View reviewed changes

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review ReadyForSDKReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Sep 13, 2017

Updating id, name and type to readonly as suggested by ARM.

25631fd

sergey-shandar assigned anuchandy and unassigned sergey-shandar Sep 14, 2017

anuchandy reviewed Sep 14, 2017

View reviewed changes

anuchandy requested changes Sep 14, 2017

View reviewed changes

anuchandy reviewed Sep 14, 2017

View reviewed changes

muazmian and others added 4 commits September 14, 2017 16:36

Changes after SDK Review.

6f47d99

Updating skipToken property.

786b207

Moving SkipToken paramter.

bbc452f

Merge branch 'current' into current

68f32db

anuchandy approved these changes Sep 19, 2017

View reviewed changes

anuchandy merged commit 6274e33 into Azure:current Sep 19, 2017

AutorestCI mentioned this pull request Sep 19, 2017

Automatic PR from RestAPI-PR1671 Azure/azure-sdk-for-python#1451

Closed

AutorestCI mentioned this pull request Sep 19, 2017

Automatic PR from RestAPI-PR1671 Azure/azure-sdk-for-ruby#983

Merged

muazmian mentioned this pull request Oct 13, 2017

Microsoft.Management InMemory and Scenario Tests Azure/azure-sdk-for-net#3777

Closed

8 tasks

rajshah11 mentioned this pull request Feb 10, 2018

Move ManagementGroups out of Resources to a separate folder #2469

Merged

10 tasks

Adding Management Groups RP Swagger. #1671

Adding Management Groups RP Swagger. #1671

Uh oh!

Conversation

muazmian commented Sep 12, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Sep 12, 2017

Uh oh!

sergey-shandar commented Sep 13, 2017

Uh oh!

ravbhatnagar left a comment

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!

azuresdkciprbot commented Sep 13, 2017

Uh oh!

anuchandy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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!

azuresdkciprbot commented Sep 19, 2017

Uh oh!

AutorestCI commented Sep 19, 2017

Uh oh!

AutorestCI commented Sep 19, 2017

Uh oh!

AutorestCI commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

muazmian commented Sep 12, 2017 •

edited by azuresdkci

Loading

anuchandy left a comment •

edited

Loading