Routing support for IotHub #3998

ssankar1984 · 2017-05-18T18:49:53Z

Description

Routing support for IotHub

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.

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

General Guidelines

[ Y] Title of the pull request is clear and informative.
[ Y] 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.
[ Y] The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

[ Y] Pull request includes test coverage for the included changes.
[ Y] 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

[ Y] 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.
[ Y] Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

[ Y] Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
[Y ] 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.
[ Y] Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

msftclas · 2017-05-18T18:49:56Z

@ssankar1984,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-05-18T18:50:06Z

Can one of the admins verify this patch?

cormacpayne · 2017-05-18T20:14:49Z

@azuresdkci add to whitelist

ssankar1984 · 2017-05-23T18:27:34Z

resolved conflict

markcowl

Since this involves changes to your cmdlet surface, you must send us the changes to the public interface for review before this cna be accepted (md file changes suffice).

I don't understand the various routing properties and why there are so many of them, please send an explanation, come to an office hours, or set up a review time.

Please include a description of changes in your changelog.

markcowl · 2017-05-23T19:30:12Z

src/Common/Commands.Common.Authentication/Commands.Common.Authentication.csproj

-    <Reference Include="Newtonsoft.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">
-      <HintPath>..\..\packages\Newtonsoft.Json.6.0.8\lib\net45\Newtonsoft.Json.dll</HintPath>
-      <Private>True</Private>
+    <Reference Include="Newtonsoft.Json, Version=9.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">


markcowl · 2017-05-23T19:30:19Z

src/Common/Commands.Common.Authentication/Commands.Common.Authentication.csproj

@@ -156,6 +153,7 @@
    </EmbeddedResource>
  </ItemGroup>
  <ItemGroup>
+    <None Include="app.config" />


markcowl · 2017-05-23T19:30:33Z

src/Common/Commands.Common.Authentication/packages.config

-  <package id="Newtonsoft.Json" version="6.0.8" targetFramework="net45" />
-</packages>
+  <package id="Microsoft.Rest.ClientRuntime.Azure.Authentication" version="2.3.0" targetFramework="net452" />
+  <package id="Newtonsoft.Json" version="9.0.1" targetFramework="net452" />


markcowl · 2017-05-23T19:30:41Z

....Common.Authentication.ResourceManager/Commands.Common.Authentication.ResourceManager.csproj

@@ -85,6 +85,7 @@
    <Compile Include="Serialization\ModelConversionExtensions.cs" />
  </ItemGroup>
  <ItemGroup>
+    <None Include="app.config" />


markcowl · 2017-05-23T19:30:56Z

...ds.ScenarioTests.ResourceManager.Common/Commands.ScenarioTests.ResourceManager.Common.csproj

-    <Reference Include="Newtonsoft.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">
-      <SpecificVersion>False</SpecificVersion>
-      <HintPath>..\..\..\packages\Newtonsoft.Json.6.0.8\lib\net45\Newtonsoft.Json.dll</HintPath>
+    <Reference Include="Newtonsoft.Json, Version=9.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">


markcowl · 2017-05-23T19:38:58Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/SetAzureRmIotHub.cs

+            Mandatory = false,
+            HelpMessage = "Routes")]
+        [ValidateNotNullOrEmpty]
+        public List<PSRouteProperties> Routes { get; set; }


What is the difference between RouteProperties and routing properties? You shoiuld just take an array of the type rather than a list. Also, parameter name should be singular (Route)

markcowl · 2017-05-23T19:39:37Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/SetAzureRmIotHub.cs

+            Mandatory = false,
+            HelpMessage = "Fallback Routes")]
+        [ValidateNotNullOrEmpty]
+        public PSFallbackRouteProperties FallbackRoute { get; set; }


Is it really necessary to have 3 different types for these 3 kinds of route properties?

We should rename the rout property types to make the distinction clear, the List<> type should be an array, and parameter names should be singular.

