[Microsoft.Network] DDoS protection plans #5968

murilogr · 2018-04-17T03:17:59Z

Description

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

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

…nto preview

cormacpayne

@murilogr a few comments for you to take a look at. If you have any questions about the piping scenarios I touched on, please refer to this document.

cormacpayne · 2018-04-17T16:26:20Z

...ceManager/Network/Commands.Network/DdosProtectionPlan/GetAzureRMDdosProtectionPlanCommand.cs

+
+            if(!string.IsNullOrEmpty(this.Name))
+            {
+                var vDdosProtectionPlan = this.NetworkClient.NetworkManagementClient.DdosProtectionPlans.Get(ResourceGroupName, Name);


@murilogr does this Get method handle when ResourceGroupName is null? If not, we should have two parameter sets to prevent the user from providing Name without ResourceGroupName:

One set with an optional ResourceGroupName parameter

One set with mandatory ResourceGroupName and Name parameters

I tried it and it validates it:

PS D:\azure-powershell> Get-AzureRmDdosProtectionPlan -Name DdosProtectionPlanName Get-AzureRmDdosProtectionPlan : 'resourceGroupName' cannot be null. At line:1 char:1 + Get-AzureRmDdosProtectionPlan -Name DdosProtectionPlanName + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : CloseError: (:) [Get-AzureRmDdosProtectionPlan], ValidationException + FullyQualifiedErrorId : Microsoft.Azure.Commands.Network.Automation.GetAzureRmDdosProtectionPlan

@murilogr the idea is that we don't have a scenario that leads users to get this error ☺️ if we have the split parameter sets where the user provides both ResourceGroupName and Name or an optional ResourceGroupName, then there is no situation where they'd get this error because they'd never be able to provide just the Name parameter

Agreed :) Added parameter sets

cormacpayne · 2018-04-17T16:28:44Z

...ceManager/Network/Commands.Network/DdosProtectionPlan/GetAzureRMDdosProtectionPlanCommand.cs

+namespace Microsoft.Azure.Commands.Network.Automation
+{
+    [Cmdlet(VerbsCommon.Get, "AzureRmDdosProtectionPlan"), OutputType(typeof(PSDdosProtectionPlan))]
+    public partial class GetAzureRmDdosProtectionPlan : NetworkBaseCmdlet


@murilogr is this a tracked resource? If so, we should add an additional parameter set that allows the user to pass just ResourceId, and you can use common code to strip out the ResourceGroupName and Name. This allows users to call Get-AzureRmResource and pipe the result to this cmdlet.

What do you mean by tracked resource?

@murilogr is this a resource that has a ResourceId that the user can retrieve from a generic REST call? (i.e., using Get-AzureRmResource -- check out this doc)

cormacpayne · 2018-04-17T16:29:48Z

...ceManager/Network/Commands.Network/DdosProtectionPlan/NewAzureRMDdosProtectionPlanCommand.cs

+        [Parameter(
+            Mandatory = false,
+            HelpMessage = "Do not ask for confirmation if you want to overwrite a resource")]
+        public SwitchParameter Force { get; set; }


@murilogr is this Force parameter necessary? The Force parameter should only be needed when you prompt the user for additional confirmation during cmdlet execution. If you are not doing this, then you shouldn't need the Force parameter

cormacpayne · 2018-04-17T16:30:17Z

...anager/Network/Commands.Network/DdosProtectionPlan/RemoveAzureRMDdosProtectionPlanCommand.cs

+
+namespace Microsoft.Azure.Commands.Network
+{
+    [Cmdlet(VerbsCommon.Remove, "AzureRmDdosProtectionPlan", SupportsShouldProcess = true)]


@murilogr please ensure that this cmdlet has an output type (in this case bool)

It has when I use PassThru parameter.

cormacpayne · 2018-04-17T16:30:28Z

...anager/Network/Commands.Network/DdosProtectionPlan/RemoveAzureRMDdosProtectionPlanCommand.cs

+        [Parameter(
+            Mandatory = false,
+            HelpMessage = "Do not ask for confirmation if you want to delete resource.")]
+        public SwitchParameter Force { get; set; }


@murilogr same comment about if we need the Force parameter

cormacpayne · 2018-04-17T16:32:46Z

src/ResourceManager/Network/Commands.Network/help/AzureRM.Network.md

 Gets an application security group.

 ### [Get-AzureRmBgpServiceCommunity](Get-AzureRmBgpServiceCommunity.md)
-{{Manually Enter Get-AzureRmBgpServiceCommunity Description Here}}


@murilogr would you mind adding a description for this cmdlet since it was removed? (you can just use the description of the cmdlet if it has one)

cormacpayne · 2018-04-17T16:33:16Z

src/ResourceManager/Network/Commands.Network/help/Get-AzureRmDdosProtectionPlan.md

+---
+external help file: Microsoft.Azure.Commands.Network.dll-Help.xml
+Module Name: AzureRM.Network
+online version:


@murilogr please refer to the ValidateHelpIssues.csv file that is a part of your Jenkins build for information about how to resolve the build errors around your help files

cormacpayne · 2018-04-17T16:33:36Z

src/ResourceManager/Network/Commands.Network/help/Get-AzureRmDdosProtectionPlan.md

+
+### Example 1: Get a specific DDoS protection plan
+```
+Get-AzureRmDdosProtectionPlan -ResourceGroupName ResourceGroupName -Name DdosProtectionPlanName


@murilogr for each of the examples you are providing, please add the corresponding output that the user would see

cormacpayne · 2018-04-17T16:33:56Z

src/ResourceManager/Network/Commands.Network/help/New-AzureRmDdosProtectionPlan.md

+---
+external help file: Microsoft.Azure.Commands.Network.dll-Help.xml
+Module Name: AzureRM.Network
+online version:


@murilogr same comments in this file about build failures and output for the examples

cormacpayne · 2018-04-17T16:34:21Z

src/ResourceManager/Network/Commands.Network/help/Remove-AzureRmDdosProtectionPlan.md

+---
+external help file: Microsoft.Azure.Commands.Network.dll-Help.xml
+Module Name: AzureRM.Network
+online version:


@murilogr same comment in this file about build failures and example output

Nilambari · 2018-04-27T21:38:32Z

Merged this PR into PR:- #5993

markcowl · 2018-04-28T19:51:51Z

closing in favor of #5993

murilogr added 12 commits April 3, 2018 19:23

Update SDK version

9788293

Add implementations

882c770

Update list of operations

2ba5085

Add tests

6e8b869

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

f292f3b

…nto preview

Add new help

364ddfc

Add tests for Set-VirtualNetwork

23bb0f8

Add test recordings

5365497

Rerun help with latest platyps

9dc1221

Remove duplicated files

c50dd42

Add change to ChangeLog.md

8146fe1

Add examples

a3470b1

azuresdkci added the needs-review label Apr 17, 2018

cormacpayne suggested changes Apr 17, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Apr 17, 2018

cormacpayne assigned murilogr Apr 17, 2018

murilogr added 5 commits April 17, 2018 17:16

Remove unnecessary ValueFromPipelineByPropertyName

2629662

Remove Force flags

61eaa87

Update help and examples

36183fc

Add boilerplate description for BGP

ed42572

Add test recordings to csproj

ae07722

markcowl assigned cormacpayne Apr 18, 2018

murilogiacometti and others added 4 commits April 18, 2018 20:42

Merge branch 'preview' into ddosProtectionPlan

dac2b82

Remove outdated instructions for tests

767a32f

Add parameter sets

877de1e

Re-record tests

bb29ed1

markcowl added the Post Build label Apr 25, 2018

Nilambari added a commit to Nilambari/azure-powershell that referenced this pull request Apr 27, 2018

Merge PR:Azure#5968

f1969cd

markcowl closed this Apr 28, 2018

[Microsoft.Network] DDoS protection plans #5968

[Microsoft.Network] DDoS protection plans #5968

Uh oh!

Conversation

murilogr commented Apr 17, 2018

Description

Checklist

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!

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!

Nilambari commented Apr 27, 2018

Uh oh!

markcowl commented Apr 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants