Add Get/Set/Clear-AzureRmDefault Cmdlets #4707
Conversation
…nto Default # Conflicts: # src/ResourceManager/Profile/AzureRM.Profile.psd1
| .SYNOPSIS | ||
| Tests Set-AzureRmDefault when resource group given does not exist | ||
| #> | ||
| function Test-SetAzureRmDefaultResourceGroupNonexistent |
There was a problem hiding this comment.
I don't understanbd what this test is covering that isn't covered by the subsequent test
There was a problem hiding this comment.
I completely redid the test - I figured I would test all of resource group in one test because all the commands are interconnected so you can't really test one alone effectively.
| $output = Set-AzureRmDefault -ResourceGroupName "TestResourceGroup" | ||
| Assert-True { $output.Name -eq "TestResourceGroup" } | ||
| Remove-AzureRmResourceGroup -Name "TestResourceGroup" -Force | ||
| Clear-AzureRmDefault |
There was a problem hiding this comment.
We need some validation that the default was cleared
| function Test-SetAzureRmDefaultResourceGroupNonexistent | ||
| { | ||
| $output = Set-AzureRmDefault -ResourceGroupName "TestResourceGroup" | ||
| Assert-True { $output.Name -eq "TestResourceGroup" } |
There was a problem hiding this comment.
This doesn';t seem like enough validation - where does the default resource group reside, was it actually stored there? Was it removed after Clear was executed?
| #> | ||
| function Test-SetAzureRmDefaultResourceGroupExists | ||
| { | ||
| Set-AzureRmDefault -ResourceGroupName "TestResourceGroup" |
There was a problem hiding this comment.
Same comments, I don't understand what this is testing
| #> | ||
| function Test-GetAzureRmDefaultNoDefault | ||
| { | ||
| $output = Get-AzureRmDefault |
There was a problem hiding this comment.
Is it guaranteed that there will be no defaults before this test runs? Do we need to clear the defaults before to ensure this?
| public override void ExecuteCmdlet() | ||
| { | ||
| IAzureContext context = AzureRmProfileProvider.Instance.Profile.DefaultContext; | ||
| IResourceManagementClient client = AzureSession.Instance.ClientFactory.CreateCustomArmClient<ResourceManagementClient>( |
There was a problem hiding this comment.
We should simply use the CreateServiceClient call here - we only need to pvide the context and the name of the resourecemanager endpiut. This call: https://github.com/Azure/azure-powershell/blob/preview/src/Common/Commands.Common.Authentication.Abstractions/Interfaces/IClientFactory.cs#L39
There was a problem hiding this comment.
Will stay the same as discussed offline
| // If no parameters are specified, show all defaults | ||
| if (!ResourceGroup) | ||
| { | ||
| if (context.ExtendedProperties.ContainsKey(Resources.DefaultResourceGroupKey)) |
There was a problem hiding this comment.
Use the context extensions to make this simpler, for example. context.IsProeprtySet, or context.SetProperty would make this simpler:
| // If any parameters are specified, show only defaults with switch parameters set to true | ||
| if (ResourceGroup) | ||
| { | ||
| if (context.ExtendedProperties.ContainsKey(Resources.DefaultResourceGroupKey)) |
| { | ||
| if (!client.ResourceGroups.CheckExistence(ResourceGroupName)) | ||
| { | ||
| ResourceGroup parameters = new ResourceGroup("West US"); |
There was a problem hiding this comment.
This would be a reason for a ShouldContinue prompt - we should not create a resource group without teeling the user we are going to create it. This would also mean needing a -Force prompt.
I also wonder whether it is appropriate to to this here, or if this should be done lazily when the default resource group is applied (I am thinking it should be done lazily)
| /// Cmdlet to get default options. | ||
| /// </summary> | ||
| [Cmdlet(VerbsCommon.Get, "AzureRmDefault", DefaultParameterSetName = ResourceGroupParameterSet)] | ||
| [OutputType(typeof(ResourceGroup))] |
There was a problem hiding this comment.
We shoudl create a PSResourceGroup object that has the proeprtieswe want, rather than relying on the REST API class
| // If no parameters are specified, clear all defaults | ||
| if (!ResourceGroup) | ||
| { | ||
| if (Force.IsPresent || ShouldContinue(Resources.RemoveDefaultsMessage, Resources.RemoveDefaultsCaption)) |
There was a problem hiding this comment.
This needs to be inside a ShouldProcess prompt, when ShouldContinue is used, the pattern is always:
if (ShouldProcess(...) {
if (Force.IsPresent || ShouldContinue(...) {
// do something
| [Cmdlet(VerbsCommon.Clear, "AzureRmDefault", DefaultParameterSetName = ResourceGroupParameterSet, | ||
| SupportsShouldProcess = true)] | ||
| [OutputType(typeof(bool))] | ||
| public class ClearAzureRMDefaultCommand : AzureRMCmdlet |
There was a problem hiding this comment.
I think we want this to extend ContextModificationCmdlet, so that it will be able to write to the persisted scope
| [Cmdlet(VerbsCommon.Set, "AzureRmDefault", DefaultParameterSetName = ResourceGroupNameParameterSet, | ||
| SupportsShouldProcess = true)] | ||
| [OutputType(typeof(PSResourceGroup))] | ||
| public class SetAzureRMDefaultCommand : AzureRMCmdlet |
There was a problem hiding this comment.
Same comment, this needs to extend ContextModificationCmdlet, and changes to the context should be inside a ModifyContext call, see cmdlets liek Set-AzureRmContext for the pattern.
Description
Implements the cmdlets for maintaining azure defaults. Current default that can be set is resource group. PR includes cmdlets and tests (making resource group optional will come in a separate PR). See #3808.
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.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcessand haveSupportShouldProcess=truespecified in the cmdlet attribute. You can find more information onShouldProcesshere.OutputTypeattribute if any output is produced - if the cmdlet produces no output, it should implement aPassThruparameter.Cmdlet Parameter Guidelines