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: Fixed execution account missing domain name #356

Merged
merged 7 commits into from
Apr 9, 2021

Conversation

jensotto
Copy link
Contributor

@jensotto jensotto commented Feb 9, 2021

Pull Request (PR) description

Improved PR #351 to include domain accounts.

This Pull Request (PR) fixes the following issues

Fixes #352
Possibly fixes or Improves #144

Task list

  • Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md in resource folder.
  • Resource parameter descriptions added/updated in 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 Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Feb 11, 2021
@PlagueHO
Copy link
Member

Hi @jensotto - thanks for submitting this. It looks like the HQRM tests are failing because an entry hasn't been added to the CHANGELOG.md: See the Update the Changelog section in: https://dsccommunity.org/guidelines/contributing/

@jensotto
Copy link
Contributor Author

It already contains a message for that change so I didn't add any further comments: Fixed issue with ExecuteAsCredential not returning fully qualified username on newer versions of Windows 10 and Windows Server 2019 - Fixes Issue #352.

@PlagueHO
Copy link
Member

Ah, I see. Unfortunately for the tests to pass you'll need to make a correction message.

@jensotto
Copy link
Contributor Author

Ok, I have mad a small change to the existing entry in changelog.

@PlagueHO
Copy link
Member

PlagueHO commented Apr 5, 2021

Hi @jensotto - sorry about the delays in getting to this. Can you resolve the conflicts and I'll review?

@jensotto
Copy link
Contributor Author

jensotto commented Apr 5, 2021

@PlagueHO conflicts resolved

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.

Thanks @jensotto - just some minor style tweaks to line length then can merge.

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jensotto)


source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1, line 1453 at r4 (raw file):

        if ($username -match "$ENV:COMPUTERNAME\\" -and $currentValues.ExecuteAsCredential)
        {
            $currentValues.ExecuteAsCredential = Set-DomainNameInAccountName -AccountName $currentValues.ExecuteAsCredential -DomainName $ENV:COMPUTERNAME

Can you reduce the line length of this line by using:

$currentValues.ExecuteAsCredential = Set-DomainNameInAccountName `
    -AccountName $currentValues.ExecuteAsCredential `
    -DomainName $ENV:COMPUTERNAME

source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1, line 1458 at r4 (raw file):

        if ($username -match "$ENV:USERDOMAIN\\" -and $currentValues.ExecuteAsCredential)
        {
            $currentValues.ExecuteAsCredential = Set-DomainNameInAccountName -AccountName $currentValues.ExecuteAsCredential -DomainName $ENV:USERDOMAIN

Can you reduce the line length of this line by using:

$currentValues.ExecuteAsCredential = Set-DomainNameInAccountName `
    -AccountName $currentValues.ExecuteAsCredential `
v    -DomainName $ENV:USERDOMAIN

Copy link
Contributor Author

@jensotto jensotto left a comment

Choose a reason for hiding this comment

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

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


source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1, line 1453 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you reduce the line length of this line by using:

$currentValues.ExecuteAsCredential = Set-DomainNameInAccountName `
    -AccountName $currentValues.ExecuteAsCredential `
    -DomainName $ENV:COMPUTERNAME

Done.


source/DSCResources/DSC_ScheduledTask/DSC_ScheduledTask.psm1, line 1458 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you reduce the line length of this line by using:

$currentValues.ExecuteAsCredential = Set-DomainNameInAccountName `
    -AccountName $currentValues.ExecuteAsCredential `
v    -DomainName $ENV:USERDOMAIN

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 r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jensotto)

@PlagueHO PlagueHO merged commit e181574 into dsccommunity:master Apr 9, 2021
@johlju johlju removed the needs review The pull request needs a code review. label Feb 26, 2023
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.

ScheduledTask - Integration Test Failure on WS2019
3 participants