Relay PS Draft #3789

v-Ajnava · 2017-04-18T01:07:07Z

Description

Relay Power Shell First draft.

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.

azurecla · 2017-04-18T02:14:48Z

@v-Ajnava, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, AZPRBOT;

cormacpayne · 2017-04-21T00:16:03Z

@v-Ajnava applying the needs-revision label until feedback from design review is resolved

markcowl

The comments from the review have only been partially applied. Specific comments inline for the first set of cmdlets - most comments apply to all cmdlets.

markcowl · 2017-04-30T23:20:06Z

src/ResourceManager/Relay/AzureRM.Relay.psd1

+# ProcessorArchitecture = ''
+
+# Modules that must be imported into the global environment prior to importing this module
+RequiredModules = @(@{ModuleName = 'AzureRM.Profile'; ModuleVersion = '2.7.0'; })


This should match the current version (2.8.0)

markcowl · 2017-04-30T23:20:37Z

src/ResourceManager/Relay/AzureRM.Relay.psd1

+    PSData = @{
+
+        # Tags applied to this module. These help with module discovery in online galleries.
+        Tags = 'Azure','ResourceManager','ARM','Relay'


How about ServiceBus as a Tag?

markcowl · 2017-04-30T23:21:16Z

src/ResourceManager/Relay/ChangeLog.md

+-->
+## Current Release
+
+## Version 0.0.1


The only heading in the file should be for 'Current Release' - the file will be updated after the release.

So please remove the heading for version 0.0.1

markcowl · 2017-04-30T23:22:45Z

src/ResourceManager/Relay/ChangeLog.md

+## Version 0.0.1
+* Adds commandlets for the Azure Relay
+
+    - New-AzureRmRelayNamespace


This is probably too much info. Simply adding a list of cdlets and a summary of what, as a group, the cmdlets allow you to do should be sufficient.

markcowl · 2017-04-30T23:23:34Z

src/ResourceManager/Relay/Commands.Relay.Test/Commands.Relay.Test.csproj

+    <Prefer32Bit>false</Prefer32Bit>
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+    <SignAssembly>true</SignAssembly>


You don't need to sign the test assembly, please don't

markcowl · 2017-05-01T00:26:57Z

...anager/Relay/Commands.Relay/Cmdlets/Namespace/RemoveAzureRelayNamespaceAuthorizationRules.cs

+    /// 'Remove-AzureRmRelayNamespaceAuthorizationRule' Cmdlet deletes specified Relay Namespace AuthorizationRule
+    /// </summary>
+    [Cmdlet(VerbsCommon.Remove, RelayNamespaceAuthorizationRuleVerb, SupportsShouldProcess = true), OutputType(typeof(bool))]
+    public class RemoveAzureRmRelayNamespaceAuthorizationRule : AzureRelayCmdletBase


Same comment - authorization rule cmdlets should be centralized

this file is excluded. missed to delete the file. deleting this file now.

markcowl · 2017-05-01T00:27:38Z

src/ResourceManager/Relay/Commands.Relay/Cmdlets/Namespace/SetAzureRelayNamespace.cs

+           ValueFromPipelineByPropertyName = true,
+           HelpMessage = "Relay Namespace object.")]
+        [ValidateNotNullOrEmpty]
+        public RelayNamespaceAttirbutesUpdateParameter InputObject { get; set; }


ValueFromPipeline, as noted previously

markcowl · 2017-05-01T00:27:56Z

...ceManager/Relay/Commands.Relay/Cmdlets/Namespace/SetAzureRelayNamespaceAuthorizationRules.cs

+    /// 'Set-AzureRmRelayNamespaceAuthorizationRule' Cmdlet updates specified Relay Namespace AuthorizationRule
+    /// </summary>
+    [Cmdlet(VerbsCommon.Set, RelayNamespaceAuthorizationRuleVerb, SupportsShouldProcess = true), OutputType(typeof(AuthorizationRuleAttributes))]
+    public class SetAzureRelayNamespaceAuthorizationRule : AzureRelayCmdletBase


Same comment about authorization rule cmdlets

I excluded individual authorization rules cmdlets but missed to delete these files. Deleting it now.

markcowl · 2017-05-01T00:30:33Z

src/ResourceManager/Relay/Commands.Relay/Cmdlets/WcfRelay/GetAzureWcfRelayAuthorizationRules.cs

+    /// <para> If AuthorizationRule name provided, a single AuthorizationRule detials will be returned</para>
+    /// <para> If AuthorizationRule name not provided, list of AuthorizationRules will be returned</para>
+    /// </summary>
+    [Cmdlet(VerbsCommon.Get, RelayWcfRelayAuthorizationRuleVerb), OutputType(typeof(List<AuthorizationRuleAttributes>))]


same comments as for all authorization rule cmdlets. Also, comments from HybridConnection relays apply ato all cmdlets here.

I excluded individual authorization rules cmdlets but missed to delete these files. Deleting it now.

markcowl · 2017-05-01T00:31:11Z

src/ResourceManager/Relay/Commands.Relay/Cmdlets/WcfRelay/NewAzureWcfRelayKey.cs

+            Position = 1,
+            HelpMessage = "Namespace Name.")]
+        [ValidateNotNullOrEmpty]
+        public string NamespaceName { get; set; }


It looks like you neglected to apply the review commenbts here

My apologies, I excluded individual authorization rules cmdlets but missed to delete these files. Deleting it now.

cormacpayne · 2017-05-01T23:12:30Z

On-demand: http://azuresdkci.cloudapp.net/job/powershell-demand/1507/

