CDN - PowerShell Functionality and Doc Improvements #2687

CamSoper · 2016-07-28T23:53:50Z

Made a ton of improvements for usability and documentation.

Modified all "Get-" cmdlets to return collections where a specific resource wasn't named.
Modified all cmdlets to accept pipeline input objects. E.g., Get-AzureRmCdnProfile now returns all profiles a user has access to. Get-AzureRmCdnProfile | Get-AzureRmCdnEndpoint will list all endpoints on all profiles a user has access to. Get-AzureRmCdnProfile | Get-AzureRmCdnEndpoint | Get-AzureRmCdnCustomDomain will list all the custom domains. Etc., etc.
Modified collection outputs to enumerate properly for Where-Object and ForEach-Object
Cleaned up cmdlet parameter help for grammar and consistency
Cleaned up help XML for grammar and consistency.
Refactored anyplace that was using exceptions for flow control, since this is generally frowned upon.
Annotated my commits.
Created new tests for improved pipeline functionality
Re-recorded all test sessions.

Still to-do: The help XML needs Examples.

…onally. :)

azurecla · 2016-07-28T23:53:59Z

Hi @CamSoper, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

It looks like you're working at Microsoft (casoper). If you're full-time, we DON'T require a contribution license agreement.

If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

azuresdkci · 2016-07-28T23:55:14Z

Can one of the admins verify this patch?

hovsepm · 2016-07-29T00:11:54Z

@azuresdkci add to whitelist

hovsepm · 2016-07-29T00:13:29Z

src/ResourceManager/Cdn/AzureRM.Cdn.psd1

@@ -9,7 +9,7 @@
 @{  

 # Version number of this module.  
-ModuleVersion = '1.0.5' 


please revert this change. It is part of our process to update versions in each release.

hovsepm · 2016-07-29T00:17:48Z

revert version change.

Refers to: src/ResourceManager/Cdn/Commands.Cdn/Properties/AssemblyInfo.cs:30 in a669f26. [](commit_id = a669f26, deletion_comment = False)

CamSoper · 2016-07-29T01:59:38Z

Should be fixed now. Sorry about that, @hovsepm !

markcowl · 2016-07-29T18:43:45Z

@CamSoper Test failures, looks like some tests need to be re-recorded

CamSoper · 2016-07-29T22:50:32Z

@markcowl Light is green, trap is clean!

CamSoper · 2016-07-29T23:27:54Z

@kuangweizhang - @hovsepm needs you to bless this, please. :)

Thanks!

kuangweizhang · 2016-07-30T00:57:12Z

src/ResourceManager/Cdn/Commands.Cdn/CustomDomain/NewAzureRmCdnCustomDomain.cs


+            var existingCustomDomain = CdnManagementClient.CustomDomains.ListByEndpoint(EndpointName, ProfileName, ResourceGroupName).Where(cd => cd.Name == CustomDomainName).FirstOrDefault();


I think we should ignore case when compare the custom domain name

Same comment for other places

kuangweizhang · 2016-07-30T01:02:53Z

Just some comments on comparing the name of the resources, otherwise, LGTM

CamSoper · 2016-07-30T05:02:20Z

Thanks, @kuangweizhang!

Implemented the changes as suggested. Should be good to merge, I think.

markcowl · 2016-08-01T17:59:51Z

src/ResourceManager/Cdn/Commands.Cdn.Test/ScenarioTests/CustomDomainTests.ps1

-    $endpointName = "sdktest-cee91bb1-996a-44f1-96e6-dceaa6707def"
-    $hostName = "sdktest-3acbafc1-3f50-4fa4-9132-6d8c944d67e9.azureedge-test.net"    
-    $customDomainName = getAssetName
+    $endpointName = "sdktest-b0939e74-75ba-4558-afe6-edc5c19ea713" 


The best practice is to make setup part of test execution, so that these values do not have to be updated whenever the test is re-recorded. Please file an issue to do setup as part of test execution

I get what you're saying, and believe me, I wanted to do this differently too, however the two CustomDomain tests rely on a non-injectable outside resource - a DNS CNAME that must be setup manually on DNS. That's why the original authors ( @kuangweizhang ? Qinyuan? ) did it this way.

markcowl · 2016-08-01T18:08:56Z

src/ResourceManager/Cdn/Commands.Cdn/CustomDomain/GetAzureRmCdnCustomDomain.cs

@@ -18,38 +18,61 @@
 using Microsoft.Azure.Commands.Cdn.Models.CustomDomain;
 using Microsoft.Azure.Commands.Cdn.Properties;
 using Microsoft.Azure.Management.Cdn;
+using System.Linq;
+using Microsoft.Azure.Commands.Cdn.Models.Endpoint;

 namespace Microsoft.Azure.Commands.Cdn.CustomDomain
 {
    [Cmdlet(VerbsCommon.Get, "AzureRmCdnCustomDomain"), OutputType(typeof(PSCustomDomain))]
    public class GetAzureRmCdnCustomDomain : AzureCdnCmdletBase


For all cmdlets where you are adding a parameter set - you need to have a DefaultParameterSetName promprty in your Cmdlet attribute. And to avoid breaks, this should match the parameter set name containign the previous cmdlet parameters

markcowl · 2016-08-01T18:29:04Z

@CamSoper Some comments, three things that apply across the board:

(1) Your remove cmdlets must not use the pattern Force || ShouldProcess, this prevents ConfirmPreference and WhatIf parameters from working correctly. The best option would be to remove the ConfirmImpact.High and simply use ShouldProcess to determine whether a prompt occurs. This doesn't show a prompt unless the user asks for it bu lowering ConfirmPreference or specifying -Confirm. A less good option would be to remove ConfirmImpact.High and use

If (ShouldProcess(targe, message)) {
   if (Force || ShouldContinue(shouldContinuePrompt)) {
   ... cmdlet execution
  }
}

This acts as above, except that, in addition, the ShouldContinue prompt always occurs unless the user specifies - Force.
An even less good option would be to use only ShouldProcess with ConfirmImpact.High. This requires users to specify -Conforim:$false to avoid prompting. You can see: https://gist.github.com/markcowl/338e16fe5c8bbf195aff9f8af0db585d for a more detailed dscussion.

(2) For any cmdlet where you have added a parameter set, the DefaultParameterSetName property og the Cmdlet attribute should be set to the old parameter set, this avoids breaking changes (and you should always have a default parameter set if there is more thna one)

(3) For any cmdlet you ar5e changing that makes a change or has side effects, I have asked for supporting the SHouldProcess pattern - this allows users to execute the cmdlet with -WhatIf to determine what it would do if executed.

…f profile with existing endpoints.

Adjusted tests for better code coverage

markcowl · 2016-08-02T05:34:54Z

on demand run here:http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1058/

LGTM once the tests pass.

CamSoper added 7 commits July 20, 2016 21:17

Multiple refactorings and inline doc corrections.

430f77a

Updates to inline docs, cmdlets, and XML help

c0945ef

Merge remote-tracking branch 'Azure/dev' into dev

6ec4ff5

Updated tests

b6b1185

Added comments to explain why I'm using my own domain.

60a6e78

merge

884daea

Fixed CustomDomainTests to use a domain that didn't belong to me pers…

a669f26

…onally. :)

azurecla added the cla-not-required label Jul 28, 2016

hovsepm reviewed Jul 29, 2016
View reviewed changes

Reverted version number, replaced tabs

d0b07a0

CamSoper added 3 commits July 29, 2016 16:05

Re-recorded tests. All passing.

1fcdbad

Forgot to include the new session records in the project. Duh.

c97f642

And also "Copy to Output Directory." Double duh.

3c1cc9d

kuangweizhang reviewed Jul 30, 2016
View reviewed changes

Linq string comparisons now being made against .ToLower()

a598341

markcowl reviewed Aug 1, 2016
View reviewed changes

CamSoper added 2 commits August 1, 2016 18:17

Implemented @markcowl 's feedback. Added new test to cover deletion o…

eddd849

…f profile with existing endpoints.

Fixed an issue with the pipeline in New-AzureRmCdnEndpoint

e94109e

Adjusted tests for better code coverage

markcowl merged commit 698277c into Azure:dev Aug 2, 2016


		var existingCustomDomain = CdnManagementClient.CustomDomains.ListByEndpoint(EndpointName, ProfileName, ResourceGroupName).Where(cd => cd.Name == CustomDomainName).FirstOrDefault();

CDN - PowerShell Functionality and Doc Improvements #2687

CDN - PowerShell Functionality and Doc Improvements #2687

Uh oh!

Conversation

CamSoper commented Jul 28, 2016

Uh oh!

azurecla commented Jul 28, 2016

Uh oh!

azuresdkci commented Jul 28, 2016

Uh oh!

hovsepm commented Jul 29, 2016

Uh oh!

hovsepm Jul 29, 2016

Choose a reason for hiding this comment

Uh oh!

hovsepm commented Jul 29, 2016

Uh oh!

CamSoper commented Jul 29, 2016

Uh oh!

markcowl commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CamSoper commented Jul 29, 2016

Uh oh!

CamSoper commented Jul 29, 2016

Uh oh!

kuangweizhang Jul 30, 2016

Choose a reason for hiding this comment

Uh oh!

kuangweizhang Jul 30, 2016

Choose a reason for hiding this comment

Uh oh!

kuangweizhang commented Jul 30, 2016

Uh oh!

CamSoper commented Jul 30, 2016

Uh oh!

markcowl Aug 1, 2016

Choose a reason for hiding this comment

Uh oh!

CamSoper Aug 1, 2016

Choose a reason for hiding this comment

Uh oh!

markcowl Aug 1, 2016 • 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!

markcowl commented Aug 1, 2016

Uh oh!

markcowl commented Aug 2, 2016

Uh oh!

Uh oh!

markcowl commented Jul 29, 2016 •

edited

Loading

markcowl Aug 1, 2016 •

edited

Loading