Adding few more function to ADHybridHealth swagger spec #3189

gamitra · 2018-06-07T00:07:37Z

This Pull Request contains the Swagger specification for Microsoft.ADHybridHealthService RP.
This RP is for Azure Active Directory Connect Health service, which has been GA-ed for last 2 years.
Azure Active Directory (Azure AD) Connect Health helps you monitor and gain insights into your on-premises identity infrastructure and the synchronization services. It enables you to maintain a reliable connection to Office 365 and Microsoft Online Services by providing monitoring capabilities for your key identity components such as Active Directory Federation Services (AD FS) servers, Azure AD Connect servers (also known as Sync Engine), Active Directory domain controllers, etc. It also makes the key data points about these components easily accessible so that you can get usage and other important insights to make informed decisions.
The information is presented in the Azure AD Connect Health portal. In the Azure AD Connect Health portal, you can view alerts, performance monitoring, usage analytics, and other information. Azure AD Connect Health enables the single lens of health for your key identity components in one place.
More details about this RP can be found at:
https://docs.microsoft.com/en-us/azure/active-directory/connect-health/active-directory-aadconnect-health

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-07T00:08:14Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-06-07T00:08:48Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-06-07T00:09:08Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-06-07T00:10:02Z

Automation for azure-libraries-for-java

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

Command: ['/usr/local/bin/autorest', '/tmp/tmp5k4xymgi/rest/specification/adhybridhealthservice/resource-manager/readme.md', '--azure-libraries-for-java-folder=/tmp/tmp5k4xymgi/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/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4278)
   Loading AutoRest extension '@microsoft.azure/autorest.java' (~2.1.32->2.1.70)
   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/adhybridhealthservice/implementation/AddsServicesInner.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 /opt/vsts/work/1/s/src/azurefluent/CodeGeneratorJvaf.cs:line 58
--- 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 /opt/vsts/work/1/s/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-06-07T00:11:02Z

Automation for azure-sdk-for-go

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

gamitra · 2018-06-07T17:26:25Z

@dsgouda, do you need any more information from me to start with this PR review?

dsgouda · 2018-06-07T17:53:30Z

@gamitra will review soon

dsgouda

Please address warnings in the CI linter build

dsgouda · 2018-06-07T18:35:47Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+        ],
+        "responses": {
+          "200": {
+            "description": " ",


Please provide a meaningful description.

Added description text for all operations.

dsgouda · 2018-06-07T18:38:39Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+            "$ref": "./examples/UserPreference.json"
+          }
+        },
+        "operationId": "addsServices_getUserPreference",


A recommended operation id would be UserPreferences_Get or AddsServicesUserPreference_Get
Also, just out of curiosity, what does Adds stand for

@dsgouda, ok I will use AddsServicesUserPreference_get, since it aligns with the naming convention I have used for the previous operations also.
Adds is Active Directory Domain Service,

This is done.

