BREAKING CHANGE: Implementing changes from PR #89

nyanhp commented 2 years ago

Pull Request (PR) description

Incorporating the changes from #89, also updated RDSessionCollection to support a list of session hosts. Also fixes #93

This Pull Request (PR) fixes the following issues

Closes #89
Fixes #93

Task list

[x] 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).
[x] Resource documentation added/updated in README.md.
[x] 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.
[x] Examples appropriately added/updated.
[x] Unit tests added/updated. See DSC Community Testing Guidelines.
[ ] Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
[x] New/changed code adheres to DSC Community Style Guidelines.

This change is

codecov[bot] commented 2 years ago

Codecov Report

Merging #103 (6c6319b) into master (a93e238) will increase coverage by 0%. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #103   +/-   ##
=====================================
  Coverage      89%    90%           
=====================================
  Files          10     10           
  Lines         509    562   +53     
=====================================
+ Hits          457    509   +52     
- Misses         52     53    +1

Files Changed	Coverage Δ
...RDSessionCollection/MSFT_xRDSessionCollection.psm1	`96% <100%> (-1%)`	:arrow_down:
...RDSessionDeployment/MSFT_xRDSessionDeployment.psm1	`97% <100%> (+2%)`	:arrow_up:

nyanhp commented 1 year ago

Hi @johlju , sorry for the huge delay. I've incorporated your changes and updated the tests to actually cover all changes I made.

johlju commented 1 year ago

@nyanhp can you please rebase this PR too? There are conflicts that need to be resolved. 🙂

nyanhp commented 1 year ago

@johlju done

CalvinRossSmith commented 1 year ago

@johlju and @nyanhp any updates on this?

johlju commented 1 year ago

It is waiting for me, but have been quite busy the last few months. Hopefully I can get to it eventually.

johlju commented 11 months ago

I try to review this weekend or next week.

nyanhp commented 11 months ago

@johlju first of all sorry for the code updates lately, we've had some additional testing at my customer. Regarding Should this remove session host with any other from of opt-in? Normally a resource should not touch things already configured on a node withut some form of opt-in. 🤔 Should we have a parameter Force or RemoveUnconfiguredSessionHosts that will be an opt-in? --> That would be better for users who don't want DSC to ensure a desired state. When I initially added the changes, me and my customer agreed that the configuration should always reflect the desired state. If e.g. session hosts are not configured, the collection is not in the desired state and they need to be removed. To be honest, this should ideally be done with the RDSDeployment as well... To be clear: The "Force" parameter would be used for both adding and removing servers, right? If that's the case, I'll add it (and more test cases) tomorrow.

Regarding the breaking change: Does it break things though? :) But I agree, it is a major update that could potentially mean trouble.

johlju commented 11 months ago

…configuration should always reflect the desired state. If e.g. session hosts are not configured, the collection is not in the desired state and they need to be removed.

I agree. I was just thinking that if we use a parallel with local users. If we add a user we don’t necessarily want to remove all others. But session host might be more like members of a local group. If we say that this user should only be member of the local group then all others should be removed. If session hosts are like members of a group I don’t see a need to change to use Force. 🤔

johlju commented 11 months ago

Regarding the breaking change: Does it break things though? :) But I agree, it is a major update that could potentially mean trouble.

Yeah. If an existing configuration will behave differently with this version then we usually make it a “breaking change” and bump major version so it is clear that this new version might behave differently from previous major versions. But then we agree. We bump major version. 😊

nyanhp commented 11 months ago

…configuration should always reflect the desired state. If e.g. session hosts are not configured, the collection is not in the desired state and they need to be removed.

I agree. I was just thinking that if we use a parallel with local users. If we add a user we don’t necessarily want to remove all others. But session host might be more like members of a local group. If we say that this user should only be member of the local group then all others should be removed. If session hosts are like members of a group I don’t see a need to change to use Force. 🤔

Nah, Force makes sense. As it is implemented now, the behavior would be the same as before: If no session collection exists, create it. If Force is set and session hosts differ but collection exists, add missing and remove surplus.

johlju commented 11 months ago

This looks good to me, I'm okay to merge this. If there are any issues they can be fixed in another PR. Okay with you @nyanhp ?

nyanhp commented 11 months ago

sounds good @johlju, thanks for the review and the comments!

dsccommunity / xRemoteDesktopSessionHost