[ADF] Adding Update-AzureRmDataFactoryV2 and Stop-AzureRmDataFactoryV2PipelineRun #5045

hvermis · 2017-11-29T04:05:32Z

Description

Adding 2 new cmdlets for ADF V2: Update-AzureRmDataFactoryV2 and Stop-AzureRmDataFactoryV2PipelineRun with tests. Also added aliases for Set- cmdlets to also be discoverable with New- verb.
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.

…so be New-

…nto ADFv2

…s check failed

hvermis · 2017-11-29T04:09:49Z

@maddieclayton This PR also contains Cancel pipeline run, please review.
CC: @milantomic6

maddieclayton

Added comments for Stop-AzureRmDataFactoryV2PipelineRun.

maddieclayton · 2017-11-29T23:04:36Z

src/ResourceManager/DataFactories/Commands.DataFactoryV2.Test/ScenarioTests/RunTests.cs

@@ -31,5 +31,11 @@ public void TestRunV2()
            RunPowerShellTest("Test-Run");
        }

+        [Fact]


Can you add a test for a successful cancellation?

Modified the run test to include it

maddieclayton · 2017-11-29T23:13:24Z

src/ResourceManager/DataFactories/AzureRM.DataFactoryV2.psd1

@@ -75,6 +75,7 @@ FunctionsToExport = @()
 # Cmdlets to export from this module, for best performance, do not use wildcards and do not delete the entry, use an empty array if there are no cmdlets to export.
 CmdletsToExport = 
               'Set-AzureRmDataFactoryV2',
+               'Update-AzureRmDataFactoryV2',


Please update the change log with information about these two new cmdlets

maddieclayton · 2017-11-29T23:21:15Z

...eManager/DataFactories/Commands.DataFactoryV2/Runs/StopAzureDataFactoryPipelineRunCommand.cs

