Adding Workbooks resource type by ericc1103 · Pull Request #2775 · Azure/azure-rest-api-specs

ericc1103 · 2018-03-29T21:19:33Z

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-29T21:20:32Z

Automation for azure-libraries-for-java

Encountered a Subprocess error: (azure-libraries-for-java)

Command: ['/usr/local/bin/autorest', '/tmp/tmpt0m8v569/rest/specification/applicationinsights/resource-manager/readme.md', '--azure-libraries-for-java-folder=/tmp/tmpt0m8v569/sdk', '--fluent', '--java', '--multiapi', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4272/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4272)
   Loading AutoRest extension '@microsoft.azure/autorest.java' (~2.1.32->2.1.49)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
FATAL: System.Exception: Duplicate File Generation: src/main/java/com/microsoft/azure/management/applicationinsights/implementation/FavoritesInner.java
   at AutoRest.Core.CodeGenerator.<Write>d__13.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Core.CodeGenerator.<Write>d__12.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Java.Azure.Fluent.CodeGeneratorJvaf.<Generate>d__6.MoveNext() in C:\Users\autorest-ci\AppData\Local\Temp\2\PUBLISHedxhk\164_20180307T193002\autorest.java\src\azurefluent\CodeGeneratorJvaf.cs:line 57
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Java.Program.<ProcessInternal>d__3.MoveNext() in C:\Users\autorest-ci\AppData\Local\Temp\2\PUBLISHedxhk\164_20180307T193002\autorest.java\src\Program.cs:line 115
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at NewPlugin.<Process>d__15.MoveNext()
FATAL: java/generate - FAILED
FATAL: Error: Plugin java reported failure.
Process() cancelled due to exception : Plugin java reported failure.
  Error: Plugin java reported failure.

AutorestCI · 2018-03-29T21:20:36Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2022

AutorestCI · 2018-03-29T21:20:37Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2691

AutorestCI · 2018-03-29T21:20:41Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#1496

…specs

fearthecowboy · 2018-03-29T22:06:34Z

@dsgouda -- you can wait for the API review board to review this.

ravbhatnagar

APIs for workbook resource looks good. links resource API have some issues.

ravbhatnagar · 2018-04-03T15:33:55Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

This PUT URL should be modeled as /workbooks/{workbookname}/links/{linkName}. Then in the request body you dont need the targetId parameter as it can be inferred from the URL.

ravbhatnagar · 2018-04-03T15:34:29Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

This URL will not work. There is a segment (workbookName) missing between workbooks and links

The other problem with this URL is that it seems like the customer requested to GET resources of type "foo" (links) but what they got was resources of type "bar" (workbooks). This will result in an alert that ARM will open against your service.

we'd talked during the last review about removing the links apis entirely because of things like this, and only letting links be modified via PATCH on the workbook itself, making the links there an array instead of just the single initial value.

Yup, thats what I recall. And that model will work. @ericc1103 - Do you plan to update the spec with the above proposal.

I am currently updating it now.

ericc1103 · 2018-04-03T20:27:48Z

I have removed a link related endpoint per request.

This reverts commit bed6bc3.

This reverts commit c245a85.

ravbhatnagar · 2018-04-04T07:09:41Z

Looks good!

dsgouda

Left a few comments, looks great for the most part.

dsgouda · 2018-04-04T16:37:07Z

...tioninsights/resource-manager/Microsoft.Insights/stable/2015-05-01/examples/WorkbookAdd.json

+                "location": "westus",
+                "name": "deadb33f-8bee-4d3b-a059-9be8dac93960",
+                "properties": {
+                    "name": "Blah Blah Blah",


nit: a meaningful name would be nice :)

We are using a resource name as GUID and display name is under properties.

dsgouda · 2018-04-04T16:38:17Z

...tioninsights/resource-manager/Microsoft.Insights/stable/2015-05-01/examples/WorkbookGet.json

