Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScheduledTask: Add support for Group Managed Service Accounts #174

Merged
merged 10 commits into from
Sep 25, 2018

Conversation

danielboth
Copy link
Member

@danielboth danielboth commented Jul 30, 2018

Pull Request (PR) description
This PR enables the use of Group Managed Service Accounts in the ScheduledTask resource. It enables the user to setup a scheduled task using a AD ServiceAccount and will set the task to 'Run whether user is logged on or not'.

This Pull Request (PR) fixes the following issues:
Fixes #111

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

@codecov-io
Copy link

codecov-io commented Jul 30, 2018

Codecov Report

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

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #174    +/-   ##
===================================
+ Coverage   90%    90%   +<1%     
===================================
  Files        7      7            
  Lines      715    724     +9     
===================================
+ Hits       648    657     +9     
  Misses      67     67

@johlju johlju added the needs review The pull request needs a code review. label Jul 31, 2018
@johlju
Copy link
Member

johlju commented Sep 3, 2018

@PlagueHO do you have time to review this one? This one seems it falls under the same functionality as the PR and the comment dsccommunity/xPSDesiredStateConfiguration#441 (comment)

@PlagueHO
Copy link
Member

PlagueHO commented Sep 4, 2018

How did I miss this! @danielboth - are you able to fix the merge conflicts? Sorry about the delay. I'll get onto reviewing this.

@danielboth
Copy link
Member Author

Hi @PlagueHO, I pulled the latest dev to get my branch up to date a few days ago, no merge conflicts fortunately :). Can you continue the review?

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great stuff @danielboth - sorry about taking so long. Pretty much faultless - just the tiniest of style nitpicks in the tests. I'll merge as soon as they're done. Thanks again!

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielboth)


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1733 at r1 (raw file):

            Context 'When a scheduled task is created using a Group Managed Service Account' {

Can you remove this blank line here?


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1803 at r1 (raw file):

                }

                It 'Test should return true if the task is in desired state and given gMSA user in DOMAIN\User$ format' {

Can you change to 'Should return true when test is called if...' - We try to make It descriptions start with 'Should'


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1809 at r1 (raw file):

                $testParameters.ExecuteAsGMSA = 'gMSA$@domain.fqdn'

                It 'Test should return true if the task is in desired state and given gMSA user in UPN format' {

Can you change to 'Should return true when test is called if the task...' - We try to make It descriptions start with 'Should'

Copy link
Member Author

@danielboth danielboth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @PlagueHO)


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1733 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you remove this blank line here?

Done.


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1803 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you change to 'Should return true when test is called if...' - We try to make It descriptions start with 'Should'

Done.


Tests/Unit/MSFT_ScheduledTask.Tests.ps1, line 1809 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you change to 'Should return true when test is called if the task...' - We try to make It descriptions start with 'Should'

Done.

Copy link
Member

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@PlagueHO PlagueHO merged commit cb145a5 into dsccommunity:dev Sep 25, 2018
@johlju johlju removed the needs review The pull request needs a code review. label Sep 25, 2018
@danielboth danielboth deleted the ScheduledTask-gMSA-support branch September 25, 2018 20:51
@ImMattAsh
Copy link

@PlagueHO - sorry if you're not the person to ask, but I was just curious if there is a planned release to PSGallery where this feature will be available. Looks like last version of 4.1.0.0 was prior to this pull request being closed. If this information can be found elsewhere, I apologize. Please feel free to point me in the direction. Thanks!

@PlagueHO
Copy link
Member

Hi @ImMattAsh - the latest version in the PSGallery is 6.0.0.0 I think (there was also a 5.0.0.0, 5.1.0.0, 5.2.0.0): https://www.powershellgallery.com/packages/ComputerManagementDsc/6.0.0.0. Might have been a PS Gallery glitch? The best way to see what has been released month to month in the DSC Resource kit is by looking here: https://blogs.msdn.microsoft.com/powershell/tag/dsc-resource-kit/

@ImMattAsh
Copy link

@PlagueHO - fwiw, it was no glitch - I missed that the name changed from xComputerManagement to ComputerManagmentDsc - completely my mistake. Thanks again, and sorry for the confusion.

@PlagueHO
Copy link
Member

PlagueHO commented Jan 4, 2019

Ah right! Of course. Glad all working and no worries at all.

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.

xScheduledTask - missing option to configure tasks with gMSA accounts.
5 participants