Iot Hub Commandlets #3250

rkmanda · 2016-12-05T22:01:54Z

Description

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 PassThrough 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 · 2016-12-05T22:02:03Z

Hi @rkmanda, 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 (rmanda). 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;

rkmanda · 2016-12-05T22:08:19Z

This is the next iteration after the older PR for the same was closed. Link to PR #3156

markcowl · 2016-12-06T19:37:19Z

@rkmanda Please clean up the commits in this PR as described here:https://github.com/Azure/azure-powershell/blob/dev/documentation/cleaning-up-commits.md

Please fill out the checklist as required as well

cormacpayne

@rkmanda I left a few comments that need to be addressed

Also, you will need to add a ChangeLog.md file to your folder (view here for an example), as well as a documentation folder containing breaking change documents (view here for an example)

cormacpayne · 2016-12-06T19:51:59Z

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

+// by using the '*' as shown below:
+// [assembly: AssemblyVersion("1.0.*")]
+[assembly: AssemblyVersion("0.0.1")]
+[assembly: AssemblyFileVersion("1.0.0.0")]


@rkmanda this should also be 0.0.1

cormacpayne · 2016-12-06T19:55:02Z

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

+	$op= $iotHubUpdated.Properties.OperationsMonitoringProperties
+	$op.OperationMonitoringEvents["Connections"] = "Information"
+	$iotHubUpdated = Set-AzureRmIotHub -ResourceGroupName $ResourceGroupName -Name $IotHubName -OperationsMonitoringProperties $op
+	Assert-True { $iothubUpdated.Properties.OperationsMonitoringProperties.OperationMonitoringEvents["Connections"] -eq "Information" }


@rkmanda do you plan on adding a test using the parameter set UpdateFileUploadProperties for Set-AzureRmIotHub?

Not as a part of the current PR. The test for this is a little involved as it requires a SAS token to be fetched on the fly. Will add the test later.

cormacpayne · 2016-12-06T19:57:54Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/GetAzureRmIotHubJob.cs

+
+    [Cmdlet(VerbsCommon.Get, "AzureRmIotHubJob")]
+    [OutputType(typeof(PSIotHubJobResponse), typeof(List<PSIotHubJobResponse>))]
+    public class GetAzureRmIotHubJob : IotHubBaseCmdlet


@rkmanda I don't see any tests for this cmdlet

The prereq for testing this is to have a test for NewAzureRmIotHubExportDevices which also relies on a valid SAS token to be generated on the fly. Will target this later.

cormacpayne · 2016-12-06T19:59:26Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/NewAzureRmIotHubExportDevices.cs

+
+    [Cmdlet(VerbsCommon.New, "AzureRmIotHubExportDevices", SupportsShouldProcess = true)]
+    [OutputType(typeof(JobResponse))]
+    public class NewAzureRmIotHubExportDevices : IotHubBaseCmdlet


@rkmanda I don't see any tests for this cmdlet

Relies on a valid SAS token to be generated on the fly. Will target this later.

cormacpayne · 2016-12-06T19:59:58Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/NewAzureRmIotHubImportDevices.cs

+
+    [Cmdlet(VerbsCommon.New, "AzureRmIotHubImportDevices", SupportsShouldProcess = true)]
+    [OutputType(typeof(JobResponse))]
+    public class NewAzureRmIotHubImportDevices : IotHubBaseCmdlet


@rkmanda I don't see any tests for this cmdlet

Relies on a valid SAS token to be generated on the fly. Will target this later.

markcowl · 2016-12-06T19:39:17Z

src/ResourceManager/IotHub/AzureRM.IotHub.psd1

+
+
+# Assemblies that must be loaded prior to importing this module  
+RequiredAssemblies = @()  


Need to have The correct version of AzureRM.Profile as a dependency, see: https://github.com/Azure/azure-powershell/blob/dev/src/ResourceManager/Cdn/AzureRM.Cdn.psd1#L54

markcowl · 2016-12-06T19:39:58Z

src/ResourceManager/IotHub/AzureRM.IotHub.psd1

+    PSData = @{
+
+        # Tags applied to this module. These help with module discovery in online galleries.
+        # Tags = @()


Please uncomment tags property and put in appropriate tags

markcowl · 2016-12-06T19:40:31Z

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

+    <PlatformTarget>AnyCPU</PlatformTarget>
+    <DebugType>pdbonly</DebugType>
+    <Optimize>true</Optimize>
+    <OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.IotHub</OutputPath>


You don't want this path for tests

markcowl · 2016-12-06T19:43:44Z

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

+        public void TestAzureIotHubLifeCycle()
+        {
+            IotHubController.NewInstance.RunPsTestWorkflow(
+                () => { return new[] { string.Format("{0} {1} {2} {3} {4}", "Test-AzureRmIotHubLifecycle", "northeurope", "powershelliothub", "powershellrg", "S1") }; },


Do not hard code regions. Instead, your test should detect an appropriate available region as part of test setup. Hard coded regions are a maintenance headache, because the test cannot be re-recorded against the test cluster in the future.

Found other resources hardcoding location directly in the script. What I have done is to ensure that the script itself does not have the hardcoding so it can be run in any location. Let me know if this works for the current PR. I will raise an issue to fix this in a subsequent PR.

markcowl · 2016-12-06T19:45:37Z

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

+    <OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.IotHub</OutputPath>
+    <DefineConstants>TRACE</DefineConstants>
+    <ErrorReport>prompt</ErrorReport>
+    <WarningLevel>4</WarningLevel>


You need the properties that support delay siging in this target. See: https://github.com/Azure/azure-powershell/blob/dev/src/ResourceManager/Cdn/Commands.Cdn/Commands.Cdn.csproj#L43 for an example. You will also need to include the delay sign key, as that assembly does

Why doi you have two property groups for the release configuration? Please clean up.

markcowl · 2016-12-06T21:15:44Z

src/ResourceManager/IotHub/Commands.IotHub/IotHub/RemoveAzureRmIotHubKey.cs

+            Position = 2,
+            Mandatory = true,
+            HelpMessage = "Name of the Key")]
+        [ValidateNotNullOrEmpty]


Any chance this will come from the pipeline (A Get call, perhaps?)

markcowl · 2016-12-06T21:16:11Z

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

+    using Microsoft.Azure.Management.IotHub;
+    using Microsoft.Azure.Management.IotHub.Models;
+
+    [Cmdlet(VerbsCommon.Set, "AzureRmIotHub", SupportsShouldProcess = true)]


Need a default parameter set setting

markcowl · 2016-12-06T21:19:56Z

src/ResourceManager/IotHub/Commands.IotHub/ResourceManagerStartup.ps1

@@ -0,0 +1,276 @@
+# ----------------------------------------------------------------------------------


What is this file intended for? Remove

markcowl · 2016-12-06T21:22:32Z

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

+## PARAMETERS
+
+### -EventHubConsumerGroupName
+EventHubConsumerGroupName.


Informative description, please

markcowl · 2016-12-06T21:26:16Z

src/ResourceManager/IotHub/Commands.IotHub/help/Set-AzureRmIotHub.md

+
+### -CloudToDevice
+CloudToDevice
+


Need an informative description for each parameter. This applies to all help files

rkmanda · 2016-12-08T09:17:36Z

@markcowl please review. Addressed most comments. Some tests are to be added and I will raise an issue for this. The region hardcoding will also be removed as a part of the test issue.

markcowl

You need to file test issues for:

missing coverage
hard-coded locations
cleanup of duplicated scripts

Send us the link for these issues and we will be able to merge the PR once it passes test and signing builds

markcowl · 2016-12-08T18:09:10Z

src/ResourceManager/IotHub/ChangeLog.md

+## Current Release
+
+## Version 1.0.0
+* Adds commandlets for the Azure IoT Hub 


Please listy the capabilities of these cmdlets, and the resources they allow you to manage.

markcowl · 2016-12-08T18:10:40Z

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

+    <OutputPath>..\..\..\Package\Release\ResourceManager\AzureResourceManager\AzureRM.IotHub</OutputPath>
+    <DefineConstants>TRACE</DefineConstants>
+    <ErrorReport>prompt</ErrorReport>
+    <WarningLevel>4</WarningLevel>


Why doi you have two property groups for the release configuration? Please clean up.

markcowl · 2016-12-08T18:16:45Z

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

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

cormacpayne · 2016-12-09T18:35:13Z

Sign job was successful: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/755/

cormacpayne · 2016-12-10T00:22:26Z

On demand: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1346/

Sign job: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-sign/760/

LGTM once both pass

rkmanda · 2016-12-11T17:44:35Z

Can this be merged now?

azurecla added the cla-not-required label Dec 5, 2016

markcowl added the Changes Requested label Dec 6, 2016

cormacpayne reviewed Dec 6, 2016

View reviewed changes

markcowl requested changes Dec 6, 2016

View reviewed changes

rkmanda added 21 commits December 7, 2016 15:04

Initial commit

52a7c72

Initial commit

4e92c66

Adding Update IotHub Commandlet

876e06a

Adding Test Framework

7ffafc4

Added Tests and SessionRecords

fde18a1

removing unused files

e91b902

Generating Help

b30533b

Adding Confirmation Messages for Remove Operations

f3e584e

Updating help content

cfb8858

installer changes

8a9d2ce

Fixing tests

6470c45

Updating help content

986f18d

fixing installer

3005d57

Address Review Comments

e7be1ad

fix build break

f4ac364

Fixing build issues

af7d3da

Fixing travis break

369aaed

fixing travis build break

50324ad

Fixing credscan issue

9863004

Addresing comments. Reving versions.

8a2cc34

Regenerate documentation

29594b4

rkmanda force-pushed the dev branch from b348795 to 29594b4 Compare December 7, 2016 23:21

Fixing build break

fb0410c

Fixis cred scan issue

5510783

markcowl requested changes Dec 8, 2016

View reviewed changes

Updating the changelog

45688ca

cormacpayne changed the base branch from dev to release-3.3.0 December 10, 2016 00:05

cormacpayne merged commit 07db5f9 into Azure:release-3.3.0 Dec 12, 2016



		# Assemblies that must be loaded prior to importing this module
		RequiredAssemblies = @()

		@@ -0,0 +1,276 @@
		# ----------------------------------------------------------------------------------


		### -CloudToDevice
		CloudToDevice

Iot Hub Commandlets #3250

Iot Hub Commandlets #3250

Uh oh!

Conversation

rkmanda commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azurecla commented Dec 5, 2016

Uh oh!

rkmanda commented Dec 5, 2016

Uh oh!

markcowl commented Dec 6, 2016

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!

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!

rkmanda commented Dec 8, 2016

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!

markcowl commented Dec 8, 2016

Uh oh!

cormacpayne commented Dec 9, 2016

Uh oh!

cormacpayne commented Dec 10, 2016

Uh oh!

rkmanda commented Dec 11, 2016

Uh oh!

Uh oh!

rkmanda commented Dec 5, 2016 •

edited

Loading