+    "responses": {
+        "200": {
+            "body": {
+                "id": "c0deea5e-3344-40f2-96f8-6f8e1c3b5722",


nit: we recommend not using prod-like strings, please replace this by {subId}/{name}, etc

dsgouda · 2018-04-04T16:39:09Z

...ninsights/resource-manager/Microsoft.Insights/stable/2015-05-01/examples/WorkbookUpdate.json

In previous examples this was "west us", please check for consistency

dsgouda · 2018-04-04T16:44:27Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

Marking this as false can cause breaking changes if new enum values are introduced

dsgouda · 2018-04-04T16:45:48Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

same comment as above for modelAsString

dsgouda · 2018-04-04T16:48:38Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

+      "description": "Azure Resource id or any target workbook resource id.",
+      "x-ms-parameter-location": "method"
+    },    
+    "CanFetchWorkbookContentParameter": {


A better name would be FetchWorkbookContentParameter

dsgouda · 2018-04-04T16:53:06Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

+          "description": "The user-defined name of the workbook."
+        },
+        "serializedData": {
+          "type": "string",


Please check if this is readOnly

dsgouda · 2018-04-04T17:00:52Z

...applicationinsights/resource-manager/Microsoft.Insights/stable/2015-05-01/workbooks_API.json

+    "/subscriptions/{subscriptionId}/providers/microsoft.insights/workbooks": {  
+      "get": {
+        "description": "Get all Workbooks defined within a specified subscriptionId. It will get a list of workbooks or linked workbooks based on $filter clause. If sourceId is specified, it will get a list of linked workbboks. If a targetId is specified, it will get a list of workboks. When a targetId is specified, a resourceName is not required. Even if it is provided, it will be ignored.",
+        "operationId": "Workbooks_ListByResourceGroup",


Consider making this a pageable operation

ericc1103 · 2018-04-05T00:57:59Z

@fearthecowboy We decided to remove a link type to make it simpler.

ericc1103 · 2018-04-05T17:43:29Z

@dsgouda I have applied all your suggestions. Please let me know if there are more things I would need to address.
Thanks,
Eric

dsgouda

LGTM

Adding Workbooks resource type.

a892e9f

azuresdkci assigned dsgouda Mar 29, 2018

azuresdkci requested a review from dsgouda March 29, 2018 21:20

Rename workbookDelete.json to WorkbookDelete.json

aee9914

ericc1103 added 2 commits March 29, 2018 14:52

Fixed undefiend parameter issue with Delete verb.

4ba66aa

Merge branch 'master' of https://github.com/ericc1103/azure-rest-api-…

e6041b2

…specs

ericc1103 added 2 commits March 30, 2018 12:22

Update swagger to add subscriptionId

6672215

Fixed duplicated operation id issue

92ff8e9

ravbhatnagar reviewed Apr 3, 2018

View reviewed changes

ericc1103 added 2 commits April 3, 2018 13:21

Removed links endpoint

c245a85

Removed unused samples

bed6bc3

ericc1103 added 2 commits April 3, 2018 14:56

Revert "Removed unused samples"

d3bac20

This reverts commit bed6bc3.

Revert "Removed links endpoint"

5259a41

This reverts commit c245a85.

ravbhatnagar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Apr 4, 2018

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Apr 4, 2018

dsgouda suggested changes Apr 4, 2018

View reviewed changes

ericc1103 added 2 commits April 4, 2018 17:50

Removed link logic and added get by resourceid

4227f6c

Applied PR comments

328b75c

dsgouda approved these changes Apr 5, 2018

View reviewed changes

dsgouda merged commit 94df861 into Azure:master Apr 5, 2018

Conversation

ericc1103 commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-libraries-for-java

Uh oh!

AutorestCI commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

fearthecowboy commented Mar 29, 2018

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!

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!

ericc1103 commented Apr 3, 2018

Uh oh!

ravbhatnagar commented Apr 4, 2018

Uh oh!

dsgouda 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!

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!

dsgouda Apr 4, 2018 • 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!

ericc1103 commented Apr 5, 2018

Uh oh!

ericc1103 commented Apr 5, 2018

Uh oh!

dsgouda left a comment

Choose a reason for hiding this comment

ericc1103 commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

AutorestCI commented Mar 29, 2018 •

edited

Loading

dsgouda Apr 4, 2018 •

edited

Loading