markcowl · 2017-05-23T19:40:55Z

src/ResourceManager/IotHub/Commands.IotHub/Models/PSRoutingServiceBusQueueEndpointProperties.cs

+    /// <summary>
+    /// The properties related to service bus queue endpoint types.
+    /// </summary>
+    public partial class PSRoutingServiceBusQueueEndpointProperties


What is the rationale for having a separate class for this? Isn't there a more generic Routing Proeprties that will do?

we need separate classes , discussed offline

markcowl · 2017-05-23T19:41:16Z

src/ResourceManager/IotHub/Commands.IotHub/Properties/AssemblyInfo.cs

@@ -32,5 +32,5 @@
 // You can specify all the values or you can default the Build and Revision Numbers 
 // by using the '*' as shown below:
 // [assembly: AssemblyVersion("1.0.*")]
-[assembly: AssemblyVersion("2.0.0")]
+[assembly: AssemblyVersion("2.0.1")]


Do not change

markcowl · 2017-05-23T19:41:48Z

src/ResourceManager/IotHub/Commands.IotHub/help/Add-AzureRmIotHubEventHubConsumerGroup.md

@@ -39,7 +38,7 @@ Adds a new consumergroup named "myconsumergroup" to the eventhub for operations
 ## PARAMETERS

 ### -EventHubConsumerGroupName
-Name of the EventHub ConsumerGroup that you want to add.
+EventHubConsumerGroupName.


Please use complete sentences in the help descriptions. This applies acrosss the board to all help files

…hell into preview # Conflicts: # src/ResourceManager/Profile/Commands.Profile/packages.config

…hell into preview

markcowl · 2017-05-26T16:32:30Z

src/ResourceManager/IotHub/Commands.IotHub.Test/Commands.IotHub.Test.csproj

@@ -146,6 +132,10 @@
      <Project>{5ee72c53-1720-4309-b54b-5fb79703195f}</Project>
      <Name>Commands.Common</Name>
    </ProjectReference>
+    <ProjectReference Include="..\..\Common\Commands.Common.Authentication.ResourceManager\Commands.Common.Authentication.ResourceManager.csproj">


You really should not need this reference

@ssankar1984 Not removed. Please push the changes to your branch.

markcowl · 2017-05-26T16:33:27Z

src/ResourceManager/IotHub/Commands.IotHub.Test/ScenarioTests/IotHubTests.ps1

@@ -37,8 +37,50 @@ function Test-AzureRmIotHubLifecycle
 	# Create or Update Resource Group
 	$resourceGroup = New-AzureRmResourceGroup -Name $ResourceGroupName -Location $Location 

+	Write-Debug " Create new eventHub " 
+	$namespaceName = "IotHubPSEHNamespaceTest"


You should not hard code names in the tests, you should generate names using test common code. This allows tests to be used against any subscription

This should run against all subs , I see this pattern followed all over the test , this helps us to delete resources if it was there before while running a new test and therefore not leave undeleted resources without usage in the subscription. For test common code , are you suggesting passing this param ?

I am suggesting generating the parameter based on a prefix, there are lots of examples of using the test library to do this.

markcowl · 2017-05-26T16:35:08Z

src/ResourceManager/IotHub/Commands.IotHub/Commands.IotHub.csproj

@@ -37,30 +37,24 @@
    <Reference Include="Hyak.Common">
      <HintPath>..\..\..\packages\Hyak.Common.1.0.3\lib\portable-net403+win+wpa81\Hyak.Common.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Azure.Management.EventHub, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Why do you add this depednency on EventHub? This creates a cross-depednency on EventHub managemrnt client between this module and the Evnethub module. Please remove this, or consider moving any necessary methods into common code.

good catch ! , removed

@ssankar1984 Not removed.

markcowl · 2017-05-26T16:35:23Z

src/ResourceManager/IotHub/Commands.IotHub/Commands.IotHub.csproj

