AzureStack InfrastructureInsights Admin Specification #2812

deathly809 · 2018-04-04T18:50:20Z

The specification has been regenerated.

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-04-04T18:50:47Z

Automation for azure-sdk-for-go

This PR contains more than 3 context, SDK generation is not enabled. Contexts found:

azsadmin/resource-manager/infrastructureinsights/Microsoft.InfrastructureInsights.Admin
azsadmin/resource-manager/InfrastructureInsights/Microsoft.InfrastructureInsights.Admin
azsadmin/resource-manager/InfrastructureInsights
azsadmin/resource-manager/infrastructureinsights

AutorestCI · 2018-04-04T18:50:53Z

Automation for azure-libraries-for-java

This PR contains more than 3 context, SDK generation is not enabled. Contexts found:

azsadmin/resource-manager/InfrastructureInsights/Microsoft.InfrastructureInsights.Admin
azsadmin/resource-manager/InfrastructureInsights
azsadmin/resource-manager/infrastructureinsights
azsadmin/resource-manager/infrastructureinsights/Microsoft.InfrastructureInsights.Admin

AutorestCI · 2018-04-04T18:50:56Z

Automation for azure-sdk-for-python

This PR contains more than 3 context, SDK generation is not enabled. Contexts found:

azsadmin/resource-manager/InfrastructureInsights
azsadmin/resource-manager/infrastructureinsights
azsadmin/resource-manager/infrastructureinsights/Microsoft.InfrastructureInsights.Admin
azsadmin/resource-manager/InfrastructureInsights/Microsoft.InfrastructureInsights.Admin

AutorestCI · 2018-04-04T18:50:59Z

Automation for azure-sdk-for-node

This PR contains more than 3 context, SDK generation is not enabled. Contexts found:

azsadmin/resource-manager/InfrastructureInsights/Microsoft.InfrastructureInsights.Admin
azsadmin/resource-manager/infrastructureinsights/Microsoft.InfrastructureInsights.Admin
azsadmin/resource-manager/InfrastructureInsights
azsadmin/resource-manager/infrastructureinsights

mcardosos · 2018-04-05T20:38:45Z

...nsights/Microsoft.InfrastructureInsights.Admin/stable/2016-05-01/InfrastructureInsights.json

