[Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 #5207

sriramvu · 2019-02-15T05:59:24Z

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

I have reviewed the documentation for the workflow.
Validation tools were run on swagger spec(s) and have all been fixed in this PR.

…/2018-01-10 to version 2018-07-10

AutorestCI · 2019-02-15T06:00:41Z

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

AutorestCI · 2019-02-15T06:01:14Z

Automation for azure-sdk-for-ruby

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

AutorestCI · 2019-02-15T06:03:05Z

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

AutorestCI · 2019-02-15T06:03:45Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

azuresdkci · 2019-02-15T06:04:34Z

Can one of the admins verify this patch?

AutorestCI · 2019-02-15T06:09:49Z

Automation for azure-sdk-for-node

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-node#4747

AutorestCI · 2019-02-15T06:52:42Z

Automation for azure-sdk-for-js

Encountered an unknown error: (azure-sdk-for-js)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/azure_devtools/ci_tools/github_tools.py", line 33, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 170, in rest_handle_action
    return rest_pull_close(body, restapi_repo, sdk_pr_target_repo, sdkbase, sdk_tag)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 185, in rest_pull_close
    rest_pr_management(rest_pr, sdk_pr_target_repo, sdk_tag, sdk_default_base)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github_handler.py", line 151, in rest_pr_management
    sdk_tag=sdk_tag
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/SwaggerToSdkNewCLI.py", line 309, in generate_sdk_from_git_object
    sdk_repo.git.push('origin', base_branch, set_upstream=True)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 548, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 1014, in _call_process
    return self.execute(call, **exec_kwargs)
  File "/usr/local/lib/python3.6/dist-packages/git/cmd.py", line 825, in execute
    raise GitCommandError(command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git push --set-upstream origin restapi_auto_recoveryservicessiterecovery/resource-manager
  stderr: 'remote: Permission to Azure/azure-sdk-for-js.git denied to AutorestCI.
fatal: unable to access 'https://AutorestCI:58ab395c311d1bd75b3e1db1cc8adaf9acc42afe@github.com/Azure/azure-sdk-for-js.git/': The requested URL returned error: 403'

sriramvu · 2019-02-15T07:14:02Z

Introduced new API version 2018-07-10 for Azure Site Recovery (ASR) service:

Breaking changes:

modified properties in InMageAzureV2EnableProtectionInput and InMageAzureV2ReplicationDetails using new contracts InMageAzureV2DiskInputDetails and InMageAzureV2ManagedDiskDetails
removed roleAssignment

Incremental changes:

new APIs addDisks, removeDisks, resolveHealthErrors, replicationSupportedOperatingSystems got added

Ran autorest --azure-validator and sanity tools under OpenAPI Validation Tools and found no issues.

nschonni · 2019-02-15T22:15:38Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

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


Resolvability -> Resolvable? Could just be a boolean that way

Basically, we thought of the boolean approach. But decided against it, due to the limitation in future extendability if we ever wanted a new value apart from the current set i.e Allowed, NotAllowed.

ravbhatnagar

few comments

ravbhatnagar · 2019-02-20T01:42:41Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

@@ -3322,6 +3476,83 @@
        }
      }
    },
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{resourceName}/replicationFabrics/{fabricName}/replicationProtectionContainers/{protectionContainerName}/replicationProtectedItems/{replicatedProtectedItemName}/ResolveHealthErrors": {


nit: camel case for subscriptions

We will be taking later as it reflects all the existing APIs too.

ravbhatnagar · 2019-02-20T01:43:17Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

@@ -3180,6 +3257,83 @@
        }
      }
    },
+    "/Subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RecoveryServices/vaults/{resourceName}/replicationFabrics/{fabricName}/replicationProtectionContainers/{protectionContainerName}/replicationProtectedItems/{replicatedProtectedItemName}/removeDisks": {


ensure linked access check is happening in ARM manifest.

As @innosam has communicated over mail, we are updating manifest in parallel.

ravbhatnagar · 2019-02-20T01:43:42Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

@@ -7189,6 +7485,17 @@
          "description": "The data pending at source virtual machine in MB.",
          "type": "number"
        },
+        "diskState": {


this should be modeled as enum

As far as I know using enums is not considered a good practice. Are you are referring to allowed values construct?

why is enums not a good practice? you can set modelasString:true and that will not be a breaking change when you add new values.

ravbhatnagar · 2019-02-20T01:45:25Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

@@ -8127,13 +8502,6 @@
            "type": "string"
          }
        },
