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

UpdateServicesComputerTargetGroup: New resource - managing computer target groups #78

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Borgquite
Copy link
Contributor

@Borgquite Borgquite commented May 29, 2024

Pull Request (PR) description

A rebased pull for #60
It looks OK to me, waiting to see if it passes the Pester tests as these may need updating to support 5

This Pull Request (PR) fixes the following issues

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.
  • 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 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

@Borgquite
Copy link
Contributor Author

Borgquite commented May 29, 2024

Hey, @johlju. Trying to rebase per our discussion in #60. However the tests are failing for resources that I'm not touching, in a way that I haven't seen before - all saying ' Cannot find the Windows PowerShell data file 'MSFT_UpdateServicesApprovalRule.strings.psd1' in directory xxx' but the files they reference seem to appear in the module - feels like this is unrelated to my pull request. Can you advise?

I guess I may also need to update the pester tests for v5 similar to #74 but I think this issue needs resolving first.

@johlju
Copy link
Member

johlju commented May 30, 2024

Will have a look as soon as I have time.

@Borgquite
Copy link
Contributor Author

@johlju Quick reminder when you have a few minutes to check out the issue with the tests here :)

@johlju
Copy link
Member

johlju commented Jun 9, 2024

@Borgquite if you wouldn't mind reviewing PR #79 so I can merge it, then this PR should be able to run the tests again after it has been rebased. 🙂

@johlju johlju added the needs review The pull request needs a code review. label Jun 9, 2024
@johlju johlju requested a review from NicolasBn June 9, 2024 13:14
@johlju
Copy link
Member

johlju commented Jun 10, 2024

I merged PR #79 so this one can be rebased.

@johlju johlju added needs review The pull request needs a code review. and removed needs review The pull request needs a code review. labels Jun 10, 2024
@Borgquite
Copy link
Contributor Author

I merged PR #79 so this one can be rebased.

Thanks - I'll see if I can get the tests to resolve myself now, will keep you posted!

@Borgquite Borgquite force-pushed the ComputerTargetGroups branch from f4d54a7 to 2011471 Compare June 14, 2024 16:43
@johlju 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 Jul 15, 2024
@johlju
Copy link
Member

johlju commented Aug 30, 2024

Ping me when the tests passes on this one, I will try to get the time to review. 🙂

@Borgquite
Copy link
Contributor Author

Ping me when the tests passes on this one, I will try to get the time to review. 🙂

Thanks - doing some fairly major reworking of the rest of UpdateServicesDsc (it's missing tonnes of useful parameters, breaks a lot of DSC 'rules' from my PoV, e.g. as others have noted, certain parameters take effect even when they are 'unspecified') and it generally just needs some love. I'll keep you posted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Target Groups Creation
3 participants