-    </Reference>
-    <Reference Include="Microsoft.Threading.Tasks.Extensions.Desktop">
-      <HintPath>..\..\..\packages\Microsoft.Bcl.Async.1.0.168\lib\net40\Microsoft.Threading.Tasks.Extensions.Desktop.dll</HintPath>
+    <Reference Include="Microsoft.Rest.ClientRuntime.Azure.Authentication, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Remove this reference

Not removed.

markcowl · 2017-05-26T16:36:21Z

src/ResourceManager/IotHub/Commands.IotHub/Commands.IotHub.csproj

@@ -178,6 +179,10 @@
    </EmbeddedResource>
  </ItemGroup>
  <ItemGroup>
+    <Content Include="Microsoft.Azure.Commands.IotHub.dll-Help.xml">


Remove. Follow the directions here for creating help markdowns: https://github.com/Azure/azure-powershell/blob/preview/documentation/help-generation.md

markcowl · 2017-05-26T16:37:20Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/SetAzureRmIotHub.cs

@@ -97,7 +101,7 @@ public class SetAzureRmIotHub : IotHubBaseCmdlet
        [Parameter(
            ParameterSetName = UpdateFileUploadPropertiesParameterSet,
            Mandatory = false,
-            HelpMessage = "fileUploadNotificationMaxDeliveryCount")]
+            HelpMessage = "FileUploadNotificationMaxDeliveryCount")]


This is not a help message, you are merely copying the name of the parameter

markcowl · 2017-05-26T16:37:43Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/SetAzureRmIotHub.cs

+            Mandatory = false,
+            HelpMessage = "RoutingProperties")]
+        [ValidateNotNullOrEmpty]
+        public PSRoutingProperties RoutingProperties { get; set; }


We discussed this. These type names need to be disambiguated

I changed the type name for RouteProperties , are you suggesting changing this too ? So we have the following now ...
PSRoutingProperties
PSRouteMetadata
PSFallbackRouteMetadata
PSEndpoints
Please suggest if you want these to be changed...

markcowl · 2017-05-26T16:38:01Z

src/ResourceManager/IotHub/Commands.IotHub/Microsoft.Azure.Commands.IotHub.dll-help.xml

@@ -0,0 +1,2634 @@
+<?xml version="1.0" encoding="utf-8"?>


Remove this file

markcowl · 2017-05-26T16:39:29Z

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

@@ -8,7 +8,7 @@ Locale: en-US

 # AzureRM.IotHub Module
 ## Description
-Commands for Managing Azure IotHubs
+{{Manually Enter Description Here}}


Need a description

Still need to fix all the help descriptions to be meaningful

markcowl · 2017-05-30T18:03:44Z

src/Common/Commands.Common.Authentication/Commands.Common.Authentication.csproj

@@ -73,7 +73,7 @@
    </Reference>
    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\..\packages\Microsoft.Rest.ClientRuntime.2.3.5\lib\net45\Microsoft.Rest.ClientRuntime.dll</HintPath>
-      <Private>True</Private>


remove any changes to this file

The true is removed and added and therefore it shows as a diff

markcowl · 2017-05-30T18:03:56Z

src/ResourceManager/Common/Commands.ScenarioTests.ResourceManager.Common/packages.config

@@ -24,4 +24,4 @@
  <package id="xunit.extensibility.core" version="2.1.0" targetFramework="net45" />
  <package id="xunit.extensibility.execution" version="2.1.0" targetFramework="net45" />
  <package id="xunit.runner.visualstudio" version="2.1.0" targetFramework="net45" />
-</packages>


remove any changes to this file

markcowl · 2017-05-30T18:04:30Z

src/ResourceManager/IotHub/Commands.IotHub.Test/Commands.IotHub.Test.csproj

@@ -146,6 +132,10 @@
      <Project>{5ee72c53-1720-4309-b54b-5fb79703195f}</Project>
      <Name>Commands.Common</Name>
    </ProjectReference>
