Webapps undelete commands #7165

nking-1 · 2018-09-07T23:07:55Z

Description

Implements the cmdlets Get-AzureRmDeletedWebApp and Restore-AzureRmDeletedWebApp, which allow users to restore a deleted app.

Sisira is working on updating the Websites project to use the new Websites SDK and fix all breaking changes. I will rebase onto her changes once her PR is accepted. I am opening this PR for early feedback since the deadline is coming up very soon.

Cmdlet review:
https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/128

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-09-10T19:51:42Z

Marking as do not merge until this gets in: #7127

cormacpayne · 2018-09-11T16:56:53Z

@Nking92 Hey Nick, for some reason, GitHub isn't letting me pull in the latest changes from preview into your branch, so would you mind doing so to resolve the build failure you're currently seeing?

nking-1 · 2018-09-12T07:42:47Z

I had an SDK update commit on my own preview branch and then branched the webapps-undelete branch off of that. I have rebased everything to be on upstream/preview, so the SDK update is now included as part of this PR, which will hopefully fix the build. Please note that both Sisira and I have updated the SDK separately. We want to take Sisira's update as the "official" one, and I will rebase my branch on top of that once you merge her PR. So basically, the only commit that needs review is 4d3ed08, and the SDK update commit was only there to get my build working.

maddieclayton · 2018-09-12T20:03:02Z

src/ResourceManager/Websites/Commands.Websites.Test/ScenarioTests/WebAppBackupRestoreTests.ps1

+		$deletedApp = Get-AzureRmDeletedWebApp -ResourceGroupName $undeleteRgName -Name $undeleteAppName -Slot "Production"
+		$deletedSlot = Get-AzureRmDeletedWebApp -ResourceGroupName $undeleteRgName -Name $undeleteAppName -Slot $undeleteSlot
+		Restore-AzureRmDeletedWebApp $deletedApp -TargetResourceGroupName $rgname -TargetName $wname -Force
+		Restore-AzureRmDeletedWebApp $deletedSlot -TargetResourceGroupName $rgname -TargetName $wname  -TargetSlot $slotName -Force


Can you add some asserts here?

maddieclayton · 2018-09-12T20:03:31Z

src/ResourceManager/Websites/Commands.Websites.Test/ScenarioTests/WebAppBackupRestoreTests.ps1

+		New-AzureRmAppServicePlan -ResourceGroupName $rgname -Name  $whpName -Location  $location -Tier $tier
+		$deletedApp = Get-AzureRmDeletedWebApp -ResourceGroupName $undeleteRgName -Name $undeleteAppName -Slot "Production"
+		$job = $deletedApp | Restore-AzureRmDeletedWebApp -TargetResourceGroupName $rgname -TargetAppServicePlanName $whpName -Force -AsJob
+		$job | Wait-Job


Same, please add some asserts here

maddieclayton · 2018-09-12T20:03:48Z

src/ResourceManager/Websites/Commands.Websites/Az.Websites.psd1

@@ -95,7 +95,8 @@ CmdletsToExport = 'Get-AzAppServicePlan', 'Set-AzAppServicePlan',
    'New-AzWebApp', 'Remove-AzWebAppBackup', 
    'Reset-AzWebAppPublishingProfile', 'Restart-AzWebApp', 
    'Set-AzWebApp', 'Start-AzWebApp', 'Stop-AzWebApp', 
-    'Get-AzWebAppSnapshot', 'Restore-AzWebAppSnapshot'
+    'Get-AzWebAppSnapshot', 'Restore-AzWebAppSnapshot',
+    'Get-AzDeletedWebApp'


Missing one of the cmdlets

Please follow these instructions to create the right aliases for the Az -> AzureRm netcore cmdlets. https://github.com/Azure/azure-powershell/blob/preview/documentation/development-docs/create-alias-mappings.md

maddieclayton · 2018-09-12T20:05:07Z

src/ResourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/AzureDeletedWebApp.cs

