Rollback modules script #5140

maddieclayton · 2017-12-13T20:44:21Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

…nto RollBack

cormacpayne · 2017-12-14T16:13:22Z

@azuresdkci test this please

cormacpayne

@maddieclayton a couple of comments to take a look at

cormacpayne · 2017-12-14T16:16:27Z

tools/DeleteModules.ps1

+# limitations under the License.
+# ----------------------------------------------------------------------------------
+
+param(


@maddieclayton I would add a CmdletBinding attribute to this file and implement SupportsShouldProcess so we can pass -WhatIf to see which modules are being deleted

cormacpayne · 2017-12-14T16:17:47Z

tools/DeleteModules.ps1

+    $repoName = "PSGallery"
+}
+
+if (($scope -eq "ModuleList") -and ($listOfModules -eq $null))


@maddieclayton can we create two separate parameter sets, one that takes Scope and another that takes ListOfModules?

cormacpayne · 2017-12-14T16:23:23Z

tools/DeleteModules.ps1

+        $azureRmCurrent = $azureRmAllVersions[0]
+        $azureRmPrevious = $azureRmAllVersions[1]
+        $currentDependencies = Find-Module -Name $query -Repository $repoName -RequiredVersion $azureRmCurrent.Version -IncludeDependencies
+        $previousDependencies = Find-Module -Name $query -Repository $repoName -RequiredVersion $azureRmPrevious.Version -IncludeDependencies


@maddieclayton these two calls to Find-Module ... -IncludeDependencies are very time expensive. The same information we are getting with these calls (dependent module names and versions) is already contained in $azureRmCurrent.Dependencies and $azureRmPrevious.Dependencies.

cormacpayne · 2017-12-14T16:25:41Z

tools/DeleteModules.ps1

+        Add-AzureRMAccount
+    }
+
+    $secret = Get-AzureKeyVaultSecret -VaultName kv-azuresdk -Name $vaultKey


@maddieclayton since this cmdlet requires us to have AzureRM.KeyVault installed, we should have a #Requires at the top of this script for AzureRM.KeyVault

cormacpayne · 2017-12-14T16:27:07Z

tools/DeleteModules.ps1

