Bug fixes and a few missing APIs by dragonfly91 · Pull Request #1089 · Azure/azure-rest-api-specs

dragonfly91 · 2017-03-31T01:11: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.

dsgouda · 2017-03-31T16:51:33Z

+            "$ref": "#/parameters/VaultName"
+          },
+          {
+            "name": "resourceBackupResourceVaultConfig",


resourceBackupResourceVaultConfig sounds too wordy and doesn't provide a lot of semantic information, consider renaming it to just backupResourceVaultConfig

dsgouda · 2017-03-31T16:54:44Z

+            "$ref": "#/parameters/VaultName"
+          },
+          {
+            "name": "resourceBackupResourceConfig",


Same comment as above for naming the param

dsgouda · 2017-03-31T16:59:21Z

+        }
+      }
+    },
+    "BackupResourceVaultConfigResource": {


This is Very confusing. I'd rather say BackupResourceVaultConfig with an allOf on BackupResourceVaultConfigProperties, on similar lines, you don't need to say BackupResource, just calling it Backup is just fine unless that is what makes semantic sense at your services' end.
My intention here is to point out that the model names are getting too verbose here without adding a lot of value.

dsgouda · 2017-03-31T17:01:49Z

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


Just call this BackupConfigProperties and call BackupResourceConfigResource as BackupConfig/BackupConfigResource

dsgouda · 2017-03-31T17:03:35Z

+      "properties": {
+        "properties": {
+          "$ref": "#/definitions/BackupResourceConfig",
+          "x-ms-client-flatten": true


Good stuff! 👍

dsgouda · 2017-03-31T17:07:07Z

+          "readOnly": true
+        },
+        "lastUpdatedTimeUtc": {
+          "description": "UTC time at which the upgrade operation status was last updated.",


Consider setting a date-time format here

Can't be changed - API is already in production

dsgouda · 2017-03-31T17:07:15Z

+          "type": "string",
+          "readOnly": true
+        },
+        "endTimeUtc": {


Consider setting a date-time format here

Can't be changed - API is already in production

dsgouda · 2017-03-31T17:08:50Z

        "properties": {
-          "$ref": "#/definitions/VaultProperties",
-          "x-ms-client-flatten": true
+          "$ref": "#/definitions/VaultProperties"


Any reason why you don't want the VaultProperties to be flattened here, it would be a better user experience if you did flattening

There seems to be a bug in our RP at this point where this structure bodes well with the service. When I applied client flattening, there were issues during Create Vault API call.

@olydis do we have any recommendations here, the nesting of properties seems a little unnatural.

Yes, flattening is recommended. If there was a bug with flattening applied, it would be very helpful for us to file that bug (tons of services rely on flattening - so it better be bug free).

I understand @dragonfly91 implied that there is a bug at their service end. If this indeed is the structure that the service expects, this shouldn't be a blocker but we highly recommend that this be revisited and fixed.

dsgouda · 2017-03-31T17:11:31Z

+        }
+      }
+    },
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/backupconfig/vaultconfig": {


General convention for constructing URLs is providers/provider.namespace/typename/{typevalue} Consider parameterizing /backupconfig/vaultconfig accordingly

dsgouda · 2017-03-31T17:12:15Z

+        "deprecated": false
+      }
+    },
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/backupstorageconfig/vaultstorageconfig": {


General convention for constructing URLs is providers/provider.namespace/typename/{typevalue} Consider parameterizing /backupstorageconfig/vaultstorageconfig accordingly

I acknowledge this issue but we can't take this at this moment, as this API is already in production

Thanks, please track this issue appropriately at your end since moving forward we plan to make these as hard requirements

dsgouda

I have a minor doubt regarding client flattening that I need to get resolved, will approve once that's done. Also, posted a couple of related questions

dsgouda · 2017-04-04T01:15:50Z

+          "type": "string",
+          "readOnly": true
+        },
+        "startTimeUtc": {


Just so I understand, do you mean that the service itself works with a plain string instead of a formatted date-time one?

dsgouda · 2017-04-04T01:19:25Z

        "properties": {
-          "$ref": "#/definitions/VaultProperties",
-          "x-ms-client-flatten": true
+          "$ref": "#/definitions/VaultProperties"


@olydis do we have any recommendations here, the nesting of properties seems a little unnatural.

dsgouda · 2017-04-04T01:20:39Z

+        "deprecated": false
+      }
+    },
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{vaultName}/backupstorageconfig/vaultstorageconfig": {


Thanks, please track this issue appropriately at your end since moving forward we plan to make these as hard requirements

dsgouda · 2017-04-04T01:24:06Z

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


This is much better, but I would go further and simply name this as BackupStorageConfigProperties (same for BackupVaultConfigProperties)

Is this fine for you:
BackupStorageConfigResource -> BackupStorageConfig
BackupStorageConfig -> BackupStorageConfigProperties

I would like that very much!

dsgouda

Other than the comment about client flattening recommendations, LGTM
Pending CI builds

AutorestCI · 2017-04-04T19:08:52Z

No modification for Python

AutorestCI · 2017-04-04T19:09:01Z

No modification for NodeJS

AutorestCI · 2017-04-04T19:09:09Z

Azure/azure-sdk-for-ruby#740

Add new VSOnline Plan access token APIs to PPE and Prod Swagger

Bug fixes and a few missing APIs

eca6fef

msftclas added the cla-not-required label Mar 31, 2017

azuresdkci assigned dsgouda Mar 31, 2017

dsgouda suggested changes Mar 31, 2017

View reviewed changes

Backup Vault Config and Backup Storage Config APIs - renamings.

da3d9ef

dsgouda reviewed Apr 4, 2017

View reviewed changes

Samuel Anudeep added 2 commits April 4, 2017 11:43

Backup Config API model renames

4c51ebb

Marking date time format

d185152

dsgouda approved these changes Apr 4, 2017

View reviewed changes

dsgouda merged commit 1c421b4 into Azure:master Apr 4, 2017

AutorestCI mentioned this pull request Apr 4, 2017

Automatic PR from RestAPI-PR1089 Azure/azure-sdk-for-ruby#740

Merged

dragonfly91 mentioned this pull request Apr 4, 2017

RecoveryServicess on latest Swagger Azure/azure-sdk-for-net#3028

Merged

8 tasks

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

Merge pull request #1089 from cmbrose/dev/bcaleb/PlanAccessTokenApis

57a9003

Add new VSOnline Plan access token APIs to PPE and Prod Swagger

Conversation

dragonfly91 commented Mar 31, 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!

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!

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!

dsgouda left a comment

Choose a reason for hiding this comment

Uh oh!

AutorestCI commented Apr 4, 2017

Uh oh!

AutorestCI commented Apr 4, 2017

Uh oh!

AutorestCI commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dragonfly91 commented Mar 31, 2017 •

edited by azuresdkci

Loading