@@ -0,0 +1,336 @@
+{
+    "swagger": "2.0",


[](start = 0, length = 4)

Could 2 space idents be used instead?

Is there a technical reason that 2 spaces is preferred? I prefer 4 spaces.

@deathly809 @mcardosos I would recommend to have a separate PR to fix an indent after this one. Please, leave the indent as it is for this PR so it's easy to review.

I would prefer the 2 spaces indent to be fixed here, actually.

I prefer 4 spaces and there is no requirement on spacing.

Personally, in general I prefer 4 spaces too. But consistency on the repo is important, and it makes reviewing a lot easier / less noisy / faster.

So far my PRs have been 4 spaces. No one has been able to point to a requirement and I have a hard time reading 2 spaces.

mcardosos · 2018-04-09T18:31:15Z

specification/azsadmin/resource-manager/InfrastructureInsights/readme.md

@@ -43,17 +43,6 @@ input-file:
 ---
 # Code Generation

-
-## Swagger to SDK
-


Why is this being deleted?

Not sure, I will revert it.

deathly809 · 2018-04-10T16:05:54Z

@mcardosos This is not a stable API, not sure why it is marked as such. Should I change the folder to preview?

mcardosos · 2018-04-12T16:24:13Z

If it is an API version where there will be changes, yes, please move to preview

mcardosos · 2018-04-12T16:32:26Z

specification/azsadmin/resource-manager/InfrastructureInsights/readme.md

-```
-
-## C# 
+## C#



There are many code generation settings on this file that I am not sure why they are gone :( They are very useful for us SDK teams

We currently will only support C#. We have no plans for anything else at this time.

Why are there no plans for other SDKs yet?

Please message my manager.

We already talked :D

mcardosos · 2018-04-12T16:33:00Z

...r/infrastructureinsights/Microsoft.InfrastructureInsights.Admin/stable/2016-05-01/Alert.json

@@ -42,6 +45,9 @@
                        "schema": {
                            "$ref": "#/definitions/AlertList"
                        }
+                    },
+                    "404": {
+                        "description": "NOT FOUND"
                    }


Unsuccessful response codes should not be included

The team prefers not to throw an error on 404 and instead return null.

I see. I highly recommend not setting it like this: other swaggers / SDKs do not follow this pattern, and it can be confusing for users when a SDK behaves in a different way.

In reply to: 181145822 [](ancestors = 181145822)

I will bring this up with the PM who is leading the admin spec/SDK/PS push.

What did you conclude there?

mcardosos · 2018-04-12T16:33:32Z

...r/infrastructureinsights/Microsoft.InfrastructureInsights.Admin/stable/2016-05-01/Alert.json

@@ -88,6 +97,9 @@
                        "schema": {
                            "$ref": "#/definitions/Alert"
                        }
+                    },
+                    "404": {
+                        "description": "NOT FOUND"
                    }


Same here, unsuccessful responses should not be included

The team prefers not to throw an error on 404 and instead return null.

mcardosos · 2018-04-12T16:34:21Z

...r/infrastructureinsights/Microsoft.InfrastructureInsights.Admin/stable/2016-05-01/Alert.json

+                        "$ref": "InfrastructureInsights.json#/parameters/ResourceGroupParameter"
+                    },
+                    {
+                        "$ref": "InfrastructureInsights.json#/parameters/RegionParameter"


RegionParameter [](start = 73, length = 15)

Is this paramater required? it does not look like it

yes it is required, it is the {location} parameter in the URI.

okok, nevermind

In reply to: 181146243 [](ancestors = 181146243)

deathly809 · 2018-04-12T16:41:31Z

@mcardosos Moved to preview, some of your responses have been removed but I responded.

mcardosos · 2018-04-12T16:47:31Z

...sights/Microsoft.InfrastructureInsights.Admin/preview/2016-05-01/InfrastructureInsights.json

+            "type": "string",
+            "x-ms-parameter-location": "method"
+        },
+        "RegionParameter": {


RegionParameter [](start = 9, length = 15)

A more accurate name could be LocationParameter #Resolved

Done. #Resolved

deathly809 · 2018-04-13T18:14:29Z

@mcardosos Is there anything else I should do?

mcardosos · 2018-04-13T21:03:00Z

This looks good. Last question is on the response status codes.

azuresdkciprbot · 2018-04-13T21:39:15Z

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/azsadmin/resource-manager/infrastructureinsights/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

Thanks for your co-operation.

mcardosos · 2018-04-14T00:43:08Z

...sights/Microsoft.InfrastructureInsights.Admin/preview/2016-05-01/InfrastructureInsights.json

+            }
+        }
+    },
+    "definitions": {


definitions [](start = 5, length = 11)

There are a bunch of definitions that are not being used on this file, are they being used at all?

I see that every definition in this file is referenced somewhere else. I know for a fact the other definitions in the other files are referenced. Looks like everything is referenced somewhere.

deathly809 · 2018-04-18T17:25:22Z

@mcardosos Everything look good?

mcardosos · 2018-04-18T21:30:20Z

...sights/Microsoft.InfrastructureInsights.Admin/preview/2016-05-01/InfrastructureInsights.json

+                }
+            }
+        },
+        "Resource": {


Resource [](start = 9, length = 8)

This definition should follow this patterns instead

"Resource": { "description": "The core properties of ARM resources", "properties": { "id": { "readOnly": true, "type": "string", "description": "Fully qualified resource Id for the resource" }, "name": { "readOnly": true, "type": "string", "description": "The name of the resource" }, "type": { "readOnly": true, "type": "string", "description": "The type of the resource." } }, "x-ms-azure-resource": true }, "TrackedResource": { "description": "The resource model definition for a ARM tracked top level resource", "properties": { "tags": { "type": "object", "additionalProperties": { "type": "string" }, "x-ms-mutability": [ "read", "create", "update" ], "description": "Resource tags." }, "location": { "type": "string", "x-ms-mutability": [ "read", "create" ], "description": "The Azure Region where the resource lives" } }, "allOf": [ { "$ref": "#/definitions/Resource" } ] }, "ProxyResource": { "description": "The resource model definition for a ARM proxy resource. It will have everything other than required location and tags", "allOf": [ { "$ref": "#/definitions/Resource" } ] },

Could you explain why? Who would reference TrackedResource and who would reference ProxyResource?

Tracked resource is for those resources that have location and tags (for example, a VM).
Proxy resource is for those other resources that happen to be derived of other resources, and therefore, they don't have a location/tags (for example, a VM extension).

@mcardosos I still don't understand why you want me to add this. Looking at fiddler all my resources have a location and tags on the returned objects. Are you saying I should make it so nested resources cannot access those fields anymore?

If they have location and tags, they should have an allof of trackedresource

azuresdkciprbot · 2018-04-19T18:35:04Z

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/azsadmin/resource-manager/infrastructureinsights/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

Thanks for your co-operation.

deathly809 · 2018-04-20T02:40:15Z

@mcardosos I have made the change you have suggested, please let me know if there is anything else.

mcardosos

Awesome! Are this changes already available in the API?

deathly809 · 2018-04-20T21:10:10Z

@mcardosos What do you mean? What we have in the spec is what is supported by the FRP currently. At least that is what the PM and some of the developers have told me.

cleanup stostools (#12699)

deathly809 added 2 commits April 4, 2018 11:46

Removed directory

2b4807f

Added updated files

8ae9c8c

azuresdkci assigned mcardosos Apr 4, 2018

azuresdkci requested a review from mcardosos April 4, 2018 18:51

mcardosos reviewed Apr 5, 2018

View reviewed changes

mcardosos reviewed Apr 9, 2018

View reviewed changes

Reverted part of readme

58dc733

mcardosos reviewed Apr 12, 2018

View reviewed changes

Moved to preview

c90b75a

mcardosos reviewed Apr 12, 2018

View reviewed changes

deathly809 and others added 2 commits April 12, 2018 14:42

Renamed RegionParameter to LocationParameter

8bff15f

Revert fabric to remove merge issues

4d703f9

deathly809 added 2 commits April 13, 2018 14:33

Removed 404s

2ee81c3

Fixed readme.md

fc00ced

mcardosos reviewed Apr 14, 2018

View reviewed changes

mcardosos reviewed Apr 18, 2018

View reviewed changes

PR Requests

54cdbd4

mcardosos approved these changes Apr 20, 2018

View reviewed changes

mcardosos merged commit bc92ce2 into Azure:master Apr 20, 2018

deathly809 mentioned this pull request Apr 20, 2018

[NOT AZURE] Updated Azure Stack Infrastructure Insights Admin SDK Azure/azure-sdk-for-net#4252

Merged

9 tasks

mccleanp pushed a commit that referenced this pull request Mar 23, 2022

Merge pull request #2812 from zhenglaizhang/clean-stostools

2ab14d2

cleanup stostools (#12699)

-              ```
-              ## C#
+              ## C#

AzureStack InfrastructureInsights Admin Specification #2812

AzureStack InfrastructureInsights Admin Specification #2812

Conversation

deathly809 commented Apr 4, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

AutorestCI commented Apr 4, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Apr 4, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Apr 4, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Apr 4, 2018 • edited Loading

Automation for azure-sdk-for-node

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

deathly809 commented Apr 10, 2018

mcardosos commented Apr 12, 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

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

deathly809 commented Apr 12, 2018

mcardosos Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

deathly809 Apr 12, 2018 • edited by mcardosos Loading

Choose a reason for hiding this comment

deathly809 commented Apr 13, 2018

mcardosos commented Apr 13, 2018

azuresdkciprbot commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deathly809 commented Apr 18, 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

azuresdkciprbot commented Apr 19, 2018

deathly809 commented Apr 20, 2018

mcardosos left a comment

Choose a reason for hiding this comment

deathly809 commented Apr 20, 2018

deathly809 commented Apr 4, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Apr 4, 2018 •

edited

Loading

AutorestCI commented Apr 4, 2018 •

edited

Loading

AutorestCI commented Apr 4, 2018 •

edited

Loading

AutorestCI commented Apr 4, 2018 •

edited

Loading

mcardosos Apr 12, 2018 •

edited

Loading

deathly809 Apr 12, 2018 •

edited by mcardosos

Loading