Adding RunOn support and other issue fixes. #4349

vrdmr · 2017-07-21T00:51:56Z

Description

Adding runOn support for Scheduled Job and Webhook cmdlets; Also adding alias for Modules in New/Set Modules. The changes were required for couple of issues raised earlier.

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.

msftclas · 2017-07-21T00:52:00Z

@vrdmr,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

azuresdkci · 2017-07-21T00:55:06Z

Can one of the admins verify this patch?

cormacpayne · 2017-07-24T18:05:30Z

@azuresdkci add to whitelist

cormacpayne

@vrdmr Hey Varad, a couple questions:

Do you plan to add test coverage for the cmdlets that are now using RunOn?
Does this PR resolve issues New-AzureRmAutomationWebHook: add support for RunOn #3082 and Register-AzureRmAutomationScheduledRunbook: add support for RunOn #3197 ?
Can you update the change log with a snippet about the changes you are making in this PR?

markcowl · 2017-08-03T19:21:56Z

@vrdmr There has been no activity on this PR in 10 days, we will have to close it this week if you do not address review feedback

vrdmr · 2017-08-04T00:39:13Z

@cormacpayne @markcowl

Do you plan to add test coverage for the cmdlets that are now using RunOn?
-- Updated the existing tests.
Does this PR resolve issues New-AzureRmAutomationWebHook: add support for RunOn #3082 and Register-AzureRmAutomationScheduledRunbook: add support for RunOn #3197 ?
-- Yes. Add-AzureRmAutomationModule and Set-AzureRmAutomationModule 'ContentLink' parameters should have the same name. #3775, Register-AzureRmAutomationScheduledRunbook: add support for RunOn #3197 and New-AzureRmAutomationWebHook: add support for RunOn #3082.
Can you update the change log with a snippet about the changes you are making in this PR?
-- Updated the ChangeLog with a small description of what has been updated.

cormacpayne

@vrdmr it doesn't look like any of the tests that were modified are testing when a value is passed through for the runOn parameter. Any reason for that?

cormacpayne · 2017-08-04T17:15:23Z

src/ResourceManager/Automation/Commands.Automation/Cmdlet/NewAzureAutomationModule.cs

@@ -39,6 +39,7 @@ public class NewAzureAutomationModule : AzureAutomationBaseCmdlet
        /// </summary>
        [Parameter(Position = 3, Mandatory = true, ValueFromPipelineByPropertyName = true,
            HelpMessage = "The url to a module zip package.")]
+        [Alias("ContentLinkUri")]


@vrdmr I think it would be better if we changed the name of this parameter to ContentLinkUri and added an alias to ContentLink. That way, when users tab through the parameters, ContentLinkUri will show up and will be consistent with what is shown when tabbing through the parameters of Set-AzureRmAutomationModule.

I agree. I'll confirm with @safeermohammed and make the change asap.

@cormacpayne Does changing he parameter name will be breaking change, i.e., does user already using the cmdlet need to do any thing? If this is not the case, fine to do as suggested.

@vrdmr @safeermohammed if we swap the parameter name and its alias, it won't be a breaking change. Existing scripts will be able to continue to use ContentLink; the only difference is when people tab through the possible parameters (as well as look at the docs for the cmdlet), they will see ContentLinkUri, which I think is a better indication of what this parameter is. The use of ContentLink is not lost in this swap.

@cormacpayne Thank you for confirmation, we will swap it

vrdmr · 2017-08-05T00:32:53Z

@cormacpayne - No particular reason for not passing any specific values for runOn. It is currently not a high priority scenario and we are in process of adding more functionality in the backend for it.

cormacpayne · 2017-08-07T16:56:48Z

src/ResourceManager/Automation/Commands.Automation/Cmdlet/NewAzureAutomationModule.cs

        [ValidateNotNullOrEmpty]
-        public Uri ContentLink { get; set; }
+        public Uri ContentLinkUri { get; set; }


@vrdmr you are still referencing ContentLink below instead of ContentLinkUri, which is causing compilation errors

Adding runOn support for Scheduled Job and Webhook cmdlets; Also adding alias for Modules in New/Set Modules. The changes were required for couple of issues raised earlier.

markcowl · 2017-08-07T21:15:17Z

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

@vrdmr Filed issue #4422 for adding additional runOn tests

vrdmr · 2017-08-07T23:27:09Z

Thanks @cormacpayne! :)

vrdmr requested a review from safeermohammed July 21, 2017 00:51

msftclas added the cla-not-required label Jul 21, 2017

azuresdkci added the needs-review label Jul 21, 2017

azuresdkci assigned cormacpayne Jul 21, 2017

cormacpayne suggested changes Jul 25, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Jul 25, 2017

cormacpayne assigned vrdmr and unassigned cormacpayne Jul 25, 2017

cormacpayne changed the base branch from preview to release-4.3.0 August 4, 2017 16:01

cormacpayne suggested changes Aug 4, 2017

View reviewed changes

cormacpayne suggested changes Aug 7, 2017

View reviewed changes

Adding RunOn support and other issue fixes.

7d81fa2

Adding runOn support for Scheduled Job and Webhook cmdlets; Also adding alias for Modules in New/Set Modules. The changes were required for couple of issues raised earlier.

markcowl added Waiting for On Demand and removed needs-revision labels Aug 7, 2017

cormacpayne approved these changes Aug 7, 2017

View reviewed changes

cormacpayne merged commit 884a396 into Azure:release-4.3.0 Aug 7, 2017

lfshr mentioned this pull request Aug 31, 2017

Add-AzureRmAutomationModule and Set-AzureRmAutomationModule 'ContentLink' parameters should have the same name. #3775

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding RunOn support and other issue fixes. #4349

Adding RunOn support and other issue fixes. #4349

vrdmr commented Jul 21, 2017 •

edited by azuresdkci

Loading

msftclas commented Jul 21, 2017

azuresdkci commented Jul 21, 2017

cormacpayne commented Jul 24, 2017

cormacpayne left a comment

markcowl commented Aug 3, 2017

vrdmr commented Aug 4, 2017 •

edited

Loading

cormacpayne left a comment

cormacpayne Aug 4, 2017

vrdmr Aug 4, 2017

safeermohammed Aug 4, 2017 •

edited

Loading

cormacpayne Aug 4, 2017

safeermohammed Aug 4, 2017

vrdmr commented Aug 5, 2017

cormacpayne Aug 7, 2017

vrdmr Aug 7, 2017

markcowl commented Aug 7, 2017

vrdmr commented Aug 7, 2017

Adding RunOn support and other issue fixes. #4349

Adding RunOn support and other issue fixes. #4349

Conversation

vrdmr commented Jul 21, 2017 • edited by azuresdkci Loading

Description

msftclas commented Jul 21, 2017

azuresdkci commented Jul 21, 2017

cormacpayne commented Jul 24, 2017

cormacpayne left a comment

Choose a reason for hiding this comment

markcowl commented Aug 3, 2017

vrdmr commented Aug 4, 2017 • edited Loading

cormacpayne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safeermohammed Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrdmr commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcowl commented Aug 7, 2017

vrdmr commented Aug 7, 2017

vrdmr commented Jul 21, 2017 •

edited by azuresdkci

Loading

vrdmr commented Aug 4, 2017 •

edited

Loading

safeermohammed Aug 4, 2017 •

edited

Loading