+
+if ([string]::IsNullOrEmpty($nugetExe))
+{
+    Write-Verbose "Use default nuget path"


@maddieclayton I would change this Write-Verbose to Write-Host, move it below the if-statement, and change the text to

Using the following NuGet path: $nugetExe

cormacpayne · 2017-12-14T16:28:02Z

tools/DeleteModules.ps1

+
+if ([string]::IsNullOrEmpty($repoName))
+{
+    Write-Verbose "Deleting from PSGallery"


@maddieclayton I would change this to Write-Host, moving it below the if-statement, and changing the text to

Deleting modules from the following repository: $repoName

cormacpayne · 2017-12-14T16:29:06Z

tools/DeleteModules.ps1

+$ModulesToDelete = Get-TargetModules -scope $scope -moduleList $listOfModules
+$ModulesToDelete
+$azureModules = Find-Module Azure* -Repository $repoName
+$ModulesToDelete | ForEach-Object {


@maddieclayton this should be wrapped in the ShouldProcess

maddieclayton · 2017-12-15T21:19:35Z

@azuresdkci Test this please

markcowl

A few minor changes, otherwise LGTM

markcowl · 2017-12-16T00:21:55Z

tools/DeleteModules.ps1

+    [Parameter(ParameterSetName='Scope', Mandatory = $true, Position = 0)]
+    [ValidateSet("AzureRMAndDependencies", "AzureAndDependencies", "NetCoreModules", "AzureStackAndDependencies")]
+    [string] $scope,
+    [Parameter(ParameterSetName='ModuleList', Mandatory = $false)]


I think this is mandatory for the ModuleList parameter set, isn't it?

markcowl · 2017-12-16T00:24:36Z

tools/DeleteModules.ps1

+    [string] $scope,
+    [Parameter(ParameterSetName='ModuleList', Mandatory = $false)]
+    [string[]] $listOfModules,
+    [Parameter(Mandatory = $false)]


One thing that would be nice is the ability to remove specific versions of modules - this would be useful for AzureStack client modules, for example. Having another parameter set with the module list as a HashTable Name => Version or the like would make this possible

Note - this is something we do not need to do for this check-in

Syntax is:
.\DeleteModules.ps1 -moduleVersionTable @(@{"Module" = "AzureRM.Websites"; "Version" = "3.0.0"}, @{"Module" = "AzureRM.Websites"; "Version" = "4.0.0"}) -repoName TestGallery

Does that look good to you?

markcowl · 2017-12-16T00:25:20Z

tools/DeleteModules.ps1

+    [Parameter(ParameterSetName='ModuleList', Mandatory = $false)]
+    [string[]] $listOfModules,
+    [Parameter(Mandatory = $false)]
+    [ValidateSet("TestGallery", "PSGallery")]


This seems like it should be a completer. It should be possible to remove a module from any registered repository.

I don't think I can add an argument completer to a script like this - I could refactor the whole thing into a psm1 with functions you can import but that seems like a lot of work for not much additional functionality. Just deleting the validate set for now and making the default PSGallery.

markcowl · 2017-12-16T00:26:28Z

tools/DeleteModules.ps1

+        [string]$repoName,
+        [string]$moduleName,
+        [string]$moduleVersion,
+        $allModules,


what types are these last two parameters?

markcowl · 2017-12-16T00:29:37Z

tools/DeleteModules.ps1

+        $allModules | ForEach-Object {
+            if ($_.Name -eq $targetName)
+            {
+                $moduleNotBeingDeleted = $false


I think we should make this $moduleBeingDeleted and reverse the sense of the bool

Also, I think this would be easier to follow if we used $allModules | Where {$_.Name -eq $targetName} to get the set of elements matching target name

markcowl · 2017-12-16T00:33:41Z

tools/DeleteModules.ps1

+    }
+    if ($dependencies.Count -ne 0)
+    {
+        Write-Warning "$moduleName $moduleVersion is a dependency for $dependencies the module(s) will have an orphaned dependency."


I think this should be a ShouldContinue prompt (which would mean adding a Force parameter)

markcowl · 2017-12-16T00:35:45Z

tools/DeleteModules.ps1

+
+$ModulesToDelete = Get-TargetModules -scope $scope -moduleList $listOfModules
+$ModulesToDeleteName = $ModulesToDelete | ForEach-Object {$_.Name}
+$ModulesToDeleteName


This returns the value on the Output stream. Use Write-Verbose to make this informational

Removed - you can run whatif to get the same information.

markcowl · 2017-12-16T01:21:37Z

tools/DeleteModules.ps1

+
+    $context = $null
+    try {
+        $context = Get-AzureRMContext


You can use -ErrorAction Stop in this call to ensure that it throws whatever the setting is in the PowerShell session

All review comments addressed

azdevxps added 7 commits December 4, 2017 13:16

Initialize deletemodule.psm1

80fa707

add functionality to deletemodules script

20bc7c6

Add filtering and warnings

878de68

small fix

cc14cc6

Merge branch 'preview' of https://github.com/Azure/azure-powershell i…

2134884

…nto RollBack

last tweaks

5c98f90

remove unnecessary comment

fed120a

azuresdkci added the needs-review label Dec 13, 2017

azuresdkci assigned shahabhijeet Dec 13, 2017

maddieclayton assigned markcowl and unassigned shahabhijeet Dec 13, 2017

cormacpayne previously requested changes Dec 14, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Dec 14, 2017

cormacpayne assigned maddieclayton and unassigned markcowl Dec 14, 2017

fix PR comments

50ac304

maddieclayton assigned cormacpayne and unassigned maddieclayton Dec 15, 2017

maddieclayton added needs-review and removed needs-revision labels Dec 15, 2017

markcowl requested changes Dec 16, 2017

View reviewed changes

markcowl unassigned cormacpayne Dec 16, 2017

markcowl added needs-revision and removed needs-review labels Dec 16, 2017

maddieclayton self-assigned this Dec 18, 2017

Address PR comments

2b81057

maddieclayton added the needs-review label Dec 19, 2017

maddieclayton removed the needs-revision label Dec 19, 2017

maddieclayton assigned markcowl and unassigned maddieclayton Dec 19, 2017

markcowl approved these changes Dec 20, 2017

View reviewed changes

markcowl removed the needs-review label Dec 20, 2017

markcowl merged commit 1047cd0 into Azure:preview Dec 20, 2017

maddieclayton deleted the RollBack branch December 20, 2017 01:14

Rollback modules script #5140

Rollback modules script #5140

Uh oh!

Conversation

maddieclayton commented Dec 13, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

cormacpayne commented Dec 14, 2017

Uh oh!

cormacpayne 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!

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 Dec 15, 2017

Uh oh!

markcowl 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!

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 Dec 13, 2017 •

edited by azuresdkci

Loading