Skip to content

Issue 22: Updating scheduled task instead of Unregister/Register#138

Merged
PlagueHO merged 14 commits intodsccommunity:devfrom
GitBadSanta:setupdate
Mar 5, 2018
Merged

Issue 22: Updating scheduled task instead of Unregister/Register#138
PlagueHO merged 14 commits intodsccommunity:devfrom
GitBadSanta:setupdate

Conversation

@GitBadSanta
Copy link
Copy Markdown

@GitBadSanta GitBadSanta commented Jan 30, 2018

Pull Request (PR) description

This Pull Request (PR) fixes the following issues:

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

kwirkykat and others added 8 commits December 14, 2016 12:51
Release of version 1.10.0.0 of xComputerManagement
Release of version 2.0.0.0 of xComputerManagement
Release of version 2.1.0.0 of xComputerManagement
Release of version 3.0.0.0 of xComputerManagement
Release of version 3.1.0.0 of xComputerManagement
Release of version 3.2.0.0 of xComputerManagement
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 30, 2018

Codecov Report

Merging #138 into dev will increase coverage by <1%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #138    +/-   ##
===================================
+ Coverage   89%    89%   +<1%     
===================================
  Files        5      5            
  Lines      638    647     +9     
===================================
+ Hits       570    579     +9     
  Misses      68     68

@GitBadSanta
Copy link
Copy Markdown
Author

This has been tested on

  • Windows 10
  • Windows 2012 R1
  • Windows 2016

The repetition trick is unfortunately leading to an overlapped code.

@PlagueHO
Copy link
Copy Markdown
Member

PlagueHO commented Feb 3, 2018

Looking really aweseome @GitBadSanta - this is an excellent change and thank you for submitting it! Just some really minor style tweaks to meet the Style Guidelines (https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md). So if you make those changes I'll get this merged ASAP!


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1181 at r1 (raw file):

        $tempScheduledTask = New-ScheduledTask @scheduledTaskArguments -ErrorAction Stop
        
        if ($currentValues.Ensure -eq 'Present') {

Can you move { to next line?


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1185 at r1 (raw file):

            $tempScheduledTask = New-ScheduledTask @scheduledTaskArguments -ErrorAction Stop

            $scheduledTask = Get-ScheduledTask -TaskName $currentValues.TaskName -TaskPath $currentValues.TaskPath -ErrorAction Stop

Non-blocking issue: Can you split parameters onto each line using ` backticks? It makes it easier to review (long lines wrap).
E.g.

$scheduledTask = Get-ScheduledTask `
  -TaskName $currentValues.TaskName `
  -TaskPath $currentValues.TaskPath `
  -ErrorAction Stop

Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1190 at r1 (raw file):

            $scheduledTask.Settings = $setting
            $scheduledTask.Principal = $principal

Can you remove blank line?


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1191 at r1 (raw file):

            $scheduledTask.Principal = $principal

        } else {

Can you move } { to own lines? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#one-newline-before-braces for more info.


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1194 at r1 (raw file):

            $scheduledTask = $tempScheduledTask
        }
       

Can you remove trailing whitespace from lines? Suggest using an editor like VS Code and enabling the "files.trimTrailingWhitespace": true setting - it will then automatically correct this.


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1215 at r1 (raw file):

            Write-Verbose -Message ($script:localizedData.UpdateScheduledTaskMessage -f $TaskName, $TaskPath)
            $null = Set-ScheduledTask -InputObject $scheduledTask @registerArguments
        

Can you remove blank line?


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1216 at r1 (raw file):

            $null = Set-ScheduledTask -InputObject $scheduledTask @registerArguments
        
        } else {

Can you move } { to own lines? See https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#one-newline-before-braces for more info.


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1226 at r1 (raw file):

            Write-Verbose -Message ($script:localizedData.RegisterScheduledTaskMessage -f $TaskName, $TaskPath)
            Write-Verbose -Message ($TaskName)

This verbose line seems unnecessary - can it be removed?


Modules/xComputerManagement/DSCResources/MSFT_xScheduledTask/MSFT_xScheduledTask.psm1, line 1229 at r1 (raw file):

         
            $null = Register-ScheduledTask @registerArguments
        }       

Got some white space here to remove 😁


Tests/Unit/MSFT_xScheduledTask.Tests.ps1, line 324 at r1 (raw file):

                        }
                    }

Can blank line be removed?


Comments from Reviewable

Release of version 4.0.0.0 of xComputerManagement
@PlagueHO
Copy link
Copy Markdown
Member

Hi @GitBadSanta - I'm going to begin renaming this repo over to ComputerManagementDsc, but I'd like to get your change in before that happens (otherwise it'll break your PR a lot). See #119. So let me know if you're able to get the final tweaks on this one through so I can begin the rename process. Thank you 😁

@GitBadSanta
Copy link
Copy Markdown
Author

Hi @PlagueHO. Sorry for the delay, and thanks for the detailled review. Hopefully all should be ok now.

@PlagueHO
Copy link
Copy Markdown
Member

@GitBadSanta - Doh! I forgot to get you to add an entry to the CHANGELOG.MD detailing the change. Are you able to add something to that? Other than that it all looks good. Sorry about taking so long too BTW!


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@PlagueHO
Copy link
Copy Markdown
Member

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


CHANGELOG.md, line 6 at r3 (raw file):

- xScheduledTask:
  - Update existing Scheduled Task using SetScheduleTask instead of UnRegister/Register - See [Issue #134](https://github.com/PowerShell/xComputerManagement/issues/134).

Thanks @GitBadSanta - one problem though: This violates the 80 char markdown line length rule: https://ci.appveyor.com/project/PowerShell/xcomputermanagement/build/1.9.390.0#L2937

Could you just make this line less than 80 chars. e.g.

- Update existing Scheduled Task using SetScheduleTask
  instead of UnRegister/Register - See [Issue #134](https://github.com/PowerShell/xComputerManagement/issues/134).

Comments from Reviewable

@PlagueHO
Copy link
Copy Markdown
Member

PlagueHO commented Mar 2, 2018

@GitBadSanta - if we can get this one merged then I can start the renaming process on this module. I haven't completed it yet because I didn't want to mess with your PR.

@GitBadSanta
Copy link
Copy Markdown
Author

@PlagueHO : hope it's ok now.
Sorry but it seems you work during the week end, and me during the week. That's not exactly the best schedule to move on quickly.

@PlagueHO
Copy link
Copy Markdown
Member

PlagueHO commented Mar 5, 2018

@GitBadSanta - no worries at all @GitBadSanta - I'm just happy for the assistance and work you're putting in!

@PlagueHO
Copy link
Copy Markdown
Member

PlagueHO commented Mar 5, 2018

:lgtm:


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@PlagueHO PlagueHO merged commit b6f03bc into dsccommunity:dev Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants