Added -Vault parameter to RecoveryServices.Backup cmdlets #6327

dragonfly91 · 2018-05-29T18:48:30Z

Description

Added -Vault parameter to RecoveryServices.Backup cmdlets

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

maddieclayton · 2018-05-30T00:25:45Z

@dragonfly91 please fill out a cmdlet design review here: https://github.com/Azure/azure-powershell-cmdlet-review-pr

dragonfly91 · 2018-05-30T16:17:33Z

@maddieclayton Created this review request here: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/79. Please check!

maddieclayton · 2018-06-04T19:36:48Z

.../RecoveryServices/Commands.RecoveryServices.Backup.Test/ScenarioTests/AzureSql/ItemTests.ps1

 	$vault = Get-AzureRmRecoveryServicesVault -ResourceGroupName "sqlpaasrg" -Name "sqlpaasrn";

-	# 2. Set the vault context


Can you have one test where you set the context with Set-AzureRmRecoveryServicesVaultContext? I assume we want this to continue to work, so we should have at least one test for it.

maddieclayton · 2018-06-04T19:49:54Z

...ResourceManager/RecoveryServices/Commands.RecoveryServices.Backup/RSBackupVaultCmdletBase.cs

+        /// </summary>
+        /// <param name="jobId">ID of the job to be fetched</param>
+        /// <returns></returns>
+        public CmdletModel.JobBase GetJobObject(string jobId, ARSVault vault = null)


Do the corresponding "GetJobObject/HandleCreatedJob" calls in RecoveryServicesBackupCmdletBase still work with the changes to the adapter? If not, please remove them.

Yes, they still work.

maddieclayton · 2018-06-04T19:52:13Z

...overyServices/Commands.RecoveryServices.Backup/help/Wait-AzureRmRecoveryServicesBackupJob.md

+Accept pipeline input: True (ByValue)
+Accept wildcard characters: False
+```
+
 ### CommonParameters


Please revert the changes to the three files below this.

maddieclayton · 2018-06-04T19:52:41Z

...Commands.RecoveryServices.Backup.Providers/Commands.RecoveryServices.Backup.Providers.csproj

@@ -88,6 +88,10 @@
      <Project>{b758fec1-35c1-4f93-a954-66dd33f6e0ec}</Project>
      <Name>Commands.RecoveryServices.Backup.ServiceClientAdapter</Name>
    </ProjectReference>
+    <ProjectReference Include="..\Commands.RecoveryServices\Commands.RecoveryServices.csproj">


Why do you need to add this reference?

This project contains the definition of ARSVault which is being referenced by the methods in the provider files.

@dragonfly91 until all of the RecoveryServices modules are consolidated into one, we should avoid introducing any coupling between the types -- this prevents us from getting into scenarios where users can only use certain Backup functionality when paired with RecoveryServices cmdlets. Once the modules are consolidated, we can have a common models projects that all of the projects share and can use interchangeably without worrying about having additional modules installed and run beforehand.

@dragonfly91 as a workaround, we could have the user provide the vault resource id, which you can parse using the ResourceIdentifier class, to get the vault name and resource group, which appear to be the only two properties you are using from the vault object (let me know if that's not the case)

@cormacpayne

When we had this discussion last time regarding merge, this point about how to address module dependency was never mentioned explicitly by the ADX team, I believe. Removing dependency on the Set-AzureRmRecoveryServicesVaultContext cmdlet is one of the biggest pain points for the customers and providing the Vault object is the most comfortable way for them. @pvrk can you comment on the workaround suggested by @cormacpayne ?

Also, this PR has been open for the last 9 days. This comment, given on the 9th day, essentially calls for a complete code revamp; all the files will have to be modified.

I understand your commitment to make things easy for the customer. But, I feel that there is no way to anticipate the PR review process. This apparent issue in our design could have been:

Educated in the initial talks about merge

Flagged in the PR Design Review

Flagged in the first few days of PR Code Review.

My concern: why didn't this comment come up in any of the above steps? We have now worked on potential throw away code and also risk missing the release.

Hope you understand our pain point and we need you to address this clearly.

@cormacpayne : So the idea is to use vault ID as a workaround and switch it back to object when the code merger is done? Even though if the idea is to use vault ID throughout, it came at the very last minute at a very high cost (we need to modify all the files) and I concur with @dragonfly91 here. We need such inputs as early as possible.

maddieclayton · 2018-06-04T19:53:00Z

...eryServices/Commands.RecoveryServices.Backup/Commands.RecoveryServices.Backup.Cmdlets.csproj

@@ -109,6 +110,10 @@
      <Project>{b758fec1-35c1-4f93-a954-66dd33f6e0ec}</Project>
      <Name>Commands.RecoveryServices.Backup.ServiceClientAdapter</Name>
    </ProjectReference>
+    <ProjectReference Include="..\Commands.RecoveryServices\Commands.RecoveryServices.csproj">


Why do you need to add this reference?

This project contains the definition of ARSVault which is being referenced by the methods in this project.

MiYanni · 2018-06-06T01:27:35Z

@dragonfly91 You have merge conflicts that need to be resolved.

dragonfly91 · 2018-06-06T11:16:45Z

Fixed the merge conflicts. Please continue review!

maddieclayton · 2018-06-12T17:02:28Z

on demand here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/931/

dragonfly91 · 2018-06-12T19:08:42Z

@maddieclayton Thank you for getting this in!

azuresdkci added the needs-review label May 29, 2018

maddieclayton added the Cmdlet Review Required ⚠️ label May 30, 2018

maddieclayton assigned dragonfly91 May 30, 2018

maddieclayton removed the needs-review label May 30, 2018

maddieclayton added needs-review Cmdlet Review Complete and removed Cmdlet Review Required ⚠️ labels Jun 1, 2018

maddieclayton assigned maddieclayton and unassigned dragonfly91 Jun 1, 2018

maddieclayton suggested changes Jun 4, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Jun 4, 2018

maddieclayton assigned dragonfly91 and maddieclayton and unassigned maddieclayton and dragonfly91 Jun 4, 2018

maddieclayton added needs-review and removed needs-revision labels Jun 5, 2018

dragonfly91 force-pushed the 1805changes branch from 2ed5bd7 to 7355ff5 Compare June 6, 2018 05:33

maddieclayton assigned cormacpayne and unassigned maddieclayton Jun 6, 2018

dragonfly91 added this to the 2018-06-15 milestone Jun 7, 2018

cormacpayne added needs-revision and removed needs-review labels Jun 7, 2018

cormacpayne assigned dragonfly91 and unassigned cormacpayne Jun 7, 2018

dragonfly91 force-pushed the 1805changes branch from e025c1c to 3075382 Compare June 12, 2018 12:15

Maddie Clayton added 20 commits June 12, 2018 09:45

Update Backup-AzureRmRecoveryServicesBackupItem.md

694ad97

Update Disable-AzureRmRecoveryServicesBackupProtection.md

7adc446

Update Disable-AzureRmRecoveryServicesBackupRPMountScript.md

7cf4396

Update Wait-AzureRmRecoveryServicesBackupJob.md

1a283ea

Update Unregister-AzureRmRecoveryServicesBackupManagementServer.md

9dcfc92

Update Get-AzureRmRecoveryServicesBackupProtectionPolicy.md

5718f8c

Update Enable-AzureRmRecoveryServicesBackupProtection.md

f85a3a5

Update Get-AzureRmRecoveryServicesBackupJob.md

4afb765

Update Get-AzureRmRecoveryServicesBackupContainer.md

abc57c2

Update Unregister-AzureRmRecoveryServicesBackupContainer.md

d2278e8

Update Restore-AzureRmRecoveryServicesBackupItem.md

237c3ec

Update Set-AzureRmRecoveryServicesBackupProtectionPolicy.md

17a71a8

Update Get-AzureRmRecoveryServicesBackupItem.md

997e8d0

Update Get-AzureRmRecoveryServicesBackupJobDetails.md

9c3bc4d

Update Get-AzureRmRecoveryServicesBackupRPMountScript.md

4863996

Update Get-AzureRmRecoveryServicesBackupManagementServer.md

104dbb4

Update Get-AzureRmRecoveryServicesBackupRecoveryPoint.md

da1d375

Update New-AzureRmRecoveryServicesBackupProtectionPolicy.md

2b27d27

Update Remove-AzureRmRecoveryServicesBackupProtectionPolicy.md

e151ff4

Update Stop-AzureRmRecoveryServicesBackupJob.md

bc796e1

maddieclayton approved these changes Jun 12, 2018

View reviewed changes

maddieclayton added Waiting for On Demand and removed Cmdlet Review Complete needs-revision labels Jun 12, 2018

maddieclayton assigned maddieclayton and unassigned dragonfly91 Jun 12, 2018

maddieclayton merged commit 98667af into Azure:preview Jun 12, 2018

		$vault = Get-AzureRmRecoveryServicesVault -ResourceGroupName "sqlpaasrg" -Name "sqlpaasrn";

		# 2. Set the vault context

Added -Vault parameter to RecoveryServices.Backup cmdlets #6327

Added -Vault parameter to RecoveryServices.Backup cmdlets #6327

Uh oh!

Conversation

dragonfly91 commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

maddieclayton commented May 30, 2018

Uh oh!

dragonfly91 commented May 30, 2018

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!

dragonfly91 Jun 8, 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!

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!

MiYanni commented Jun 6, 2018

Uh oh!

dragonfly91 commented Jun 6, 2018

Uh oh!

maddieclayton commented Jun 12, 2018

Uh oh!

dragonfly91 commented Jun 12, 2018

Uh oh!

Uh oh!

dragonfly91 commented May 29, 2018 •

edited

Loading

dragonfly91 Jun 8, 2018 •

edited

Loading