+    public class StopAzureDataFactoryPipelineRunCommand : DataFactoryContextBaseCmdlet
+    {
+        [Parameter(ParameterSetName = ParameterSetNames.ByInputObject, Position = 0, Mandatory = true, ValueFromPipeline = true,
+            HelpMessage = Constants.HelpPipelineRunId)]


This help message is confusing for the user - make a new entry in Constants for this InputObject

maddieclayton · 2017-11-29T23:31:52Z

...eManager/DataFactories/Commands.DataFactoryV2/Runs/StopAzureDataFactoryPipelineRunCommand.cs

+        public override void ExecuteCmdlet()
+        {
+            ByFactoryObject(DataFactory);
+            if (ParameterSetName.Equals(ParameterSetNames.ByInputObject, StringComparison.OrdinalIgnoreCase))


You might also move ByInputObject from DataFactoryContextActionBaseCmdlet to DataFactoryContextBaseCmdlet so you can use it here. Alternatively, is there a reason you aren't inheriting from DataFactoryContextActionBaseCmdlet?

Not this one because it is also assigning the pipeline run id in case by input object.
ActionBase also has ResourceId parameter which can't be added to this cmdlet as the pipelineRun is not an azure resource.

That makes sense, got it.

maddieclayton · 2017-11-29T23:49:42Z

...rceManager/DataFactories/Commands.DataFactoryV2/help/Stop-AzureRmDataFactoryV2PipelineRun.md

+
+### Example 1
+```
+PS C:\> Stop-AzureRmDataFactoryV2PipelineRun -ResourceGroupName "ADF" -DataFactoryName "UncycloADF" -PipelineRunId b9730a13-aa12-4926-a8b3-8e3a974ab0bd


Can you add an example for each parameter set? Also, make sure this isn't a real DataFactory/PipelineRunId.

Our other cmdlets also only have 1, at most 2 examples - at some point we will have vendors to write more detailed descriptions in Powersehll docs and add more examples.

Okay, as long as there are definite plans in the future for improving documentation.

maddieclayton · 2017-11-29T23:51:03Z

...rceManager/DataFactories/Commands.DataFactoryV2/help/Stop-AzureRmDataFactoryV2PipelineRun.md

+true
+```
+
+This command stops the pipelin run with id b9730a13-aa12-4926-a8b3-8e3a974ab0bd in the factory UncycloADF.


… scenario

maddieclayton

One small comment, and then the Stop-AzureRmDataFactoryV2PipelineRun cmdlet looks good to me. Waiting on review from @cormacpayne for the other cmdlet.

maddieclayton · 2017-11-30T01:37:47Z

src/ResourceManager/DataFactories/Commands.DataFactoryV2.Test/ScenarioTests/RunTests.ps1

@@ -43,7 +43,8 @@ function Test-Run
        Set-AzureRmDataFactoryV2Pipeline -ResourceGroupName $rgname -Name $pipelineName -DataFactoryName $dfname -File ".\Resources\pipeline.json" -Force

        $Run = Invoke-AzureRmDataFactoryV2Pipeline -ResourceGroupName $rgname -PipelineName $pipelineName -DataFactoryName $dfname -Parameter @{"OutputBlobName"="test";}
-
+        Assert-True { Stop-AzureRmDataFactoryV2PipelineRun -ResourceGroupName $rgname -PipelineRunId $Run -DataFactoryName $dfname -Force -PassThru}
+


There is a bigger problem here that none of the Get cmdlets are not being thoroughly tested, but this is good enough for the Stop cmdlet.

Yes, I saw that too. The author probably wanted to just make sure it doesn't throw an exception. I will follow up with them.

maddieclayton · 2017-11-30T01:40:39Z

src/ResourceManager/DataFactories/Commands.DataFactoryV2/Changelog.md

@@ -19,5 +19,8 @@
 -->

 ## Current Release
+* Added 2 new cmdlets: Update-AzureRmDataFactoryV2 and Stop-AzureRmDataFactoryV2PipelineRun
+
+## 2017.09.25 - Version 0.3.0 (Previous Release)


No need to add this new header, it will be done automatically in the next release. Just keep everything under the current release header.

issues addressed

…nto ADFv2

cormacpayne

@hvermis a couple of comments to take a look at

cormacpayne · 2017-12-01T19:29:55Z

src/ResourceManager/DataFactories/Commands.DataFactoryV2/Changelog.md

@@ -19,5 +19,5 @@
 -->

 ## Current Release
-* Azure DataFactory Powershell cmdlets for ADF V2 Private Preview


@hvermis looks like the change log didn't get updated from the previous release - this should look like the following:

## Current Release * Added two new cmdlets: Update-AzureRmDataFactoryV2 and Stop-AzureRmDataFactoryV2PipelineRun ## Version 0.1.0 * Azure DataFactory PowerShell cmdlets for ADF V2 Private Preview

Should it be Version 0.1.0 or Version 0.3.0? We released our V2 preview under 0.3.0.

@hvermis it should be 0.3.0 in that case

cormacpayne · 2017-12-01T19:32:05Z

...eManager/DataFactories/Commands.DataFactoryV2/DataFactories/UpdateAzureDataFactoryCommand.cs

+                Identity = Identity
+            };
+
+            WriteObject(DataFactoryClient.UpdatePSDataFactory(parameters));


@hvermis looks like there is no ShouldProcess block wrapping this call

cormacpayne · 2017-12-01T19:33:17Z

...eManager/DataFactories/Commands.DataFactoryV2/DataFactories/UpdateAzureDataFactoryCommand.cs

+            HelpMessage = Constants.HelpIdentityForFactory)]
+        [Parameter(ParameterSetName = ParameterSetNames.ByResourceId, Position = 1, Mandatory = false, ValueFromPipelineByPropertyName = true,
+            HelpMessage = Constants.HelpIdentityForFactory)]
+        public FactoryIdentity Identity { get; set; }


@hvermis where is a user supposed to get this value from?

This is one of the properties on data factory which gets created when they create a factory. The class is part of our SDK, and they can create a new instance of that class and pass it to update command, or they can get it from an existing factory.

I changed the Value from pipeline to be by object. Here's an exampl of how it would look like:
(Get-AzureRmDataFactoryv2 -ResourceGroupName hvtest -DataFactoryName hvadf1).Identity|Update-AzureRmDataFactoryV
2 -DataFactoryName hvadf -ResourceGroupName hvtest

Is this worth supporting? Should I just remove the ValueFromPipeline completely?

@hvermis there currently exists a parameter set where DataFactory and Identity are in the same parameter set and both accept their value from the pipeline, which shouldn't happen. Only DataFactory should accept its value from the pipeline the in the parameter set, and we should look at ways of allowing the user to update the value of Identity.

cormacpayne · 2017-12-01T19:33:50Z

...eManager/DataFactories/Commands.DataFactoryV2/DataFactories/UpdateAzureDataFactoryCommand.cs

+            HelpMessage = Constants.HelpTagsForFactory)]
+        public Hashtable Tag { get; set; }
+
+        [Parameter(ParameterSetName = ParameterSetNames.ByFactoryName, Position = 2, Mandatory = false, ValueFromPipelineByPropertyName = true,


@hvermis please remove the Position property from both Tag and Identity if they are optional parameters

cormacpayne · 2017-12-01T19:36:51Z

...eManager/DataFactories/Commands.DataFactoryV2/DataFactories/UpdateAzureDataFactoryCommand.cs

+        public Hashtable Tag { get; set; }
+
+        [Parameter(ParameterSetName = ParameterSetNames.ByFactoryName, Position = 2, Mandatory = false, ValueFromPipelineByPropertyName = true,
+            HelpMessage = Constants.HelpIdentityForFactory)]


@hvermis since Tag and Identity appear in all of the parameter sets and are optional, you can remove the multiple Parameter attributes that each one has and have only one Parameter attribute on each one, which will have no defined ParameterSetName

cormacpayne · 2017-12-01T19:38:04Z

src/ResourceManager/DataFactories/Commands.DataFactoryV2/DataFactoryContextBaseCmdlet.cs

@@ -35,5 +36,14 @@ public abstract class DataFactoryContextBaseCmdlet: DataFactoryBaseCmdlet
            HelpMessage = Constants.HelpFactoryName)]
        [ValidateNotNullOrEmpty]
        public string DataFactoryName { get; set; }
+
+        protected void ByFactoryObject(PSDataFactory dataFactory)


@hvermis why does dataFactory need to be passed in if you have a reference to DataFactory above?

Refactoring issues - was trying to make more generic for other cmdlets too that have PSDatafactory called InputObject, and hit other issues. Removed it.

cormacpayne · 2017-12-01T19:41:13Z

...eManager/DataFactories/Commands.DataFactoryV2/Runs/StopAzureDataFactoryPipelineRunCommand.cs

+        public string PipelineRunId { get; set; }
+
+        [Parameter(Mandatory = false, HelpMessage = Constants.HelpDontAskConfirmation)]
+        public SwitchParameter Force { get; set; }


@hvermis any reason you need to prompt the user for additional confirmation?

No, but our other Stop- cmdlets also have this, and I was getting static analysis build failure, but them I found that if I leave ShouldProcess and remove -Force, the static analysis check passes. I will update this and other cmdlets.

cormacpayne · 2017-12-01T19:42:26Z

src/ResourceManager/DataFactories/Commands.DataFactoryV2/help/Update-AzureRmDataFactoryV2.md

+```
+
+### -Force
+Don't ask for confirmation.```yaml


