Adding a new api to fetch the health details of the resource "vault". Apart form this this PR also contains some fixes based on some new rules added in the auto rest linter tool. #1506

mukum · 2017-08-04T17:21:05Z

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.

msftclas · 2017-08-04T17:21:09Z

@mukum,
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

dsgouda · 2017-08-04T21:51:14Z

Sorry to bother, but reopening PR to test CI builds.

msftclas · 2017-08-04T21:51:20Z

@mukum,
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

veronicagg · 2017-08-05T00:58:55Z

@ravbhatnagar please take a look, thanks!

ravbhatnagar

@mukum - This PR contains a lot of changes like new property additions, newly added models, adding APIs etc. We only discussed the new API changes in our meeting and so Many of the other changes are difficult to understand. Once you have addressed the questions/comments in the PR, please schedule a review with the Azure API review board ( @johanste , @NiklasGustafsson ) and myself.

ravbhatnagar · 2017-08-07T21:09:54Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+          "200": {
+            "description": "OK",
+            "schema": {
+              "$ref": "#/definitions/VaultHealthDetails"


Since this is not a tracked resource, you dont need location in the response body

Agree, But since all our proxy resource derive from a common base class hence it is there.

ravbhatnagar · 2017-08-07T21:11:16Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+      "description": "class to define the summary of the health error details.",
+      "type": "object",
+      "properties": {
+        "summaryCode": {


I think this could be an enum with a known set of values.

Since this is the error code only which we have exposed at other classes as well, like health error, we don't feel this to be exposed as an enum hence going ahead with string.

Is this property read only?

Hi Veronica, yes it is a read only property. Our service is bit different as we have the get and put object schema different, hence even if we don't mark it read only the client cannot used the same object received in the get call to Put/Update. Although i agree marking a property read only is a good practice and we have this as one of our backlog item while moving to the next api version.

ok, thanks, yes, I would recommend doing that in your next version if you don't see it being possible now (or causes a breaking change).

ravbhatnagar · 2017-08-07T21:11:22Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+          "description": "The code of the health error.",
+          "type": "string"
+        },
+        "category": {


I think this could be an enum with a known set of values.

Agree changing it to enum.

ravbhatnagar · 2017-08-07T21:11:28Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+          "description": "The category of the health error.",
+          "type": "string"
+        },
+        "severity": {


I think this could be an enum with a known set of values.

Agree changing it to enum.

ravbhatnagar · 2017-08-07T21:19:02Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

    "application/json"
  ],
  "paths": {
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{resourceName}/replicationVaultHealth": {


Is this API also giving the health summary for all the child resources of vault (fabric/ protectedItem etc)? If yes, then does the caller of this API need to have read access on all the underlying resources as well. Also, from the API signature it is unclear. Moreover, in the payload it will be unclear which "replicationItem" does a particular issue correspond to? Am I missing something?

No gaurav, This API will give give the health summary of immediate child resources only. At the vault level we are going with the exception which would give protected items level summary as well.
Yes the caller of the API will need to have the read access on the underlying resources and we will give the same. In the payload you can see each health summary contains the list of affectedResourceCorrelationId to related this health impacts which all resources.

ravbhatnagar · 2017-08-07T21:23:29Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+          "description": "Type of error.",
+          "type": "string"
+        },
+        "errorLevel": {


Could this be an enum

These are pre approved classes/object model and any change now will be a breaking change.

ravbhatnagar · 2017-08-07T21:25:24Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+          "description": "DRA error message.",
+          "type": "string"
+        },
+        "entityId": {


What is this entityId?

This is the correlation id of the health with the impacted resource.

ravbhatnagar · 2017-08-07T21:25:50Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+          "description": "ID of the entity.",
+          "type": "string"
+        },
+        "childErrors": {


Which child error corresponds to which event? how will this be used?

This is a new property getting added. Will need a api-version update.

All errors relate to the same entity. There is a "root cause and impact" kind of relationship between them hence root cause is the parent and impact health errors are the child.

Since all are object are discovered object(not client created) hence this new property added shouldn't require the api-version update.

ravbhatnagar · 2017-08-07T21:28:01Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+        }
+      }
+    },
+    "EventQueryParameter": {


How will this model be used?

This will be used to query the list of events based on the filters provided as part of the EventQueryParameter. This will have value like startdate to enddate, severity of the filter... impacted resourcee name and so...on..

ravbhatnagar · 2017-08-07T21:31:29Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

            "$ref": "#/definitions/HealthError"
          }
        },