dsgouda · 2018-06-07T18:39:32Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+          }
+        ],
+        "responses": {
+          "200": {


Please provide meaningful description for all operations
Please confirm whether this is a long running operation

@dsgouda, did not get your point. What is a meaningful description for an operation which does not display any result? Do you have an example for me to follow?

@dsgouda , also will be good if you can help me with an example for long running operation.

Here is an example.
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/network/resource-manager/Microsoft.Network/stable/2018-05-01/applicationGateway.json#L72

So none of the operations listed in my swagger spec are long running.
Do I still need to add that flag? All the delete, add,get are targetted operations with a max latency of 6s.

This one I left out, waiting for the clarification for my previous qs.

Given the latency you do not need to set the flag.

dsgouda · 2018-06-07T18:41:00Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+            "type": "string"
+          },
+          {
+            "name": "withDetails",


A better name for this parameter would be showDetails or fetchDetails

This APIs are already in production and being used by UI, so I cant rename any parameter.

Left this out also, because this is a production code.

dsgouda · 2018-06-07T18:41:54Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+          }
+        ],
+        "responses": {
+          "200": {


Please add x-ms-pageable extension

I revisited this operation,. its not a list. It's a get function. I will change the name of the operation to avoid confusion.

dsgouda · 2018-06-07T18:42:39Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+        "tags": [ "Adds" ],
+        "description": "Gets Replication status for a given Active Directory Domain Service, that is onboarded to Azure Active Directory Connect Health.",
+        "x-ms-examples": {
+          "addsServices_getReplicationStatus": {


Please rename operationid to ReplicationStatus_Get or AddsServicesReplicationStatus_Get

dsgouda · 2018-06-07T18:44:08Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+            "description": "The next partition key to query for.",
+            "required": true,
+            "type": "string",
+            "enum": [ " " ],


Please create a model definition and refer its schema for enum parameters

The reason why it was added like this is:
The nextRowKey and nextPartitionKey were introduced to allow optimization from UI calls to our APIs. But that was never carried forward. I checked the code again and I see this is marked as mandate parameter but passed as string.Empty.
How to handle this scenario in swagger spec?

So the previous reviewer suggested using this enum pattern.

May I ask one question here, do we need to revisit the old changes? I have already got it reviewed and checked them in 1 month back. Reiterating through that will cost both of us time, I have done everything the previous reviewer asked me to do, and now I am again getting suggestion to re-do the same.
#2989
This is the previous pull request for your reference.

Left this out, as mentioned above.

The way the enum is modeled here is not the issue. We discourage usage of inline definitions of schemas (which is enum here). Please follow an example here and move the schema definition into definitions section

Got it now. Will send another iteration with the change.

@dsgouda , I made the changes for the enum definition as suggested by you. But after that build is failing:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/389568900

Can you please help me understand what am I doing wrong here? How can I correct that? Thanks for helping.

dsgouda · 2018-06-07T18:46:44Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+            "$ref": "./examples/Service_MetricSets.json"
+          }
+        },
+        "operationId": "service_listMetrics",


Similar to all other operationids, please rename this to ServiceMetrics_List and Metrics_List

dsgouda · 2018-06-07T18:47:52Z

...esource-manager/Microsoft.ADHybridHealthService/stable/2014-01-01/ADHybridHealthService.json

+    "ReplicationDetailsList": {
+      "description": "The list of replication details.",
+      "type": "object",
+      "properties": {


Please mark properties readOnly wherever applicable.

Do you mean all input properties should be marked readonly?
In our code we dont explicitly have that enforcement.

Need some help understanding this comment. I left it out of the current iteration. Once I get more clarification, I will add this.

If the model properties are to have public getters, readOnly flag is not necessary. If not, it must be set for each of the properties of the model. This ensures better quality SDK

Thanks for explaining. All the model properties for our service are public getters.

@gamitra I think I misspoke here. My bad.
When the property setters of a model are to be public, the readOnly flag is not necessary. If you wish to ensure that the setters are non public, please set this flag.

Went back and took a look. The setters are all public, so leaving this one.

dsgouda · 2018-06-07T22:54:06Z

@gamitra Please address linter errors/warnings here

gamitra · 2018-06-08T00:51:29Z

@dsgouda, working on the linter error. The build looks flaky, most of the builds are failing with connection timeout error.
I will update you once I have a good build ready for your review.

dsgouda · 2018-06-08T16:26:13Z

@gamitra the build is failing due to syntax errors reported here
Please take a look

gamitra · 2018-06-08T19:36:48Z

@dsgouda, this started failing after I tried to move the enum to definition section as suggested by you.
However, I am unable to figure out why is this failure, been trying to resolve it from yesterday. Do you see any obvious issue?
It would be really helpful, if you can point me to the issue.

dsgouda · 2018-06-08T20:50:00Z

@gamitra looked into the CI failures and I think I know the issue. Looks like we can't have schema definitions for query parameters.
tldr;
Please revert to the earlier inline definition for the enum and that should fix the CI issues.
Lastly, could you explain why you need an empty enum here.
@fearthecowboy FYI

gamitra · 2018-06-08T21:24:06Z

@dsgouda, sure I will revert it back to the previous change. The reason why we use emty enum here is:
The nextRowKey and nextPartitionKey were introduced to allow optimization from UI calls to our APIs. But that was never carried forward.However, in code this is marked as mandate parameter but passed as string.Empty.
That is the reason we had to craft the enum as empty.

dsgouda · 2018-06-08T23:15:06Z

@gamitra please check the Json for correctness

dsgouda

LGTM subject to CIs passing

gamitra · 2018-06-08T23:32:28Z

@dsgouda , Build passed. I see you have signed off also. Thank You for that.
How do I merge the pull request? The "Merge pull request" button for me is showing disabled..
Last review, the reviewer merged it for me.
Can you please help me merge this PR?

dsgouda · 2018-06-08T23:33:20Z

Done.

gamitra · 2018-06-08T23:34:01Z

Thank You so much for your help. Really appreciate it!

gamitra added 2 commits June 6, 2018 17:04

Adding few more functions to ADHybridHealth Swagger spec

453e314

Adding few more Example files

3ce5564

azuresdkci assigned dsgouda Jun 7, 2018

azuresdkci requested a review from dsgouda June 7, 2018 00:08

AutorestCI mentioned this pull request Jun 7, 2018

[AutoPR adhybridhealthservice/resource-manager] Adding few more function to ADHybridHealth swagger spec Azure/azure-sdk-for-python#2696

Merged

AutorestCI mentioned this pull request Jun 7, 2018

[AutoPR adhybridhealthservice/resource-manager] Adding few more function to ADHybridHealth swagger spec Azure/azure-sdk-for-go#1984

Merged

gamitra added 5 commits June 6, 2018 17:24

Update ADHybridHealthService.json

96812db

Update ADHybridHealthService.json

811c961

Update ADHybridHealthService.json

9b47651

Update AddUserPreference.json

d24ed7b

Update AddUserPreference.json

17f6d32

dsgouda suggested changes Jun 7, 2018

View reviewed changes

Update ADHybridHealthService.json

81b67e5

Update ADHybridHealthService.json

a8de2d9

gamitra added 2 commits June 7, 2018 18:21

Update ADHybridHealthService.json

7d11f51

Update ADHybridHealthService.json

9d1ccb4

Update ADHybridHealthService.json

2a54361

Update ADHybridHealthService.json

3af8f27

Update ADHybridHealthService.json

f10a244

dsgouda approved these changes Jun 8, 2018

View reviewed changes

dsgouda merged commit fc63801 into Azure:master Jun 8, 2018

Adding few more function to ADHybridHealth swagger spec #3189

Adding few more function to ADHybridHealth swagger spec #3189

Uh oh!

Conversation

gamitra commented Jun 7, 2018 • 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!

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

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

AutorestCI commented Jun 7, 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 Jun 7, 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 Jun 7, 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 Jun 7, 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!

gamitra commented Jun 7, 2018

Uh oh!

dsgouda commented Jun 7, 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!

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!

gamitra Jun 7, 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!

gamitra commented Jun 7, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

gamitra Jun 7, 2018 •

edited

Loading

dsgouda Jun 7, 2018 •

edited

Loading