Logic App Integration Account Control Numbers cmdlets #3566

daviburg · 2017-03-02T22:20:53Z

Description

New cmdlets for Logic App Integration Account Control Numbers:
Get, Set, List generated control numbers, Get, Remove received control numbers.

Building atop Azure/azure-sdk-for-net#2858

Underlying sessions entities are intentionally not exposed to cmdlet user. Cmdlet user will know high level business concept of control numbers. Sessions is Microsoft's B2B connector specific underlying storage mechanism for this and other states.
Full CRUD is intentionally not exposed to cmdlet user. Select commands above are exposed for the specific purpose of a service operator to fix up current state in the event of a disaster recovery to a back-up site.

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.
For the sake of completeness, will add an additional commit with change log update to this PR.

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.
Remove cmdlet produces no output and a PassThru parameter would not work for it either.

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-03-02T22:25:07Z

Can one of the admins verify this patch?

cormacpayne · 2017-03-03T00:45:39Z

@azuresdkci add to whitelist

markcowl · 2017-03-03T01:51:36Z

@daviburg this is failing because it can't find your package. Have you published the package yet?

shahabhijeet · 2017-03-03T17:47:57Z

@azuresdkci test this please

…sion removed by mistake while regenerating the file.

markcowl · 2017-03-06T21:35:09Z

...anager/LogicApp/Commands.LogicApp.Test/ScenarioTests/IntegrationAccountGeneratedIcnTests.ps1

+.SYNOPSIS
+Returns true if current mode is record
+#>
+function IsRecordMode()


This is already availble from TestSupport.RunningMocked. Duplicating this is maintenance problem going forward.

So if (-not [Microsoft.WindowsAzure.Commands.Utilities.Common.TestMockSupport]::RunningMocked) gives me the wrong result when running in Record mode, i.e. return trues which cause setup code not to run and the test to fail. I will keep the approach I mimicked from existing test code which works reliably.

markcowl · 2017-03-06T21:37:39Z

...p/Commands.LogicApp/Cmdlets/IntegrationAccount/GetAzureIntegrationAccountAgreementCommand.cs

@@ -21,22 +21,32 @@ namespace Microsoft.Azure.Commands.LogicApp.Cmdlets
    /// <summary>
    /// Gets the integration account agreement by name 
    /// </summary>
-    [Cmdlet(VerbsCommon.Get, "AzureRmIntegrationAccountAgreement"), OutputType(typeof (object))]
+    [Cmdlet(VerbsCommon.Get, "AzureRmIntegrationAccountAgreement")]
+    [OutputType(typeof(object))]


This is essentially useless, please be more specific about the output type

This change resulted in a breaking change build error. I think that as object return type provided no value this breaking change is unlikely to actually break any client. Is there a way to acknowledgement the breaking change as acceptable?

00:48:41 "D:\workspace\powershell\src\Package\Debug\ResourceManager\AzureResourceManager\AzureRM.LogicApp\Microsoft.Azure.Commands.LogicApp.dll","Microsoft.Azure.Commands.LogicApp.Cmdlets.GetAzureIntegrationAccountAgreementCommand","Get-AzureRmIntegrationAccountAgreement","0","1020","The cmdlet 'Get-AzureRmIntegrationAccountAgreement' no longer has output type 'Object'.","Make cmdlet 'Get-AzureRmIntegrationAccountAgreement' return type 'Object'."

markcowl · 2017-03-06T21:39:14Z

...ommands.LogicApp/Cmdlets/IntegrationAccount/GetAzureIntegrationAccountGeneratedIcnCommand.cs

+    /// Gets the integration account generated interchange control number by agreement name
+    /// </summary>
+    [Cmdlet(VerbsCommon.Get, "AzureRmIntegrationAccountGeneratedIcn")]
+    [OutputType(typeof(IntegrationAccountClient.IntegrationAccountControlNumber), typeof(IList<IntegrationAccountClient.QualifiedIntegrationAccountControlNumber>))]


Why areen't these simple public types?

markcowl · 2017-03-06T21:39:30Z

...Commands.LogicApp/Cmdlets/IntegrationAccount/GetAzureIntegrationAccountReceivedIcnCommand.cs

+    /// Gets the integration account received interchange control number by agreement name and control number value.
+    /// </summary>
+    [Cmdlet(VerbsCommon.Get, "AzureRmIntegrationAccountReceivedIcn")]
+    [OutputType(typeof(IntegrationAccountClient.IntegrationAccountControlNumber))]


Why is this not a simple, public type?

markcowl · 2017-03-06T21:40:00Z

...ands.LogicApp/Cmdlets/IntegrationAccount/UpdateAzureIntegrationAccountGeneratedIcnCommand.cs