+        "failoverHealthErrorDetails": {


So you the health status property has been added to the GETs of fabric etc. Why do you need a separate API to just get the healthStatus? Also, new properties have been added all across this swagger. This would require an api-version change on both service and swagger.

The health summary detail is the summary of the container level resource. The purpose of this is to give a dashboard kind of view at the container level resource like vault/fabric. As stated earlier since we have discovered resources only adding a new property shouldn't be a breaking change.

ravbhatnagar · 2017-08-07T21:40:48Z

@veronicagg - This may need to be reviewed in person with the Azure API review board. The newly added properties and models are not are not self explanatory. The API that is being added is fine and feedback has been provided on that.

veronicagg · 2017-08-07T22:27:59Z

@ravbhatnagar thanks! will wait for API review board sign off.

veronicagg

Please review linter errors at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/261175730#L675
Please review examples errors - invalid format for date-time:

veronicagg · 2017-08-08T18:26:48Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+      "description": "class to define the summary of the health error details.",
+      "type": "object",
+      "properties": {
+        "summaryCode": {


Is this property read only?

veronicagg · 2017-08-11T17:23:05Z

@mukum @ravbhatnagar have you been able to sync on ARM feedback?

ravbhatnagar · 2017-08-11T18:00:04Z

@veronicagg - No, we haven't been able to agree on the right design for this API yet. We are close though and should be able to close today or Monday. @mukum - FYI.

veronicagg · 2017-08-14T17:46:16Z

@ravbhatnagar once you're good with the changes on this PR, please label it and let me know, thanks!

ravbhatnagar · 2017-08-15T10:54:27Z

@mukum - The change for the new API looks good now. However, as discussed earlier it seems that there are new properties in this swagger that were already added on the service side but getting added to swagger now. Please confirm if thats the case. Because in such cases, an api-version update is required.
@veronicagg - SDK can start reviewing.

azuresdkciprbot · 2017-08-15T16:09:55Z

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/recoveryservicessiterecovery/resource-manager/readme.md
Before the PR: Warning(s): 83 Error(s): 22
After the PR: Warning(s): 87 Error(s): 24

AutoRest Linter Guidelines | AutoRest Linter Issues

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

Thanks for your co-operation.

veronicagg · 2017-08-16T01:44:25Z

thanks @ravbhatnagar.
@mukum have you been able to look into the errors from CI that I pointed out before? Feel free to let me know if you have questions.

mukum · 2017-08-16T15:32:24Z

@veronicagg
Hi Veronica,
I went through the errors from CL, all the errors are for camel casing of the property name(for the older set of properties name). We have this as one of the backlog items once we move to the newer api version.
One other is the Job object which has few properties other than Id type and Location at the top level. I talked with my colleague regarding the same and as per him we had got exception for this last time. Since we are not upgrading the version of the API we are not fixing it, will take it in the next iteration when we move to the newer api version.

mukum · 2017-08-16T16:04:12Z

@ravbhatnagar as discussed earlier, since we have the get and put object schema completely different, hence adding a new property in the get object schema doesn't require a version update.

veronicagg

Thanks @mukum .
On camel casing, ok.
For Job model which has additional properties at the top level, as far as I know it's not allowed for ARM, @ravbhatnagar could you confirm you approved this?
Have you considered flattening properties per the warnings at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/264396941#L1286?
Have you evaluated if the boolean properties flagged should be considered for enums? ex. https://travis-ci.org/Azure/azure-rest-api-specs/jobs/264396941#L1934
Do you have any questions on the "TrackedResource...." warnings? have you evaluated them?
thanks!

veronicagg · 2017-08-16T17:01:15Z

...veryservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json

+        ],
+        "summary": "Adds a recovery services provider.",
+        "description": "The operation to add a recovery services provider.",
+        "operationId": "ReplicationRecoveryServicesProviders_Create",


Operation missing example: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/264396941#L950

@avrai @vijaynar Please add the missing examples if any.

@veronicagg For Job model this is the PR"#1209" where i guess it was closed. Since we are not changing the version of the API we are not making that change.

@veronicagg Booleans properties we have evaluated and we don't feel the need to change it to enum.

@veronicagg Gone through the different tracked resources warning. Since there is no version upgrade we don't want to introduce new api's. We will try to accommodate the warning scenarios with the newer version release.

veronicagg · 2017-08-16T21:04:29Z

@mukum there looks to be some model validation errors too, please review: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/264396943#L657

veronicagg · 2017-08-16T21:09:30Z

@ravbhatnagar please add the sign-off label once you approve this PR. I see that there are breaking changes being introduced, so I'd like have confirmation on sign-off from ARM team.

ravbhatnagar · 2017-08-18T06:42:37Z

@veronicagg - from what I can see in the PR, jobs is an existing model. So issues in the model are older issues. As discussed we will get issues with existing APIs/models fixed in a separate cycle. Other property additions have already happened in the service and these are being added to swagger now.
@mukum - please note that the jobs model will need to be fixed since it contains some top level properties which are not allowed as per ARM RPC. These properties will need to be moved inside the properties bag in your next api-version rev.

veronicagg · 2017-08-18T17:27:02Z

Thanks @ravbhatnagar.
@mukum Please add the pending example and open & link the issue we talked about tracking the update of the examples that are failing (due to bug fixes in our tool). With those, I can approve. Thanks!

veronicagg · 2017-08-21T18:57:33Z

@mukum any updates?

salameer · 2017-08-22T22:10:09Z

@mukum please respond to the feedback to avoid closing this PR due to lack of activity

Thanks

Samer

… job fixed.

azuresdkciprbot · 2017-08-23T14:58:58Z

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/recoveryservicessiterecovery/resource-manager/readme.md
Before the PR: Warning(s): 83 Error(s): 22
After the PR: Warning(s): 85 Error(s): 23

AutoRest Linter Guidelines | AutoRest Linter Issues

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

Thanks for your co-operation.

veronicagg · 2017-08-24T01:40:04Z

Thanks @mukum for addressing the issues.
Samples are compliant, though it's good idea to consider making them specific, so that you get more coverage on the properties, you may want to add this to the issue you linked.
Regarding the removed operation, if it's implemented on the service time, it may get reported as not in the swagger by the ARM team, just so that you're aware - FYI @ravbhatnagar

AutorestCI · 2017-08-24T01:43:24Z

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

AutorestCI · 2017-08-24T01:43:31Z

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

AutorestCI · 2017-08-24T01:44:25Z

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

… Apart form this this PR also contains some fixes based on some new rules added in the auto rest linter tool. (Azure#1506) * Adding vault health detail api. * Changes related to vault health dashboard api * Removing the api RecoveryServicesProvider and making the examples for job fixed.

Adding vault health detail api.

eac20f0

msftclas added the cla-not-required label Aug 4, 2017

azuresdkci assigned veronicagg Aug 4, 2017

dsgouda closed this Aug 4, 2017

dsgouda reopened this Aug 4, 2017

msftclas added the cla-not-required label Aug 4, 2017

veronicagg added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 5, 2017

ravbhatnagar suggested changes Aug 7, 2017

View reviewed changes

veronicagg suggested changes Aug 9, 2017

View reviewed changes

Changes related to vault health dashboard api

44ac351

ravbhatnagar added ReadyForSDKReview and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Aug 15, 2017

veronicagg suggested changes Aug 16, 2017

View reviewed changes

veronicagg added the potential-sdk-breaking-change label Aug 16, 2017

ravbhatnagar approved these changes Aug 18, 2017

View reviewed changes

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Aug 18, 2017

mukum mentioned this pull request Aug 23, 2017

Workaround in the jobs model examples needs to be fixed as the corresponding issue in the OAV tool has been fixed. #1578

Closed

Removing the api RecoveryServicesProvider and making the examples for…

4dd1d9b

… job fixed.

veronicagg approved these changes Aug 24, 2017

View reviewed changes

veronicagg merged commit dab840e into Azure:current Aug 24, 2017

Adding a new api to fetch the health details of the resource "vault". Apart form this this PR also contains some fixes based on some new rules added in the auto rest linter tool. #1506

Adding a new api to fetch the health details of the resource "vault". Apart form this this PR also contains some fixes based on some new rules added in the auto rest linter tool. #1506

Uh oh!

Conversation

mukum commented Aug 4, 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 Aug 4, 2017

Uh oh!

dsgouda commented Aug 4, 2017

Uh oh!

msftclas commented Aug 4, 2017

Uh oh!

veronicagg commented Aug 5, 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!

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!

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar commented Aug 7, 2017

Uh oh!

veronicagg commented Aug 7, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Aug 11, 2017

Uh oh!

ravbhatnagar commented Aug 11, 2017

mukum commented Aug 4, 2017 •

edited by azuresdkci

Loading