Updating Operations API to support Python's AutoRest #1453

sonathan · 2017-07-21T05:37:56Z

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-07-21T05:38:00Z

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

Please review model validation errors in added example at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/255998310#L661.
I understand you're not making changes in the rest of the spec, so just making sure you know about linter error/warnings being reported: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/255998308#L674.
Thanks!

veronicagg · 2017-07-24T17:45:53Z

specification/recoveryservices/resource-manager/readme.md

+``` yaml
+directive:
+  - suppress: DefinitionsPropertiesNamesCamelCase
+    reason: Autorest invalidates two letter acronyms as well and changes in data contracts require service wide changes and require more time


what do you mean by Autorest invalidates 2 letter acronyms as well?
Have you requested approval for this suppression already? Please refer to: https://github.com/Azure/adx-documentation-pr/wiki/Swagger-Validation-Errors-Suppression

Yes, we have requested approval for this supprestion already and also for the XmsExamplesProvidedValidation, as said by @ravbhatnagar that "As Samer said, we cant give an exception for the examples violation. This will not block the PR to be merged but it will be shown as a violation till it is fixed." we had a mail thread with [email protected], [email protected] with subject: Violations Suppression requested for PR 1139

@sonathan thanks for pointing this out. Regarding the specific violation DefinitionsPropertiesNamesCamelCase the justification in the email and the above doesn't seem to match the reason why the error is being reported. I see properties named as as "Provider" for "ClientDiscoveryDisplay", which violates the rule. I understand this is a breaking change and you may not want to take it right now, would this be a permanent exception? @ravbhatnagar @salameer

@sonathan - We cant add suppression for the camel casing violation. Till this is fixed, it will continue to show as violation.

veronicagg · 2017-07-24T17:50:13Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

      }
    },
-    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/operations": {
+    "/providers/Microsoft.RecoveryServices/operations": {


@ravbhatnagar FYI - fix that's a potential breaking change.

For this you can refer: #1267 CLI is not yet released and it will be including these changes.

@veronicagg - This fix is fine since otherwise the API had no meaning and clients couldnt anyway meaningfully use it. And as @sonathan pointed out, the CLI is not yet released so from that perspective it should be ok as well.

veronicagg · 2017-07-24T17:52:35Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

        }
      }
    },
-    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/operations": {


subscriptionID and resourceGroupName are in the path parameters included below (line 65), please remove them if not applicable (https://travis-ci.org/Azure/azure-rest-api-specs/jobs/255998309#L685)

Thankyou. Removing SubsId and ResourceGroupName from parameter from subsequent commit.

thanks, will take a look in your next commit.

veronicagg · 2017-07-26T21:22:25Z

specification/recoveryservices/resource-manager/readme.md

+``` yaml
+directive:
+  - suppress: DefinitionsPropertiesNamesCamelCase
+    reason: Autorest invalidates two letter acronyms as well and changes in data contracts require service wide changes and require more time


@sonathan thanks for pointing this out. Regarding the specific violation DefinitionsPropertiesNamesCamelCase the justification in the email and the above doesn't seem to match the reason why the error is being reported. I see properties named as as "Provider" for "ClientDiscoveryDisplay", which violates the rule. I understand this is a breaking change and you may not want to take it right now, would this be a permanent exception? @ravbhatnagar @salameer

ravbhatnagar · 2017-07-28T22:34:36Z

@veronicagg - As stated above, lets not add suppression for the camel case fix. It will remain a violation as the service team is expected to fix it.

veronicagg · 2017-07-28T22:41:54Z

thanks @ravbhatnagar .
@sonathan please remove suppression that was added to the readme file, then I can approve this PR. thanks!

…ns.json with correct API-Version

veronicagg · 2017-07-31T17:37:14Z

...services/resource-manager/Microsoft.RecoveryServices/2016-06-01/examples/ListOperations.json

+                                    {
+                                        "name": "AzureBackupReport",
+                                        "displayName": "AzureBackup Reporting Data",
+                                        "blobDuration": "PT1H"


There is a remaining issue with the example model validation https://travis-ci.org/Azure/azure-rest-api-specs/jobs/259348817#L657
The value for blobDuration doesn't match the date-time format per https://xml2rfc.tools.ietf.org/public/rfc/html/rfc3339.html#anchor14
Please update these.

Updated. Thanks Veronica!

veronicagg · 2017-08-02T17:51:52Z

@sonathan just let me know if you have questions about the issue pointed out above.

sonathan · 2017-08-04T06:12:37Z

@veronicagg I have updated the examples.

veronicagg

questions inline

veronicagg · 2017-08-04T17:09:56Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

        },
        "blobDuration": {
          "description": "Blob duration.",
-          "format": "date-time",


is the field really not a date-time?

veronicagg · 2017-08-04T17:11:27Z

...services/resource-manager/Microsoft.RecoveryServices/2016-06-01/examples/ListOperations.json

                        },
                        "Origin": "user"
                    },