+    <ProjectReference Include="..\..\Common\Commands.Common.Authentication.ResourceManager\Commands.Common.Authentication.ResourceManager.csproj">


@ssankar1984 Not removed. Please push the changes to your branch.

markcowl · 2017-05-30T18:05:21Z

src/ResourceManager/IotHub/Commands.IotHub/Commands.IotHub.csproj

@@ -37,30 +37,24 @@
    <Reference Include="Hyak.Common">
      <HintPath>..\..\..\packages\Hyak.Common.1.0.3\lib\portable-net403+win+wpa81\Hyak.Common.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Azure.Management.EventHub, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


@ssankar1984 Not removed.

markcowl · 2017-05-30T18:05:34Z

src/ResourceManager/IotHub/Commands.IotHub/Commands.IotHub.csproj

-    </Reference>
-    <Reference Include="Microsoft.Threading.Tasks.Extensions.Desktop">
-      <HintPath>..\..\..\packages\Microsoft.Bcl.Async.1.0.168\lib\net40\Microsoft.Threading.Tasks.Extensions.Desktop.dll</HintPath>
+    <Reference Include="Microsoft.Rest.ClientRuntime.Azure.Authentication, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Not removed.

markcowl · 2017-05-30T18:07:04Z

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

@@ -8,7 +8,7 @@ Locale: en-US

 # AzureRM.IotHub Module
 ## Description
-Commands for Managing Azure IotHubs
+{{Manually Enter Description Here}}


Still need to fix all the help descriptions to be meaningful

…hell into preview

markcowl · 2017-06-02T23:00:05Z

src/ResourceManager/IotHub/Commands.IotHub.Test/ScenarioTests/IotHubTests.ps1

@@ -37,8 +37,50 @@ function Test-AzureRmIotHubLifecycle
 	# Create or Update Resource Group
 	$resourceGroup = New-AzureRmResourceGroup -Name $ResourceGroupName -Location $Location 

+	Write-Debug " Create new eventHub " 
+	$namespaceName = "IotHubPSEHNamespaceTest"


I am suggesting generating the parameter based on a prefix, there are lots of examples of using the test library to do this.

markcowl · 2017-06-02T23:17:13Z

src/ResourceManager/IotHub/Commands.IotHub/packages.config

-  <package id="Microsoft.Rest.ClientRuntime" version="2.3.5" targetFramework="net452" />
-  <package id="Microsoft.Rest.ClientRuntime.Azure" version="3.3.5" targetFramework="net452" />
-  <package id="Newtonsoft.Json" version="6.0.8" targetFramework="net452" />
+  <package id="Microsoft.IdentityModel.Clients.ActiveDirectory" version="2.28.3" targetFramework="net452" />


markcowl · 2017-06-02T23:17:23Z

src/ResourceManager/IotHub/Commands.IotHub/packages.config

+  <package id="Microsoft.IdentityModel.Clients.ActiveDirectory" version="2.28.3" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.8" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime.Azure" version="3.3.7" targetFramework="net452" />
+  <package id="Newtonsoft.Json" version="9.0.1" targetFramework="net452" />


markcowl · 2017-06-02T23:17:38Z

src/ResourceManager/Profile/Commands.Profile/packages.config

@@ -9,9 +9,9 @@
  <package id="Microsoft.Bcl.Build" version="1.0.14" targetFramework="net45" />
  <package id="Microsoft.IdentityModel.Clients.ActiveDirectory" version="2.28.3" targetFramework="net45" />
  <package id="Microsoft.Net.Http" version="2.2.28" targetFramework="net45" />
-  <package id="Microsoft.Rest.ClientRuntime" version="2.3.5" targetFramework="net45" />


Do not change this file

markcowl · 2017-06-02T23:20:32Z

src/ResourceManager/IotHub/Commands.IotHub/help/Add-AzureRmIotHubKey.md

@@ -61,22 +63,22 @@ Accept wildcard characters: False
 ```

 ### -PrimaryKey
-The Primary Key.
+PrimaryKey


The Primary Key

…hell into preview

markcowl · 2017-06-03T23:55:47Z

src/Common/Commands.Common.Authentication/Commands.Common.Authentication.csproj

@@ -156,7 +156,9 @@
    </EmbeddedResource>
  </ItemGroup>
  <ItemGroup>
-    <None Include="packages.config" />
+    <None Include="packages.config">


You need to revert this change

markcowl · 2017-06-04T06:36:55Z

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

markcowl · 2017-06-05T00:38:17Z

will close in favor of #4075

Routing support for IotHub

66093c2

msftclas added the cla-not-required label May 18, 2017

azuresdkci added the needs-review label May 18, 2017

azuresdkci assigned shahabhijeet May 18, 2017

Sankar Sundaram and others added 2 commits May 22, 2017 16:52

fix version

68e163a

Merge branch 'preview' into preview

29880bd

markcowl unassigned shahabhijeet May 23, 2017

markcowl removed the needs-review label May 23, 2017

markcowl assigned ssankar1984 May 23, 2017

markcowl requested changes May 23, 2017

View reviewed changes

Sankar Sundaram added 4 commits May 23, 2017 17:55

fix comments

95d7e7a

Merge branch 'preview' of https://github.com/ssankar1984/azure-powers…

e797936

…hell into preview # Conflicts: # src/ResourceManager/Profile/Commands.Profile/packages.config

fixes based on comments

b4a856f

fixes based on comments

9de53f8

markcowl added the needs-revision label May 24, 2017

Sankar Sundaram added 6 commits May 23, 2017 18:34

fix comments

86d9eae

fix comments

6276780

fix comments

4d46b2b

Merge branch 'preview' of https://github.com/ssankar1984/azure-powers…

2422a3f

…hell into preview

include new package

058c7d3

fix comments

64b32ea

markcowl requested changes May 26, 2017

View reviewed changes

markcowl requested changes May 30, 2017

View reviewed changes

Sankar Sundaram added 3 commits May 31, 2017 10:05

fix comments

e5d1423

fix comments

c787d26

fix comments

40819dc

Merge branch 'preview' of https://github.com/ssankar1984/azure-powers…

1793757

…hell into preview

cormacpayne removed the needs-revision label Jun 1, 2017

cormacpayne assigned markcowl and unassigned ssankar1984 Jun 1, 2017

cormacpayne added the needs-review label Jun 1, 2017

markcowl requested changes Jun 2, 2017

View reviewed changes

Sankar Sundaram and others added 4 commits June 2, 2017 17:13

Fix comments

37cdafd

Merge branch 'preview' of https://github.com/ssankar1984/azure-powers…

3b84036

…hell into preview

fix

7d661d5

Update Add-AzureRmIotHubKey.md

173a9fb

markcowl requested changes Jun 4, 2017

View reviewed changes

markcowl added 2 commits June 3, 2017 23:33

revert changes to common.authentication

fa28749

Revert changes to common.authentication

8f75e6f

markcowl approved these changes Jun 4, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Jun 4, 2017

markcowl changed the base branch from preview to release-4.1.0 June 4, 2017 23:41

markcowl closed this Jun 5, 2017

Routing support for IotHub #3998

Routing support for IotHub #3998

Uh oh!

Conversation

ssankar1984 commented May 18, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

msftclas commented May 18, 2017

Uh oh!

azuresdkci commented May 18, 2017

Uh oh!

cormacpayne commented May 18, 2017

Uh oh!

ssankar1984 commented May 23, 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!

markcowl May 26, 2017 • 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!

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

ssankar1984 commented May 18, 2017 •

edited by azuresdkci

Loading

markcowl May 26, 2017 •

edited

Loading