Repository navigation
[LogicApp] Add new Integration Account artifact cmdlets - #8483
Maddie Clayton (maddieclayton) merged 5 commits into
Conversation
|
refortie Please fix merge conflicts and I'll take a look tomorrow. |
22fec5c to
f935301
Compare
Maddie Clayton (maddieclayton)
left a comment
Maddie Clayton (maddieclayton)
left a comment
There was a problem hiding this comment.
Really great looking cmdlets! I left a few comments, mostly cosmetic or future proofing.
| case ParameterSet.ByIntegrationAccountAndParameters: | ||
| { | ||
| var releaseCriteria = new BatchReleaseCriteria(); | ||
| if (this.MessageCount > 0) |
There was a problem hiding this comment.
Instead of doing this validation here, please add a [ValidateRange(0, int.MaxValue)] for MessageCount, BatchSize, and ScheduleInterval
There was a problem hiding this comment.
This isn't validation. When the parameter isn't specified the default value is set, I'm checking to see if the optional parameter was specified.
There was a problem hiding this comment.
Shouldn't you be checking if the parameter is set rather than is greater than 0? Currently there is nothing preventing the user from providing a value less than zero. You can check if that parameter is set using this.MyInvocation.BoundParameters.ContainsKey("MessageCount")
There was a problem hiding this comment.
I'll move over and use that and add the validation in the inputs section.
EDIT: Does using the ValdiateRange supersede the ValidateNotNullOrEmpty? Aka, should I include both or does the ValidateRange also check for nulls
Maddie Clayton (maddieclayton)
assigned
refortie and unassigned
Maddie Clayton (maddieclayton)
Maddie Clayton (maddieclayton)
assigned
Maddie Clayton (maddieclayton) and unassigned
refortie
| case ParameterSet.ByIntegrationAccountAndParameters: | ||
| { | ||
| var releaseCriteria = new BatchReleaseCriteria(); | ||
| if (this.MessageCount > 0) |
There was a problem hiding this comment.
Shouldn't you be checking if the parameter is set rather than is greater than 0? Currently there is nothing preventing the user from providing a value less than zero. You can check if that parameter is set using this.MyInvocation.BoundParameters.ContainsKey("MessageCount")
Maddie Clayton (maddieclayton)
assigned
refortie and unassigned
Maddie Clayton (maddieclayton)
| /// <param name="resourceGroupName">Resource group name</param> | ||
| /// <param name="subscriptionId">Subscription id</param> | ||
| /// <returns>App service plan id</returns> | ||
| internal static string BuildAppServicePlanId(string planName, string resourceGroupName, string subscriptionId) |
There was a problem hiding this comment.
Just noticed this wasn't used anywhere and removed it
|
refortie Nice work! And thanks so much for splitting this up. |
4 participants
Description
Adding in the cmdlets for Assemblies and Batch Configurations. Completed Design Review: Design Review
Checklist
CONTRIBUTING.mdplatyPSmodule