Swagger for diagnostic settings and association resource types #3295

sankalpsoni · 2018-06-23T17:42:52Z

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-06-23T17:44:10Z

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

AutorestCI · 2018-06-23T17:44:51Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-06-23T17:45:56Z

Automation for azure-sdk-for-java

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

AutorestCI · 2018-06-23T17:46:48Z

Automation for azure-sdk-for-ruby

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

AutorestCI · 2018-06-23T17:48:15Z

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

annatisch · 2018-06-25T22:39:30Z

Thanks @sankalpsoni!
There's still a number of model validation errors:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/396031137

And linter errors - both missing examples and ARM violations:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/396031137

Are you able to take a look?

sankalpsoni · 2018-06-26T04:28:11Z

Are you talking errors which are reported in "Allowed Failures"? I am presuming that they should not block the sign off.

What is the next step in the review?

@annatisch Can you confirm if the "Allowed Failures" are must fix?

ravbhatnagar · 2018-06-27T07:41:37Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+                            "$ref": "#/definitions/GuestDiagnosticSettingsResource"
+                        }
+                    },
+                    "404": {


Shouldnt this be handled by the "default" error response? I dont think you need to add it explicitly

ravbhatnagar · 2018-06-27T07:43:51Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+        }
+      }
+    },
+    "paths": {


Since guestDiagnosticSettings is a top level tracked resource, supporting PATCH is required. Ideally it should support updating all patchable properties. But at the least, it should support updating tags.

ravbhatnagar · 2018-06-27T07:44:32Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+    },
+    "paths": {
+        "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/microsoft.insights/guestDiagnosticSettings/{diagnosticSettingsName}": {
+            "put": {


Is this a long running operation?

Nope. Should be completed in ms.

johnkemnetz · 2018-06-27T22:23:52Z

...soft.insights/preview/2018-06-01-preview/examples/createOrUpdateGuestDiagnosticSettings.json

+            "location": "Global",
+            "tags": {},
+            "properties": {
+                "osType":  "windows",


Can we capitalize the first letter of the OSes -- Windows/Linux?

johnkemnetz · 2018-06-27T22:25:22Z

...soft.insights/preview/2018-06-01-preview/examples/createOrUpdateGuestDiagnosticSettings.json

+                        },
+                        "sinks": [
+                            {
+                                "kind": "LogAnalytics"


Don't you need the details for the kind? The workspace resource ID at least?

Not for this private preview. Remember - multi-homing is not supported. Workspace resource id would be passed by extension

johnkemnetz · 2018-06-27T22:27:34Z

...ts/preview/2018-06-01-preview/examples/createOrUpdateGuestDiagnosticSettingsAssociation.json

@@ -0,0 +1,42 @@
+{
+    "parameters": {
+        "resourceUri":"subscriptions/4abcdefgh-ijkl-mnop-qrstuvwxyz/resourceGroups/rhctestenv/providers/Microsoft.ClassicCompute/virtualMachines/rhctestenvV1PI",


Shouldn't our default example be an ARM VM?

johnkemnetz · 2018-06-27T22:28:11Z

...ts/preview/2018-06-01-preview/examples/createOrUpdateGuestDiagnosticSettingsAssociation.json

+{
+    "parameters": {
+        "resourceUri":"subscriptions/4abcdefgh-ijkl-mnop-qrstuvwxyz/resourceGroups/rhctestenv/providers/Microsoft.ClassicCompute/virtualMachines/rhctestenvV1PI",
+        "diagnosticSettingsAssociationName": "SampleAssociationName",


Maybe "centralITDefault" would be a better name to use as a sample.

johnkemnetz · 2018-06-27T22:29:08Z

...soft.insights/preview/2018-06-01-preview/examples/createOrUpdateGuestDiagnosticSettings.json

+    "parameters": {
+        "subscriptionId": "187f412d-1758-44d9-b052-169e2564721d",
+        "resourceGroupName": "Default-ResourceGroup",
+        "diagnosticSettingsName": "SampleDiagSetting",


Suggestion: instead of SampleDiagSetting, use "productionMachineSetting" , which is more like a real customer scenario (one setting for all production machines)

johnkemnetz · 2018-06-28T18:46:47Z

@sankalpsoni looks good to me.

annatisch · 2018-06-28T20:57:08Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+        "/subscriptions/{subscriptionId}/providers/microsoft.insights/guestDiagnosticSettings": {
+            "get": {
+                "description": "Get a list of all guest diagnostic settings in a subscription.",
+                "operationId": "guestDiagnosticsSettings_ListBySubscriptionId",                


This can be renamed to: "guestDiagnosticsSettings_List" as the subscription ID is implicit.

annatisch · 2018-06-28T21:02:02Z

...er/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettingsAssociation_API.json

+        "/subscriptions/{subscriptionId}/providers/microsoft.insights/guestDiagnosticSettingsAssociations": {
+            "get": {
+                "description": "Get a list of all guest diagnostic settings association in a subscription.",
+                "operationId": "guestDiagnosticsSettingsAssociation_ListBySubscriptionId",                


This can be renamed to: "guestDiagnosticsSettingsAssociation_List" as the subscription ID is implicit.

annatisch · 2018-06-28T21:08:47Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+            },
+            "description": "Virtual machine guest diagnostics settings resource."
+        },
+        "DiagnosticSettings": {


This model name is incompatible with an already existing model name:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/monitor/resource-manager/microsoft.insights/preview/2017-05-01-preview/diagnosticsSettings_API.json#L277

Do you intend to generate both specs into the same SDK? If so, this will need to be renamed (e.g. "GuestDiagnosticSettings"). Otherwise if this spec is intended to replace diagnosticSettings_API then it should be okay.

annatisch · 2018-06-28T21:20:08Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+                        "$ref": "#/parameters/SubscriptionIdParameter"
+                    },
+                    {
+                        "name": "diagnosticSettingsPatch",


Could this please be renamed to just "name: "parameters"
We try to avoid API terms like "patch" in the clients :)

annatisch · 2018-06-29T18:02:39Z

...ource-manager/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettings_API.json

+        "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/microsoft.insights/guestDiagnosticSettings": {
+            "get": {
+                "description": "Get a list of all guest diagnostic settings in a resource group.",
+                "operationId": "guestDiagnosticsSettings_ListByRG",                


Please revert this to "guestDiagnosticsSettings_ListByResourceGroup"
This one didn't need to be changed.

annatisch · 2018-06-29T18:03:19Z

...er/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettingsAssociation_API.json

+        "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/microsoft.insights/guestDiagnosticSettingsAssociations": {
+            "get": {
+                "description": "Get a list of all guest diagnostic settings association in a resource group.",
+                "operationId": "guestDiagnosticsSettingsAssociation_ListByRG",                


Please revert this to "guestDiagnosticsSettingsAssociation_ListByResourceGroup"
This one didn't need to be changed.

annatisch · 2018-06-29T18:04:35Z

...er/microsoft.insights/preview/2018-06-01-preview/guestDiagnosticSettingsAssociation_API.json

+                        "$ref": "#/parameters/DiagnosticSettingsAssociationNameParameter"
+                    },
+                    {
+                        "name": "diagnosticSettingsAssociation",


Is there are reason you revert the name from "parameters" here? It's not an issue, but I'm curious - as it would then be consistent with the other patch operation.

I do not remember. I have changed it back,

Now I know - it caused Model validation error. Done correctly now :)

annatisch · 2018-07-06T14:03:57Z

ping @sankalpsoni

annatisch · 2018-07-09T15:10:11Z

This PR is missing ARM sign-off. Since many days have passed, please let me know if you prefer to:

follow up with ravbhatnagar and wait, or
I can merge the PR without ARM’s team sign-off. Please note that even after merge, you should follow up with ARM team regarding ARM issues and fixes may impact the specs and/or generated SDKs. Thanks!

ping @ravbhatnagar

ravbhatnagar · 2018-07-09T18:53:37Z

Reviewed over PR and skype. Looks good.

sankalpsoni added 2 commits June 23, 2018 18:56

Association resource and example

8ba71ec

Guest diagnostic setting

f229b35

azuresdkci assigned annatisch Jun 23, 2018

azuresdkci requested a review from annatisch June 23, 2018 17:43

AutorestCI mentioned this pull request Jun 23, 2018

[AutoPR monitor/resource-manager] Swagger for diagnostic settings and association resource types Azure/azure-sdk-for-java#2137

Merged

sankalpsoni added 5 commits June 23, 2018 23:52

Fixing Travis CI Failure

a0ee666

Fixing travis build

90b000d

Travis failures

0734d7c

Travis failures

6524601

Travis failures

b1f3c4f

annatisch added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jun 26, 2018

sankalpsoni added 2 commits June 26, 2018 13:54

Travis failures

af6c116

Travis failures

d09b424

ravbhatnagar reviewed Jun 27, 2018

View reviewed changes

johnkemnetz reviewed Jun 27, 2018

View reviewed changes

sankalpsoni added 3 commits June 28, 2018 13:24

Review comments

22eff74

Trying out to fix linter error

4ca6bc6

Trying out to fix linter error

411a8a5

annatisch requested changes Jun 28, 2018

View reviewed changes

sankalpsoni added 3 commits June 29, 2018 14:46

PR Comments

794d132

Adding more examples

ce1133b

Build failures

cf9f582

sankalpsoni added 6 commits June 29, 2018 16:19

Build failures

8d3e723

Documentation fixes

84994fb

Documentation fixes

bed5a2b

Fixes

4e3741e

Fixes

4ba1266

Fixes

cc48583

annatisch requested changes Jun 29, 2018

View reviewed changes

sankalpsoni added 2 commits July 9, 2018 15:25

Comments resolution

3dd7e67

Comments resolution

352fcc0

AutorestCI mentioned this pull request Jul 9, 2018

[AutoPR monitor/resource-manager] Swagger for diagnostic settings and association resource types Azure/azure-sdk-for-ruby#1452

Merged

Model CI Failure

a33822c

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 Jul 9, 2018

annatisch approved these changes Jul 9, 2018

View reviewed changes

annatisch merged commit 49901c0 into Azure:master Jul 9, 2018

Swagger for diagnostic settings and association resource types #3295

Swagger for diagnostic settings and association resource types #3295

Conversation

sankalpsoni commented Jun 23, 2018 • edited Loading

PR information

Quality of Swagger

AutorestCI commented Jun 23, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Jun 23, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Jun 23, 2018 • edited Loading

Automation for azure-sdk-for-java

AutorestCI commented Jun 23, 2018 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Jun 23, 2018 • edited Loading

Automation for azure-sdk-for-go

annatisch commented Jun 25, 2018

sankalpsoni commented Jun 26, 2018 • 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

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

johnkemnetz commented Jun 28, 2018

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

annatisch Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annatisch commented Jul 6, 2018

annatisch commented Jul 9, 2018

ravbhatnagar commented Jul 9, 2018

sankalpsoni commented Jun 23, 2018 •

edited

Loading

AutorestCI commented Jun 23, 2018 •

edited

Loading

AutorestCI commented Jun 23, 2018 •

edited

Loading

AutorestCI commented Jun 23, 2018 •

edited

Loading

AutorestCI commented Jun 23, 2018 •

edited

Loading

AutorestCI commented Jun 23, 2018 •

edited

Loading

sankalpsoni commented Jun 26, 2018 •

edited

Loading

annatisch Jun 29, 2018 •

edited

Loading