Fix mismatch between user and builtinaccount #387

nickgw · 2022-05-21T21:24:30Z

Currently ScheduledTask conflates the param BuiltInAccount and *-ScheduledTask cmdlet's user parameter. This fixes that.

Pull Request (PR) description

Currently ScheduledTask conflates the param BuiltInAccount and *-ScheduledTask cmdlet's user parameter. This fixes that.

This Pull Request (PR) fixes the following issues

Fixes ScheduledTask: Get-TargetResource returns wrong account for BuiltInAccount parameter #385

Task list

Added an entry to the change log under the Unreleased section of the
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Community Testing Guidelines.
Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
New/changed code adheres to DSC Community Style Guidelines.

This change is

…ltinaccount

codecov · 2022-06-03T14:40:00Z

Codecov Report

Merging #387 (73dce2d) into main (3aea926) will increase coverage by 0%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #387   +/-   ##
===================================
  Coverage    90%    90%           
===================================
  Files        17     17           
  Lines      1716   1726   +10     
===================================
+ Hits       1554   1564   +10     
  Misses      162    162

Impacted Files	Coverage Δ
...Resources/DSC_ScheduledTask/DSC_ScheduledTask.psm1	`87% <100%> (+<1%)`	⬆️

nickgw · 2022-06-03T14:43:18Z

@PlagueHO This should be good to go now, when you get a chance to check it out!

ShawnHardwick · 2022-06-20T20:18:29Z

Should the MOF schema be updated to match this as well?

https://github.com/dsccommunity/ComputerManagementDsc/blob/main/source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.schema.mof#L16

nickgw · 2022-06-20T21:41:24Z

Should the MOF schema be updated to match this as well?

https://github.com/dsccommunity/ComputerManagementDsc/blob/main/source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.schema.mof#L16

The inputs don't actually change. The code was comparing the wrong properties.

PlagueHO · 2022-06-21T07:37:21Z

Hi @nickgw - sorry was tied up on other things. I'll get onto this close to the end of the week. Just need to try and avoid breaking Windows Server 2016 (unfortunately we can't test on it at the moment unless we get something like TK going in Azure).

nickgw · 2022-07-08T21:09:58Z

Hey @PlagueHO just bumping this (and my related comments in the issue) when you get a chance!

PlagueHO

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nickgw)

CHANGELOG.md line 7 at r1 (raw file):

## [Unreleased]
### Fixed

Can you add a blank line above this line?

source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1 line 832 at r1 (raw file):

                instead?
            #>
            $username = Set-DomainNameInAccountName -AccountName SYSTEM -DomainName 'NT AUTHORITY'

Nit: Just for consistency, can you put single quotes around SYSTEM?

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1784 at r1 (raw file):

                }

                # It 'Should Disregard ExecuteAsCredential and Set User to the BuiltInAccount' {

If a test isn't verifying the correct thing, we should remove it rather than commenting. However, in this case, I think the test should be adjusted to reflect the actual function.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1791 at r1 (raw file):

                # }

                $testParameters.Add('LogonType', 'Password')

Can you also add some additional tests here to verify the changed functionallity?

…/nickgw/ComputerManagementDsc into ngermany_fixuservsbuiltinaccount

nickgw · 2022-10-29T19:07:59Z

@PlagueHO Do you mind taking another look at this? I've added another test.

nickgw

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md line 7 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a blank line above this line?

Done.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1784 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

If a test isn't verifying the correct thing, we should remove it rather than commenting. However, in this case, I think the test should be adjusted to reflect the actual function.

Done.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1791 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you also add some additional tests here to verify the changed functionallity?

Done.

nickgw · 2022-11-19T22:09:06Z

@PlagueHO do you mind taking another look at this? I'd love to close this off my list.

PlagueHO · 2022-11-20T21:38:41Z

Cool! Thanks @nickgw - I'll review tonight.

PlagueHO

Just some minor tweaks and we can merge!

Reviewed 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nickgw)

CHANGELOG.md line 7 at r1 (raw file):

Previously, nickgw (Nick G) wrote…

Done.

There is an extra f at the beginning of this line.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1774 at r4 (raw file):

            Context 'When a scheduled task is created using a Built In Service Account' {
                #

I think this # can be removed.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1809 at r4 (raw file):

                    }
                }
                #

I think this has can be removed.

…/nickgw/ComputerManagementDsc into ngermany_fixuservsbuiltinaccount

nickgw

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md line 7 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

There is an extra f at the beginning of this line.