markcowl

A couple of nits, which you could clean up in a subsequent PR

markcowl · 2017-05-02T06:33:04Z

src/ResourceManager/Relay/Commands.Relay/Cmdlets/AuthorizationRule/NewAzureRelayKey.cs

+            {
+                if (ShouldProcess(target: RegenerateKey, action: string.Format(Resources.RegenerateKeyHybirdconnection, Name, HybridConnection)))
+                {
+                    WriteObject(Client.HybridConnectionsRegenerateKeys(ResourceGroupName, Namespace, HybridConnection, Name, RegenerateKey));


Should enumerate the keys if this is a collection (for each of these branches), bu using WriteObject(collection, true)

markcowl · 2017-05-02T06:35:05Z

...er/Relay/Commands.Relay/Cmdlets/CheckNameAvailability/TestAzureRelayCheckNameAvailability.cs

@@ -0,0 +1,43 @@
+// ----------------------------------------------------------------------------------


<nit>For .Net coding Guildines compatibility, you should change the name fo the file to match the name of the class </nit>

markcowl · 2017-05-02T06:38:35Z

tools/StaticAnalysis/Exceptions/SignatureIssues.csv

@@ -1453,3 +1453,18 @@
 "Microsoft.Azure.Commands.Network.dll","Microsoft.Azure.Commands.Network.NewAzureRmIpsecPolicyCommand","New-AzureRmIpsecPolicy","1","8410","Parameter SADataSizeKilobytes of cmdlet New-AzureRmIpsecPolicy does not follow the enforced naming convention of using a singular noun for a parameter name.","Consider using a singular noun for the parameter name."
 "Microsoft.Azure.Commands.Network.dll","Microsoft.Azure.Commands.Network.NewAzureVirtualNetworkGatewayConnectionCommand","New-AzureRmVirtualNetworkGatewayConnection","1","8410","Parameter UsePolicyBasedTrafficSelectors of cmdlet New-AzureRmVirtualNetworkGatewayConnection does not follow the enforced naming convention of using a singular noun for a parameter name.","Consider using a singular noun for the parameter name."
 "Microsoft.Azure.Commands.Network.dll","Microsoft.Azure.Commands.Network.NewAzureVirtualNetworkGatewayConnectionCommand","New-AzureRmVirtualNetworkGatewayConnection","1","8410","Parameter IpsecPolicies of cmdlet New-AzureRmVirtualNetworkGatewayConnection does not follow the enforced naming convention of using a singular noun for a parameter name.","Consider using a singular noun for the parameter name."
+"Microsoft.Azure.Commands.AnalysisServices.Dataplane.dll","Microsoft.Azure.Commands.AnalysisServices.Dataplane.RestartAzureAnalysisServer","Restart-AzureAnalysisServicesInstance","2","8210","Restart-AzureAnalysisServicesInstance does not have a Force parameter but the cmdlet verb 'Restart' indicates that it may perform destructive actions under certain circumstances. Consider whether the cmdlet should have a Force parameter anduse ShouldContinue under some circumstances. ","Consider wether the cmdlet should have a Force parameter and use ShouldContinue under some circumstances. "


You don't need to set suppressions for severity 2 signature issues - these are just wranings. Sev 0 and Sev 1 need to be suppressed, if valid.

markcowl · 2017-05-02T07:04:17Z

signing run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/1005/

msftclas added the cla-required label Apr 18, 2017

azuresdkci assigned markcowl Apr 18, 2017

azurecla added cla-signed and removed cla-required labels Apr 18, 2017

cormacpayne added the needs-revision label Apr 21, 2017

cormacpayne assigned v-Ajnava and unassigned markcowl Apr 21, 2017

cormacpayne added needs-review and removed needs-revision labels Apr 27, 2017

cormacpayne assigned cormacpayne and unassigned v-Ajnava Apr 27, 2017

Relay PS design review updated

a021621

v-Ajnava force-pushed the Relaypower branch from d1d7e02 to a021621 Compare April 29, 2017 17:57

markcowl requested changes May 1, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels May 1, 2017

markcowl assigned v-Ajnava and unassigned cormacpayne May 1, 2017

Ajit Navasare (MINDTREE LIMITED) added 2 commits May 1, 2017 13:11

PR review comments updated

9764f6e

Reivew comments updated

0851fcd

v-Ajnava force-pushed the Relaypower branch from 55f2d97 to 0851fcd Compare May 1, 2017 22:35

cormacpayne added needs-review and removed needs-revision labels May 1, 2017

cormacpayne assigned markcowl and unassigned v-Ajnava May 1, 2017

markcowl approved these changes May 2, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels May 2, 2017

azuresdkci added the needs-review label May 2, 2017

cormacpayne removed the needs-review label May 2, 2017

markcowl merged commit a22c522 into Azure:preview May 2, 2017

v-Ajnava deleted the Relaypower branch April 12, 2018 01:17

		@@ -0,0 +1,43 @@
		// ----------------------------------------------------------------------------------

Relay PS Draft #3789

Relay PS Draft #3789

Uh oh!

Conversation

v-Ajnava commented Apr 18, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azurecla commented Apr 18, 2017

Uh oh!

cormacpayne commented Apr 21, 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!

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!

cormacpayne commented May 1, 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!

markcowl commented May 2, 2017 • edited by cormacpayne Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

v-Ajnava commented Apr 18, 2017 •

edited by azuresdkci

Loading

markcowl commented May 2, 2017 •

edited by cormacpayne

Loading