+    /// Updates the integration account generated interchange control number.
+    /// </summary>
+    [Cmdlet(VerbsCommon.Set, "AzureRmIntegrationAccountGeneratedIcn", SupportsShouldProcess = true)]
+    [OutputType(typeof(IntegrationAccountClient.IntegrationAccountControlNumber))]


simple type

In other words - you need to provide a simple type definition, not a type defined within another type.

markcowl · 2017-03-06T21:40:50Z

src/ResourceManager/LogicApp/Commands.LogicApp/Commands.LogicApp.csproj

@@ -193,6 +200,10 @@
      <Project>{3819d8a7-c62c-4c47-8ddd-0332d9ce1252}</Project>
      <Name>Commands.ResourceManager.Common</Name>
    </ProjectReference>
+    <ProjectReference Include="..\..\Resources\Commands.ResourceManager\Cmdlets\Commands.Resources.Rest.csproj">


This is not allowed. You cannot have a dependency on another cmdlets project

markcowl · 2017-03-06T21:42:00Z

src/ResourceManager/LogicApp/Commands.LogicApp/Utilities/IntegrationAccountControlNumber.cs

+    /// <summary>
+    /// LogicApp client partial class for integration account control number types.
+    /// </summary>
+    public partial class IntegrationAccountClient


These look like generated types, why are these here and not in your SDK?

Because the types are client-side only. The swagger and SDK specify what the server is aware of, which here is just an obscure/opaque JToken object. The clients store state specific to themselves within the session. Different client-defined types can and are stored in different sessions.

If these are exchanged with the service, you should consider adding these types to your API definition. This is not a blocker for the PR.

* Added Set-AzureRmIntegrationAccountReceivedIcn * Specified return type(s) of integration account cmdlets * Fixed return types for control number cmdlets

markcowl · 2017-03-07T18:54:43Z

@daviburg You have two issues causing static analysis failures:

Cmdlets with nouns that are plural, please fix this
The commands that now have a better return type than object - please add exceptiosn for these, our tool is incorrectly flagging these as breakign changes when they are not. We have an issue to fix the tool, but adding the escepotions for now should fix the issue.

Cleaned up help files.

daviburg · 2017-03-07T20:39:24Z

I added the exceptions for the return type. I did not find in the Jenkins build artifacts or console output where is the "Cmdlets with nouns that are plural" issue.

markcowl · 2017-03-08T18:40:21Z

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

LGTM once the on demand run passes

Logic App Integration Account Control Numbers cmdlets

5324c73

cormacpayne added the in progress label Mar 2, 2017

Updated Logic App change log.

834ba77

cormacpayne added cla-not-required and removed in progress labels Mar 3, 2017

markcowl assigned daviburg Mar 3, 2017

markcowl added the Changes Requested label Mar 3, 2017

daviburg added 3 commits March 3, 2017 11:49

Logic App tests fix: copy missing recorded session.

35bfb1e

Logic test recorded sessions: Restored CS002:SecretInNextLine suppres…

4cc5113

…sion removed by mistake while regenerating the file.

Logic App help content clean-up.

88b93d1

cormacpayne assigned markcowl and unassigned daviburg Mar 6, 2017

markcowl requested changes Mar 6, 2017

View reviewed changes

markcowl assigned daviburg and unassigned markcowl Mar 6, 2017

Logic cmdlets updates

d5aa8a5

* Added Set-AzureRmIntegrationAccountReceivedIcn * Specified return type(s) of integration account cmdlets * Fixed return types for control number cmdlets

Logic breaking change false positives marked as exceptions.

1ca56bd

Cleaned up help files.

Merge branch 'dev' into dev

4d7af19

markcowl changed the base branch from dev to release-3.7.0 March 8, 2017 00:26

markcowl added 2 commits March 7, 2017 16:26

Merge branch 'release-3.7.0' into dev

57c6bee

Merge branch 'release-3.7.0' into dev

acc733d

cormacpayne assigned markcowl and unassigned daviburg Mar 8, 2017

cormacpayne added the needs-approval label Mar 8, 2017

cormacpayne removed the Changes Requested label Mar 8, 2017

markcowl approved these changes Mar 8, 2017

View reviewed changes

markcowl merged commit 1094854 into Azure:release-3.7.0 Mar 8, 2017

Logic App Integration Account Control Numbers cmdlets #3566

Logic App Integration Account Control Numbers cmdlets #3566

Uh oh!

Conversation

daviburg commented Mar 2, 2017

Description

Uh oh!

azuresdkci commented Mar 2, 2017

Uh oh!

cormacpayne commented Mar 3, 2017

Uh oh!

markcowl commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shahabhijeet commented Mar 3, 2017

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 commented Mar 7, 2017

Uh oh!

daviburg commented Mar 7, 2017

Uh oh!

markcowl commented Mar 8, 2017

Uh oh!

Uh oh!

markcowl commented Mar 3, 2017 •

edited

Loading