Done.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1774 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I think this # can be removed.

Done.

tests/Unit/DSC_ScheduledTask.Tests.ps1 line 1809 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I think this has can be removed.

Done.

Code quote:

When a scheduled task is created using a Built In Service Accoun

nickgw · 2022-11-22T16:44:33Z

@PlagueHO I think I hit all your comments!

PlagueHO

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickgw)

nickgw added 6 commits May 21, 2022 17:18

Fix mismatch between user and builtinaccount

99bcac5

Updating changelog

5feb6fd

Dont exceed 100char

b009bae

remove test

a49b884

remove bad whitespce, handle when logontype is set incorrectly

14b6455

combine ifs

874ca5e

PlagueHO added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label May 23, 2022

nickgw added 3 commits June 3, 2022 09:54

Merge remote-tracking branch 'origin/main' into ngermany_fixuservsbui…

22914f4

…ltinaccount

Fixing builtinuser assignment

49a6401

updating test

ee9648b

PlagueHO added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 21, 2022

PlagueHO requested changes Jul 16, 2022

View reviewed changes

nickgw added 9 commits July 23, 2022 15:18

updating code

fdc5381

Uncommenting out test

93b7a15

Trigger CI

dfb4163

Pass User too

a6edb9c

bolster test

dece569

Add tests

674b7c5

Update test to use test-target resource

a05afba

moving get-scheduledtask up

666b530

trying to mock schdtask

ede5740

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Oct 5, 2022

nickgw added 5 commits October 10, 2022 15:13

remove erroneous f

083474a

adding removed tests

7c451cf

add new test

d1ba4ee

remove bad tests

ed04ba8

Merge branch 'ngermany_fixuservsbuiltinaccount' of https://github.com…

5e56c3b

…/nickgw/ComputerManagementDsc into ngermany_fixuservsbuiltinaccount

nickgw commented Nov 1, 2022

View reviewed changes

nickgw requested a review from PlagueHO November 6, 2022 00:20

Merge branch 'main' into ngermany_fixuservsbuiltinaccount

a9e44e2

PlagueHO added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Nov 22, 2022

PlagueHO requested changes Nov 22, 2022

View reviewed changes

nickgw added 2 commits November 22, 2022 10:17

Merge branch 'ngermany_fixuservsbuiltinaccount' of https://github.com…

71ae2a0

…/nickgw/ComputerManagementDsc into ngermany_fixuservsbuiltinaccount

Updating based on review

73dce2d

nickgw commented Nov 22, 2022

View reviewed changes

nickgw requested a review from PlagueHO November 22, 2022 18:53

PlagueHO approved these changes Nov 23, 2022

View reviewed changes

PlagueHO merged commit 6c3f2fd into dsccommunity:main Nov 23, 2022

nickgw deleted the ngermany_fixuservsbuiltinaccount branch November 23, 2022 14:47

johlju removed the needs review The pull request needs a code review. label Feb 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mismatch between user and builtinaccount #387

Fix mismatch between user and builtinaccount #387

nickgw commented May 21, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading

nickgw commented Jun 3, 2022

ShawnHardwick commented Jun 20, 2022

nickgw commented Jun 20, 2022

PlagueHO commented Jun 21, 2022

nickgw commented Jul 8, 2022

PlagueHO left a comment

nickgw commented Oct 29, 2022

nickgw left a comment

nickgw commented Nov 19, 2022

PlagueHO commented Nov 20, 2022

PlagueHO left a comment

nickgw left a comment

nickgw commented Nov 22, 2022

PlagueHO left a comment

Fix mismatch between user and builtinaccount #387

Fix mismatch between user and builtinaccount #387

Conversation

nickgw commented May 21, 2022 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov bot commented Jun 3, 2022 • edited Loading

Codecov Report

nickgw commented Jun 3, 2022

ShawnHardwick commented Jun 20, 2022

nickgw commented Jun 20, 2022

PlagueHO commented Jun 21, 2022

nickgw commented Jul 8, 2022

PlagueHO left a comment

Choose a reason for hiding this comment

nickgw commented Oct 29, 2022

nickgw left a comment

Choose a reason for hiding this comment

nickgw commented Nov 19, 2022

PlagueHO commented Nov 20, 2022

PlagueHO left a comment

Choose a reason for hiding this comment

nickgw left a comment

Choose a reason for hiding this comment

nickgw commented Nov 22, 2022

PlagueHO left a comment

Choose a reason for hiding this comment

nickgw commented May 21, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading