Marketplace ordering Add Get/Set-AzureRmMarketplaceTerms Cmdlets #4694

prbansa · 2017-10-01T06:13:44Z

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 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.

azuresdkci · 2017-10-01T06:15:10Z

Can one of the admins verify this patch?

cormacpayne · 2017-10-02T16:27:06Z

@azuresdkci add to whitelist

cormacpayne

@prbansa a few initial comments.

Also, it looks like you will need to regenerate the wxi file as there are a lot of unnecessary changes made to that file.

@cormacpayne - Need some help to fix the wxi file, not sure what needs to be done, this was autogenerated

thanks @cormacpayne , updated the wxi file.

cormacpayne · 2017-10-03T16:40:24Z

src/ResourceManager/MarketplaceOrdering/AzureRM.MarketplaceOrdering.psd1

+# RootModule = ''
+
+# Version number of this module.
+ModuleVersion = '1.0.0'


@prbansa please set this to version 0.1.0, which is the initial version we set for all modules.

cormacpayne · 2017-10-03T16:41:23Z

src/ResourceManager/MarketplaceOrdering/ChangeLog.md

+-->
+## Current Release
+
+## Version 1.0.0


@prbansa please remove this header and put all information under the Current Release header. We have a script that runs during the release that will place all of this information under the correct header.

cormacpayne · 2017-10-03T16:41:49Z

...urceManager/MarketplaceOrdering/Commands.MarketplaceOrdering.Test/Properties/AssemblyInfo.cs

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


@prbansa both of these versions should match the module version, 0.1.0

cormacpayne · 2017-10-03T16:43:55Z

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

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


@prbansa same comment about these versions matching the module version, 0.1.0

cormacpayne · 2017-10-03T16:50:07Z

...Manager/MarketplaceOrdering/Commands.MarketplaceOrdering/help/Set-AzureRmMarketplaceTerms.md

+```
+
+### -InputObject
+Terms object returned in Get-AzureRmMarketplaceTerms cmdlet. This is a mandatory parameter if Accepted paramter is true.```yaml


@prbansa the ```yaml string at the end should be moved to a line by itself; this is a bug with platyPS. Please see other parameters in this file for an example of how this should look.

cormacpayne · 2017-10-03T16:52:03Z

...tplaceOrdering/Commands.MarketplaceOrdering/Cmdlets/Agreements/GetAzureRmMarketplaceTerms.cs

+
+namespace Microsoft.Azure.Commands.MarketplaceOrdering.Cmdlets.Agreements
+{
+    [Cmdlet(VerbsCommon.Get, "AzureRmMarketplaceTerms", DefaultParameterSetName = Constants.ParameterSetNames.AgreementParameterSet), OutputType(typeof(PSAgreementTerms))]


@prbansa I missed the design review for these cmdlets, so this may have been answered: is this a resource that can be created for a specific subscription, or even a resource group? Most Get-* cmdlets support providing no parameters (returns all resources in the subscription) or providing a resource group (returns all resources in the resource group).

@cormacpayne - Get-AzureRmMarketplaceTerms returns "Terms" object which user can accept and fully automate 3rd party VM deployment experience.
This is not a resource so Get-* is not supported in this case.

cormacpayne · 2017-10-03T16:56:17Z

...tplaceOrdering/Commands.MarketplaceOrdering/Cmdlets/Agreements/SetAzureRmMarketplaceTerms.cs

+        [Parameter(Mandatory = false, ValueFromPipeline = true, HelpMessage = "Terms object returned in Get-AzureRmMarketplaceTerms cmdlet. This is a mandatory parameter if Accepted paramter is true.", ParameterSetName = Constants.ParameterSetNames.AgreementParameterSet)]
+        [ValidateNotNullOrEmpty]
+        [Alias("Terms")]
+        public PSAgreementTerms InputObject { get; set; }


@prbansa InputObject should be in a separate parameter set from Publisher, Product and Name since it will already have the values for these parameters (they should be properties of the object returned from the Get-* cmdlet).

Also, this parameter should accept its value from the pipeline, which will allow a user to pipe the result from the Get-* cmdlet into this parameter. Please take a look at this guide.

cormacpayne · 2017-10-03T16:57:45Z

...tplaceOrdering/Commands.MarketplaceOrdering/Cmdlets/Agreements/SetAzureRmMarketplaceTerms.cs

+
+        [Parameter(Mandatory = true, HelpMessage = "Boolean which would indicate the status of acceptance of the terms, it should be true if any version of the terms have been accepted.", ParameterSetName = Constants.ParameterSetNames.AgreementParameterSet)]
+        [ValidateNotNullOrEmpty]
+        public SwitchParameter Accepted { get; set; }


@prbansa take another look at this parameter after moving the InputObject parameter into a separate parameter set. Right now it is a mandatory SwitchParameter, meaning it should always be provided in the single parameter set that exists today, which provides no value to the cmdlet.

@cormacpayne Let's discuss this, I changed this from bool based on comment I got on this PR
https://github.com/Azure/azure-powershell-pr/pull/331
Look for comments on this file -MarketplaceOrdering/Cmdlets/Agreements/SetAzureRmMarketplaceTerms.cs

changed to bool as discussed

prbansa · 2017-10-04T05:01:37Z

@cormacpayne - updated after review, please look at my changes. Let's work together to fix wxi file

maddieclayton · 2017-10-04T23:58:57Z

...Manager/MarketplaceOrdering/Commands.MarketplaceOrdering/help/Set-AzureRmMarketplaceTerms.md

+```
+
+### -DefaultProfile
+The credentials, account, tenant, and subscription used for communication with azure.```yaml


The ```yaml text needs to be on a line by itself; this is a bug in the platyPS module. Please see the below parameters for how this should look.

cormacpayne

@prbansa a couple minor things I forgot to mention:

(1) You will need to add your module to the AzureRM.psd1 roll-up module. This will ensure that your cmdlets are installed when a user runs Install-Module AzureRM.

(2) In order for your tests to be run, you will need to make sure they get picked up by our Smart Tester (which scans the files in a PR and runs tests associated with the files changed). To do so, you will need to update the following things:

Add your test dll to this json file (you can follow the format of the other services)
There are a few tests with hard-coded values in the Smart Tester that fail whenever a new module is added, so would you mind adding a Skip to these tests so you don't have to deal with updating the values? We will create a work item to update these values.

prbansa · 2017-10-05T02:03:14Z

@cormacpayne Updated the code as discussed, I like to understand how to skip the tests as you suggested.

prbansa · 2017-10-05T16:05:07Z

@cormacpayne this build failed because of unit tests as you predicted, let me know how can I skip the tests and if any other feedback you have on my latest commit.

maddieclayton · 2017-10-05T17:33:44Z

...rketplaceOrdering/Commands.MarketplaceOrdering.Test/Commands.MarketplaceOrdering.Test.csproj

+    <None Include="ScenarioTests\AgreementsTests.ps1">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>
+    <None Include="SessionRecords\Microsoft.Azure.Commands.MarketplaceOrdering.Test.ScenarioTests.AgreementsTests\TestGetAgreement.json" />


Session records should all be copied to output directory (similar to ScenarioTest\AgreementTests.ps1 above) - this should solve your issue.

<None Include="SessionRecords\Microsoft.Azure.Commands.MarketplaceOrdering.Test.ScenarioTests.AgreementsTests\TestGetAgreement.json" > <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> </None>

Sure will update this

cormacpayne

@prbansa two minor comments, otherwise LGTM

cormacpayne · 2017-10-06T16:36:10Z

...Manager/MarketplaceOrdering/Commands.MarketplaceOrdering/help/Set-AzureRmMarketplaceTerms.md

+```
+
+### -Reject
+Pass this to reject the legal terms.```yaml


@prbansa looks like the ```yaml string got added to the wrong line in the last commit

yes, fixed.

cormacpayne · 2017-10-06T16:37:00Z

...tplaceOrdering/Commands.MarketplaceOrdering/Cmdlets/Agreements/GetAzureRmMarketplaceTerms.cs

+            }
+            catch (ErrorResponseException e)
+            {
+                WriteWarning(e.Body.Error.Message);


@prbansa I think we should do some null-checking around this, I would hate for the actual cmdlet error to not get bubbled up because some part of this chain is null

cormacpayne · 2017-10-06T20:45:04Z

On-demand: http://azuresdkci.cloudapp.net/job/powershell-demand/1901/
Packing job: http://azuresdkci.cloudapp.net/job/posh-pack/23/

cormacpayne · 2017-10-06T23:27:51Z

@prbansa you will need to resolve the merge conflicts; I sent you a message offline about how to do so.

Also, the on-demand job is failing StaticAnalysis because the noun used for a couple of cmdlets/parameters is plural, which goes against PowerShell guidance. However, in this case, it is allowed, so you will need to suppress these errors. To do so, you will need to open on-demand job and open the SignatureIssues.csv file in a text editor. Copy the severity 1 issues (there should be three) and append them to the end of tools/StaticAnalysis/Exceptions/SignatureIssues.csv (also open in a text editor). Please let me know if you have any questions.

…s for workernode during cluster creation.

…e to comments on PR

…SetGeneratorTester.cs

…ated Set-AzureRmMarketplaceTerms.md

… to noun

prbansa · 2017-10-07T07:46:44Z

@cormacpayne , @markcowl I have made the necessary changes to merge the conflicts in wxi, it gave some trouble.
Do you think it will be better if I submit a new PR.

cormacpayne · 2017-10-09T16:22:38Z

On-demand: http://azuresdkci.cloudapp.net/job/powershell-demand/1913/
Packaging job: http://azuresdkci.cloudapp.net/job/posh-pack/34/

markcowl · 2017-10-10T18:53:53Z

src/ResourceManager/MarketplaceOrdering/AzureRM.MarketplaceOrdering.psd1

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


This version will need to be updated for the preview branch

…hell into MarketplaceOrdering_CmdLets # Conflicts: # tools/AzureRM/AzureRM.psd1

cormacpayne · 2017-10-10T21:10:02Z

On-demand: http://azuresdkci.cloudapp.net/job/powershell-demand/1920/
Packaging job: http://azuresdkci.cloudapp.net/job/posh-pack/37/

prbansa · 2017-10-11T01:40:39Z

@cormacpayne - I tested the installer from - http://azuresdkci.cloudapp.net/job/posh-pack/37/artifact/setup/build/Debug/
Looks good.

markcowl · 2017-10-11T05:21:14Z

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

release build here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/posh-release/3/

azuresdkci added the needs-review label Oct 1, 2017

azuresdkci assigned markcowl Oct 1, 2017

cormacpayne suggested changes Oct 3, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Oct 3, 2017

cormacpayne assigned prbansa and unassigned markcowl Oct 3, 2017

maddieclayton reviewed Oct 4, 2017

View reviewed changes

cormacpayne suggested changes Oct 5, 2017

View reviewed changes

maddieclayton reviewed Oct 5, 2017

View reviewed changes

cormacpayne suggested changes Oct 6, 2017

View reviewed changes

cormacpayne added Waiting for On Demand Merge Conflicts needs-revision and removed needs-revision Waiting for On Demand labels Oct 6, 2017

Abhijith Padmakumar and others added 7 commits October 6, 2017 20:33

Update New-AzureRmHDInsightCluster cmdlet to support data disks group…

595f7fb

…s for workernode during cluster creation.

Add New Cmdlets for MarketplaceOrdering

417e57d

Update the AzureRM.Profile version for MarketplaceOrdering

6cb4d45

MarketplaceOrdering updated after review comments

94026f6

Updated wxi file with MarketplaceOrdering cmdlets

89d187b

MarketplaceOrdering use swithparameter instead of bool and changes du…

95e20c0

…e to comments on PR

MarketplaceOrdering updated help files and disable some tests in Test…

4507d32

…SetGeneratorTester.cs

prbansa added 2 commits October 6, 2017 20:33

Marketplace cmdlet minor updates - removed try/catch from get and upd…

d718f57

…ated Set-AzureRmMarketplaceTerms.md

MarketplaceOrdering cmdlets fix the git merging issues

9278752

prbansa force-pushed the MarketplaceOrdering_CmdLets branch from 3865fa3 to 9278752 Compare October 7, 2017 06:34

MarketplaceOrdering updated SignatureIssues.csv to supress errors due…

56f193e

… to noun

markcowl changed the base branch from preview to release-4.4.1 October 7, 2017 23:56

Merge branch 'release-4.4.1' into MarketplaceOrdering_CmdLets

acee87f

cormacpayne approved these changes Oct 9, 2017

View reviewed changes

markcowl requested changes Oct 10, 2017

View reviewed changes

Merge branch 'release-4.4.1' of https://github.com/Azure/azure-powers…

21f0dce

…hell into MarketplaceOrdering_CmdLets # Conflicts: # tools/AzureRM/AzureRM.psd1

Merge branch 'release-4.4.1' into MarketplaceOrdering_CmdLets

6c167fd

markcowl approved these changes Oct 11, 2017

View reviewed changes

markcowl merged commit 221015a into Azure:release-4.4.1 Oct 11, 2017

Marketplace ordering Add Get/Set-AzureRmMarketplaceTerms Cmdlets #4694

Marketplace ordering Add Get/Set-AzureRmMarketplaceTerms Cmdlets #4694

Uh oh!

Conversation

prbansa commented Oct 1, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

azuresdkci commented Oct 1, 2017

Uh oh!

cormacpayne commented Oct 2, 2017

Uh oh!

cormacpayne left a comment • edited by prbansa 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!

prbansa commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

prbansa commented Oct 5, 2017

Uh oh!

prbansa commented Oct 5, 2017

Uh oh!

maddieclayton Oct 5, 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!

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!

cormacpayne commented Oct 6, 2017

Uh oh!

cormacpayne commented Oct 6, 2017

Uh oh!

prbansa commented Oct 7, 2017

Uh oh!

cormacpayne commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented Oct 10, 2017

Uh oh!

prbansa commented Oct 1, 2017 •

edited by azuresdkci

Loading

cormacpayne left a comment •

edited by prbansa

Loading

maddieclayton Oct 5, 2017 •

edited

Loading