-        "roleAssignments": {


why is this property being removed?

Om Nishant omnishan@microsoft.com: After the roles and responsibilities were revisited to keep up with compliance and customer expectations, extraneous fields were removed to prevent any misleading and unrealistic expectations.

what happens when customer sets this property using an old api-version which supported it and gets the resource using new api-version. And then may be after that, does a PUT again from the new api-version

roleAssignments under AzureToAzureVmSyncedConfigDetails is present only in response of GET ReplicationProtectedItem.

Actually these are role assignments of the Virtual Machine that is getting replicated (Virtual Machine and ReplicationProtectedItem have 1:1 association in ASR).
We were storing & syncing roleAssignments of the Virtual Machine - which we have stopped doing (shared the reason from Om Nishant in previous comment).

So, there is no PUT and only GET associated with the property which we have removed.

...ger/Microsoft.RecoveryServices/stable/2018-07-10/examples/SupportedOperatingSystems_Get.json

ravbhatnagar · 2019-02-20T01:51:54Z

There is an open issue with this design which is due to a limitation in ASR service and second this has already been implemented on service side. Ideally addDisk, removeDisk, resolveHealthError kind of scenarios should have been addressed via a PUT or PATCH on the parent resource instead of as a POST action. Please make sure you are doing the right auth check in the manifest
Pasting email conversation below for some context -

_Gaurav Bhatnagar
Thu 11/15/2018 12:39 AM
Sure, please let me know if you have questions on Linked access checks.
Please note this is not an ideal API design. More so, when it seems like this path was chosen to overcome engineering complexities/challenges. We strongly discourage the use of POST to change resource models like in this case. Imagine if every service in Azure starts enabling POST APIs to update certain properties on the resources it owns. Soon the API surface in Azure will become confusing, inconsistent and hard to use. But since this is already implemented/deployed on the service side, we cant change it now.
Apologies again for missing this thread earlier.
Thanks,
Gaurav

Sent from Outlook
Harkirat Singh
Tue 11/13/2018 10:38 PM
Please find the comments inline.

From: Gaurav Bhatnagar Gaurav.Bhatnagar@microsoft.com
Sent: 14 November 2018 02:42
To: Harkirat Singh hsingh@microsoft.com
Cc: Himanshu Saini Himanshu.Saini@microsoft.com; Mukesh Kumar (CDAP) mukeshk@microsoft.com; Vijay Narayan vijay.narayan@microsoft.com; Azure Resource Manager RP API Review armapireview@microsoft.com
Subject: Re: SiteRecovery API to support add/remove protected disks
Ok, both these are not ideal. Wanted to clarify a couple of things -

For #1 comment below - Does the second PUT fail on an existing resource fail if some property values are changed in the payload. If a property is patchable using a PATCH API, it should be updatable using a PUT as well.

I can confirm that for replication protected item, the second PUT call with the same input is handled idempotence. While, if some of the input is changed, we fail with a user error.

#2a -Seems like an internal implementation detail.

yes this is specific to asr, but the jobs are available as ARM resource replicationJobs/{jobname}.

And, when the get is done on the resource we expose the list of task executed for a particular long running job.

#2b - IMO, it doesn't complicate the scenario. May be it complicates the implementation on the backend. But, as far as user experience is concerned, it provides flexibility to the user. The user can choose to update any number of properties as part of the patch. the user doesn't have to update a few properties using PATCH and somehow figure out that for disk update they need a separate POST API.

Yes, it does complicate the implementation. From user experience perspective, the bright side for the POST call is that user need not provided the existing disk set and their parameters, when adding a new disk.

Moreover, in the current model, RBAC is broken. Since now someone without /write permissions on replicationProtectedItems can change its state by doing a POST.

It looks like this has already been implemented. Can you please confirm? If yes, I would recommend that we atleast do a linked access check to ensure the RBAC is not an issue.

Yes, we have already completed the implementation. I will definitely ensure that the linked access check to ensure the RBAC is not an issue.

Thanks for bringing this point, I will reach you if I have doubts on “how to” of this required action.

Thanks,

Gaurav

Sent from Outlook

From: Harkirat Singh
Sent: Tuesday, November 13, 2018 1:39 AM
To: Gaurav Bhatnagar
Cc: Himanshu Saini; Mukesh Kumar (CDAP); Vijay Narayan
Subject: RE: SiteRecovery API to support add/remove protected disks

Hey Gaurav,

I was away last week for Diwali holidays, so could not respond.

Yes, we did consider the PATCH/PUT call, and eventually decided on separate POST due to following reasons:

PUT
The site recovery service was not designed to have PUT calls update the parameters.

            So, currently none of our PUT calls support update.

PATCH
We do have a PATCH call on the protected item, where multiple parameters can be updated.

            But using this verb for add/remove disks have following cons for us:

a. The PATCH call fires a workflow/job which is generic in nature.

The add/remove disk scenario needed descriptive task which are different from the current PATCH.

b. And, if we do allow the disk update through PATCH, user can club different updates together with add/remove disk, which complicates the scenario.

In summary separate API’s is more convenient to the user as only the diff of the disk needs to be provided.

Thanks

Harkirat Singh_

ravbhatnagar · 2019-02-22T00:36:09Z

@sriramvu - just one pending open question

viverm · 2019-02-22T13:48:41Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

+        }
+      ],
+      "properties": {
+        "vmDisksUris": {


is naming fine DisksUris?

property names should also be camel cased.

vmDisksUris is camel cased, did not get the comment.
Suggesting a change?

vmDiskUris

double plural is odd

yes, this is correct. I thought you asked a question and I replied saying es, propertynames should also be camel cased.

viverm

Service is already out with changes.
No point in holding things for this minor things.

viverm · 2019-02-24T18:53:55Z

...vicessiterecovery/resource-manager/Microsoft.RecoveryServices/stable/2018-07-10/service.json

+        }
+      ],
+      "properties": {
+        "vmDisksUris": {


vmDiskUris

double plural is odd

ravbhatnagar · 2019-02-25T09:06:06Z

Signing off from ARM side.

sriramvu and others added 4 commits February 15, 2019 10:30

Adds base for updating Microsoft.RecoveryServices from version stable…

e0f3ef1

…/2018-01-10 to version 2018-07-10

Updates readme

10a3e27

Updates API version in new specs and examples

91724ea

Changes with new API version 2018-07-10

e76d1e9

AutorestCI mentioned this pull request Feb 15, 2019

[AutoPR recoveryservicessiterecovery/resource-manager] [Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 Azure/azure-sdk-for-ruby#2236

Merged

azuresdkci assigned praries880 Feb 15, 2019

azuresdkci requested a review from praries880 February 15, 2019 06:01

AutorestCI mentioned this pull request Feb 15, 2019

[AutoPR recoveryservicessiterecovery/resource-manager] [Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 Azure/azure-sdk-for-node#4688

Closed

AutorestCI mentioned this pull request Feb 15, 2019

[AutoPR recoveryservicessiterecovery/resource-manager] [Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 Azure/azure-sdk-for-js#1240

Closed

nschonni reviewed Feb 15, 2019

View reviewed changes

praries880 added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Feb 19, 2019

ravbhatnagar reviewed Feb 20, 2019

View reviewed changes

ravbhatnagar added the ARMChangesRequested <valid label in PR review process>add this label when require changes after ARM review label Feb 20, 2019

Completed the example for SupportedOperatingSystems_Get

c44f4dd

viverm reviewed Feb 22, 2019

View reviewed changes

viverm reviewed Feb 25, 2019

View reviewed changes

praries880 approved these changes Feb 25, 2019

View reviewed changes

praries880 merged commit 8e55b6e into Azure:master Feb 25, 2019

AutorestCI mentioned this pull request Feb 25, 2019

[AutoPR recoveryservicessiterecovery/resource-manager] [Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 Azure/azure-sdk-for-node#4746

Merged

sriramvu deleted the dev-recoveryservicessiterecovery-Microsoft.RecoveryServices-2018-07-10 branch February 26, 2019 09:11

sriramvu mentioned this pull request Mar 5, 2019

[Hub Generated] Review request: Enhanced A2ARecoveryPointDetails with "disks" #5307

Merged

2 tasks

sriramvu mentioned this pull request Mar 21, 2019

Azure Site Recovery (ASR) service changes for new api-version 2018-07-10 Azure/azure-sdk-for-net#5539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 #5207

[Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 #5207

sriramvu commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

azuresdkci commented Feb 15, 2019

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

sriramvu commented Feb 15, 2019

nschonni Feb 15, 2019

innosam Feb 18, 2019

ravbhatnagar left a comment

ravbhatnagar Feb 20, 2019

sriramvu Feb 21, 2019

ravbhatnagar Feb 20, 2019

sriramvu Feb 21, 2019

ravbhatnagar Feb 20, 2019

innosam Feb 21, 2019 •

edited

Loading

ravbhatnagar Feb 21, 2019

ravbhatnagar Feb 20, 2019

sriramvu Feb 21, 2019

ravbhatnagar Feb 21, 2019

sriramvu Feb 22, 2019

ravbhatnagar commented Feb 20, 2019

ravbhatnagar commented Feb 22, 2019

viverm Feb 22, 2019

ravbhatnagar Feb 22, 2019

sriramvu Feb 23, 2019

viverm Feb 24, 2019

ravbhatnagar Feb 25, 2019

viverm left a comment

viverm Feb 24, 2019

ravbhatnagar commented Feb 25, 2019

[Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 #5207

[Hub Generated] Review request for Microsoft.RecoveryServices to add version 2018-07-10 #5207

Conversation

sriramvu commented Feb 15, 2019 • edited Loading

Contribution checklist:

AutorestCI commented Feb 15, 2019 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Feb 15, 2019 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Feb 15, 2019 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Feb 15, 2019 • edited Loading

Automation for azure-sdk-for-java

azuresdkci commented Feb 15, 2019

AutorestCI commented Feb 15, 2019 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Feb 15, 2019 • edited Loading

Automation for azure-sdk-for-js

sriramvu commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravbhatnagar left a 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

innosam Feb 21, 2019 • edited Loading

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

ravbhatnagar commented Feb 20, 2019

ravbhatnagar commented Feb 22, 2019

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

viverm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravbhatnagar commented Feb 25, 2019

sriramvu commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

AutorestCI commented Feb 15, 2019 •

edited

Loading

innosam Feb 21, 2019 •

edited

Loading