@hvermis make sure to move the trailing ```yaml string to a line by itself

Force is gone

maddieclayton · 2017-12-05T17:20:02Z

@hvermis Some merge conflicts have come up, can you please resolve these?

…port its update.

cormacpayne

@hvermis one minor comment, otherwise LGTM

cormacpayne · 2017-12-06T17:26:13Z

...rceManager/DataFactories/Commands.DataFactoryV2/help/Stop-AzureRmDataFactoryV2PipelineRun.md

@@ -0,0 +1,205 @@
+---
+external help file: Microsoft.Azure.Commands.DataFactoryV2.dll-Help.xml
+online version: 


@hvermis for the two cmdlets you are adding, can you make sure that the online version is set to the following URL:

https://docs.microsoft.com/en-us/powershell/module/azurerm.datafactories/<cmdlet_name>

All existing cmdlet markdown files have this field, so feel free to use them as an example

cormacpayne · 2017-12-06T17:28:06Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/244/

hvermis and others added 5 commits November 28, 2017 14:16

Adding Update-Datafactory cmdlet; Adding alias for Set- cmdlets to al…

c9067fb

…so be New-

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

8f38bdf

…nto ADFv2

Initial check-in for cancel run feature

57f27f1

Updating Cencel Run cmdlet according to the review

ceb7f8e

Adding a test for Update-Factory n bringng -Frc back as StaticAnalysi…

73157ad

…s check failed

hvermis requested a review from cormacpayne November 29, 2017 04:05

azuresdkci added the needs-review label Nov 29, 2017

azuresdkci assigned cormacpayne Nov 29, 2017

maddieclayton previously requested changes Nov 29, 2017

View reviewed changes

Addressed review comments

5b6a148

maddieclayton assigned hvermis and unassigned cormacpayne Nov 30, 2017

maddieclayton added needs-revision and removed needs-review labels Nov 30, 2017

Modified test-run to also include stop-pipelinerun for the successful…

0acbc7d

… scenario

hvermis assigned maddieclayton and unassigned hvermis Nov 30, 2017

maddieclayton reviewed Nov 30, 2017

View reviewed changes

maddieclayton assigned cormacpayne and unassigned maddieclayton Nov 30, 2017

maddieclayton added needs-review and removed needs-revision labels Nov 30, 2017

Fixed Changelog.md

a87ab3a

twitchax and others added 3 commits November 30, 2017 14:37

Merge branch 'preview' into ADFv2

93e3e70

Removed -Force from Update-DataFactory

3ffca26

Merge branch 'ADFv2' of https://github.com/hvermis/azure-powershell i…

85f3b4e

…nto ADFv2

cormacpayne suggested changes Dec 1, 2017

View reviewed changes

cormacpayne added the needs-revision label Dec 1, 2017

cormacpayne removed the needs-review label Dec 1, 2017

cormacpayne assigned hvermis and unassigned cormacpayne Dec 1, 2017

Addressed review comments

ae4cc8c

hvermis assigned cormacpayne and unassigned hvermis Dec 1, 2017

maddieclayton added needs-review and removed needs-revision labels Dec 1, 2017

cormacpayne added needs-revision and removed needs-review labels Dec 4, 2017

cormacpayne assigned hvermis and unassigned cormacpayne Dec 4, 2017

maddieclayton added the Merge Conflicts label Dec 5, 2017

hvermis added 2 commits December 5, 2017 16:55

Removed Identity parameter from Update-Factory as backend doesn't sup…

739b828

…port its update.

Updated from preview branch

1433565

hvermis changed the base branch from preview to release-5.1.0 December 6, 2017 01:06

maddieclayton removed the Merge Conflicts label Dec 6, 2017

cormacpayne suggested changes Dec 6, 2017

View reviewed changes

Fixed reference for online help

80492e9

maddieclayton assigned cormacpayne and unassigned hvermis Dec 6, 2017

maddieclayton added needs-review and removed needs-revision labels Dec 6, 2017

cormacpayne approved these changes Dec 6, 2017

View reviewed changes

cormacpayne merged commit d004c02 into Azure:release-5.1.0 Dec 6, 2017

[ADF] Adding Update-AzureRmDataFactoryV2 and Stop-AzureRmDataFactoryV2PipelineRun #5045

[ADF] Adding Update-AzureRmDataFactoryV2 and Stop-AzureRmDataFactoryV2PipelineRun #5045

Uh oh!

Conversation

hvermis commented Nov 29, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

hvermis commented Nov 29, 2017

Uh oh!

maddieclayton 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!

maddieclayton 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!

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!

hvermis Dec 1, 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

hvermis commented Nov 29, 2017 •

edited by azuresdkci

Loading

hvermis Dec 1, 2017 •

edited

Loading