-                    {


why did you decide to remove this part? It's good to include more and cover more fields, we need to make sure the wire matches what's in the swagger, that's the purpose of having the example covering more.

ravbhatnagar · 2017-08-07T22:31:25Z

@veronicagg - I see the label indicating WaitForARMFeedback. Just wanted to confirm if anything is needed from my side on this.

veronicagg · 2017-08-07T22:34:14Z

@ravbhatnagar no, you've commented on the suppression not being allowed above, and the label did not get removed then. I'll remove it now, next time, feel free to remove it after you provide your feedback. thanks!
On this PR I'm waiting on @sonathan regarding my questions on the examples.

azuresdkciprbot · 2017-08-08T10:58:52Z

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/recoveryservices/resource-manager/readme.md
Before the PR: Warning(s): 28 Error(s): 40
After the PR: Warning(s): 28 Error(s): 43

AutoRest Linter Guidelines | AutoRest Linter Issues

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

Thanks for your co-operation.

sonathan · 2017-08-08T11:36:19Z

Hi @veronicagg

As we are in preview, and the contracts had changed in service side, I have updated those here and have also updated examples to cover out as much scenarios as possible.

veronicagg · 2017-08-08T17:37:00Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

-          "description": "Blob duration.",
-          "type": "string",
-          "readOnly": true
+          "description": "blob duration",


since this field is not specified as date-time, is there an enum that corresponds to it? How does the customer know what goes in there? The example has a value that looks quite arbitrary, therefore my question. thanks!

Hi @veronicagg , As this field is in Response and Shoebox creates blobs in customer storage account every hour. The value PT1H indicates blobs per hour. Thanks!

Thanks @sonathan . Should this be part of its description? Are customers expected to know what it means, if so, how? thanks!

veronicagg · 2017-08-08T17:43:06Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

+          "type": "string"
+        }
+      }
+    },


FYI - Renaming of the model may cause a breaking change, if there were any SDKs released based on the prior spec.

veronicagg · 2017-08-08T18:10:44Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

        "name": {
-          "description": "Name of the log.",
-          "type": "string",
-          "readOnly": true


Please review: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/262193673#L1444

veronicagg · 2017-08-08T18:11:06Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

-          "readOnly": true
+          }
        },
        "NextLink": {


Please review: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/262193673#L1456

Thanks for pointing out.

veronicagg · 2017-08-08T18:21:05Z

...fication/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json

        "Name": {
-          "description": "Name of the operation.",
-          "type": "string",
-          "readOnly": true


Please review: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/262193673#L1469

@sonathan it appears that this one is still showing up https://travis-ci.org/Azure/azure-rest-api-specs/jobs/262537716#L1457 therefore causing 1 more error in the Linter compared to the spec before this PR, you may want to update this one as well.

veronicagg · 2017-08-08T18:22:30Z

@sonathan one more question, since you mentioned you're in preview...

are the changes included in this spec deployed?
is there a reason why your api-version is not updated or includes "preview" in it?

sonathan · 2017-08-08T21:15:06Z

@veronicagg Yes the changes are already deployed. To be honest we have added this API only for compliance, whenever there is a need we refer to recoveryServicesBackup for this API.

azuresdkciprbot · 2017-08-09T06:20:31Z

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/recoveryservices/resource-manager/readme.md
Before the PR: Warning(s): 28 Error(s): 40
After the PR: Warning(s): 28 Error(s): 41

AutoRest Linter Guidelines | AutoRest Linter Issues

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

Thanks for your co-operation.

veronicagg

Looks good, just one question left and one linter error that appears to not have been updated in the last commit.

veronicagg · 2017-08-11T17:22:13Z

@sonathan if you have any questions about my last comment, please let me know, thanks.

veronicagg · 2017-08-16T17:09:42Z

@sonathan any updates?

veronicagg · 2017-08-17T18:18:01Z

@sonathan FYI - the PR will get closed in a few days if there's no activity from your side. It it gets closed, you may reopen it when you're back to it. Thanks!

veronicagg · 2017-08-21T18:56:55Z

@sonathan This PR will get closed tomorrow, if no updates are received, thanks.

veronicagg · 2017-08-22T18:34:09Z

@sonathan please reopen the PR and tag me when you're back to this, thanks.

Updating Operations API to support Python's AutoRest

d769a7b

msftclas added the cla-not-required label Jul 21, 2017

azuresdkci assigned veronicagg Jul 21, 2017

sonathan added 2 commits July 21, 2017 15:15

Updating readme.md

d06d868

Adding a surpressing message in readme

80511ec

This was referenced Jul 21, 2017

Python CI test failing #1454

Closed

Updating swagger_to_sdk_config.json Azure/azure-sdk-for-python#1317

Merged

veronicagg added the potential-sdk-breaking-change label Jul 24, 2017

veronicagg suggested changes Jul 24, 2017

View reviewed changes

veronicagg suggested changes Jul 26, 2017

View reviewed changes

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

removing subsId and RGName from parameters

cafbcd9

Removing supression from ReadMe.md and updating Examples/ListOperatio…

54a1681

…ns.json with correct API-Version

veronicagg suggested changes Jul 31, 2017

View reviewed changes

Updating examples in Operations API

cf761aa

veronicagg suggested changes Aug 4, 2017

View reviewed changes

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

Updating Client operations API with new contract

a09f0f5

veronicagg suggested changes Aug 8, 2017

View reviewed changes

Correcting error DescriptionMustNotBeNodeName

d26daee

veronicagg suggested changes Aug 9, 2017

View reviewed changes

veronicagg closed this Aug 22, 2017

sonathan mentioned this pull request Aug 31, 2017

Updating Operations API to support Python's AutoRest #1619

Merged

10 tasks

Updating Operations API to support Python's AutoRest #1453

Updating Operations API to support Python's AutoRest #1453

Uh oh!

Conversation

sonathan commented Jul 21, 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 Jul 21, 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!

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 Jul 28, 2017

Uh oh!

veronicagg commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Aug 2, 2017

Uh oh!

sonathan commented Aug 4, 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!

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar commented Aug 7, 2017

Uh oh!

veronicagg commented Aug 7, 2017

Uh oh!

azuresdkciprbot commented Aug 8, 2017

Uh oh!

sonathan commented Aug 8, 2017

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

sonathan commented Jul 21, 2017 •

edited by azuresdkci

Loading