+
+namespace Microsoft.Azure.Commands.WebApps.Cmdlets.WebApps
+{
+    public class AzureDeletedWebApp


Please add a "PS" prefix to indicate that this is a PowerShell model

maddieclayton · 2018-09-12T20:07:53Z

...esourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/RestoreAzureDeletedWebApp.cs

+    /// <summary>
+    /// Restores a deleted Azure Web App's contents and settings to an existing web app
+    /// </summary>
+    [Cmdlet("Restore", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "DeletedWebApp"), OutputType(typeof(void))]


Would it make sense to do a get here and return the restored web app? I feel like that would be the expected behavior.

maddieclayton · 2018-09-12T20:09:37Z

...esourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/RestoreAzureDeletedWebApp.cs

+    {
+        // Note, there can be multiple deleted web apps with the same name and resource group. Internally each deleted app has a unique ID.
+        [Parameter(Position = 0, Mandatory = true, HelpMessage = "The deleted Azure Web App.", ValueFromPipeline = true)]
+        public AzureDeletedWebApp InputObject;


Looks like you only did the inputobject parameter set - you need the interactive one as well (as discussed in the design review)

Restore-AzureRmDeletedWebApp -ResourceGroupName <string> -Name <string> [-TargetResourceGroupName <string>] -TargetName <string>

Sorry, I overlooked that. The problem is that there could be a web app with the same name and resource group that was deleted and re-created multiple times. This is actually a relatively common case. In the backend we've given each deleted app a unique ID to avoid ambiguity.

We could default to restoring the most recent deleted app with the provided name and resource group. Do you think that would be good, or could it be too confusing?

Hmm I do see your point. I think it would be good to be restore the most recently deleted app, but add a warning message if there were multiple stating that we chose to restore the most recent app. Could you also add a parameter set to accept that unique id? Or do you think that the users would not ever want to use that functionality? Apologies if we've previously discussed this.

I'm working on adding the option to undelete by deleted site name with your suggestions.

I don't think many people would use a deleted site ID parameter. The ID is a generated integer that's only discoverable by using the Get-AzureRmDeletedWebApp cmdlet. It's easiest to just use the deleted site object directly instead of pulling the ID out of it. The ID is also something we'd prefer to keep somewhat hidden from customers because it's not much use to them.

maddieclayton · 2018-09-12T20:10:20Z

...esourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/RestoreAzureDeletedWebApp.cs

+        public AzureDeletedWebApp InputObject;
+
+        [Parameter(Position = 1, Mandatory = true, HelpMessage = "The resource group containing the new Azure Web App.", ValueFromPipelineByPropertyName = true)]
+        public string TargetResourceGroupName { get; set; }


In the design review this was not mandatory - why has this changed? Also, none of these parameters should have ValueFromPipelineByPropertyName = true, please remove from all.

Changed this to be optional

maddieclayton · 2018-09-13T20:02:50Z

@Nking92 Is this feature supposed to go in stable or a preview module?

maddieclayton · 2018-09-14T01:05:41Z

@Nking92 Please fix merge conflicts.

maddieclayton · 2018-09-14T18:30:39Z

@Nking92 In order to get into the Ignite release you will need to update this PR by the end of the day. We are required to hand off to cloudshell by 10am Monday morning.

…uild errors from breaking changes. Re-record backup tests.

…dWebApp

…sserts to tests. Fix az mapping.

…K. Fix build errors from breaking changes. Re-record backup tests." This reverts commit 6964d17.

maddieclayton · 2018-09-14T22:31:03Z

...esourceManager/Websites/Commands.Websites/Cmdlets/BackupRestore/RestoreAzureDeletedWebApp.cs

+    /// <summary>
+    /// Restores a deleted Azure Web App's contents and settings to an existing web app
+    /// </summary>
+    [Cmdlet("Restore", ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "DeletedWebApp"), OutputType(typeof(PSSite))]


This should have a DefaultParameterSetName attribute

Please also add help files for your change: https://github.com/Azure/azure-powershell/blob/preview/documentation/development-docs/help-generation.md

Also please update the changelog at src/Websites/Commands.Websites/Changelog.md

azuresdkci1x added the needs-review label Sep 7, 2018

maddieclayton added the Do Not Merge 🚫 label Sep 10, 2018

maddieclayton self-assigned this Sep 10, 2018

maddieclayton added the Preview label Sep 11, 2018

nking-1 force-pushed the webapps-undelete branch from 0908a7b to 4d3ed08 Compare September 12, 2018 07:36

maddieclayton suggested changes Sep 12, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Sep 12, 2018

maddieclayton assigned nking-1 and unassigned maddieclayton Sep 12, 2018

maddieclayton added Ignite and removed Preview labels Sep 13, 2018

maddieclayton changed the base branch from preview to release-2018-09-17 September 14, 2018 01:05

maddieclayton added Merge Conflicts and removed Do Not Merge 🚫 labels Sep 14, 2018

Nicholas King added 5 commits September 14, 2018 15:00

Update WebSites projects to use v2.0.0 WebSites Management SDK. Fix b…

6964d17

…uild errors from breaking changes. Re-record backup tests.

Implement and test Get-AzureRmDeletedWebApp and Restore-AzureRmDelete…

7e5b324

…dWebApp

Remove secrets from backup test recordings

97bea41

Add interactive parameter set for Restore-AzureRmDeletedWebApp. Add a…

cf9eb4f

…sserts to tests. Fix az mapping.

Revert "Update WebSites projects to use v2.0.0 WebSites Management SD…

12788c0

…K. Fix build errors from breaking changes. Re-record backup tests." This reverts commit 6964d17.

nking-1 force-pushed the webapps-undelete branch from 3072907 to 12788c0 Compare September 14, 2018 22:16

maddieclayton suggested changes Sep 14, 2018

View reviewed changes

Nicholas King added 2 commits September 14, 2018 16:44

Add help files for deleted web app cmdlets. Update changelog.

9182bf5

Add online version field to deleted app help files to fix the build

38072bd

Nicholas King and others added 3 commits September 14, 2018 17:36

Add DeletedWebApps extension method to websites client for .NET Core

bcf2f4f

Update RestoreAzureDeletedWebApp.cs

e73978c

Update Commands.Websites.Test.csproj

a4ea039

maddieclayton approved these changes Sep 15, 2018

View reviewed changes

maddieclayton merged commit a259f88 into Azure:release-2018-09-17 Sep 15, 2018

Webapps undelete commands #7165

Webapps undelete commands #7165

Uh oh!

Conversation

nking-1 commented Sep 7, 2018

Description

Checklist

Uh oh!

maddieclayton commented Sep 10, 2018

Uh oh!

cormacpayne commented Sep 11, 2018

Uh oh!

nking-1 commented Sep 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maddieclayton commented Sep 13, 2018

Uh oh!

maddieclayton commented Sep 14, 2018

Uh oh!

maddieclayton commented Sep 14, 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!

Uh oh!