xCluster*: Added support for distinguished cluster name #258

IlleNilsson · 2021-04-18T13:32:26Z

Pull Request (PR) description

Adds support for distinguished cluster name so one can place the cluster node in specific OU

This Pull Request (PR) fixes the following issues

Issue #256

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

codecov · 2021-04-18T13:42:41Z

Codecov Report

Merging #258 (bd656ae) into main (faa9aa3) will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #258   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files         8      9    +1     
  Lines       611    630   +19     
===================================
+ Hits        611    630   +19

Impacted Files	Coverage Δ
source/DSCResources/DSC_Cluster/DSC_Cluster.psm1	`100% <100%> (ø)`
...usterPreferredOwner/DSC_ClusterPreferredOwner.psm1	`100% <100%> (ø)`
...urces/DSC_ClusterProperty/DSC_ClusterProperty.psm1	`100% <100%> (ø)`
...sources/DSC_WaitForCluster/DSC_WaitForCluster.psm1	`100% <100%> (ø)`
...erClusterDsc.Common/FailoverClusterDsc.Common.psm1	`100% <100%> (ø)`

johlju · 2021-04-22T17:27:59Z

I need to have time to look into this change. I will try to get to it as soon as possible, but having a lot on my plate at the moment.

johlju · 2021-04-22T17:46:08Z

If you can add unit tests to the changes in the PR so they changes is being tested. That would help as I need that to merge the code.

dennisl68-castra · 2021-09-23T13:49:53Z

@IlleNilsson Could you please have a look at the failed Unit tests?
They don't seem very hard to fix...

IlleNilsson · 2021-09-23T14:07:25Z

I do not know where to start. But it would be a great opportunity for you to familiarize with the DSC community.

…

________________________________ From: dennisl68-castra ***@***.***> Sent: Thursday, September 23, 2021 3:50:03 PM To: dsccommunity/xFailOverCluster ***@***.***> Cc: Ilian Nilsson ***@***.***>; Mention ***@***.***> Subject: Re: [dsccommunity/xFailOverCluster] xCluster*: Added support for distinguished cluster name (#258) @IlleNilsson<https://github.com/IlleNilsson> Could you please have a look at the failed Unit tests? They don't seem very hard to fix... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#258 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AID6F4BH54SWIVJX73CNRIDUDMWAXANCNFSM43EHIDZA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

dennisl68-castra · 2021-09-23T14:11:56Z

@IlleNilsson Perhaps you could review the failed Unit Tests in this Pull Request (look at the Details of each failed stage of Review requested) and remedy the failures of your request and then push your code again for a new review?

IlleNilsson · 2021-09-23T14:18:59Z

I already communicated with the community and stated that I can fix the actual problem but I’m not skilled enough to fix tests.

…

________________________________ From: dennisl68-castra ***@***.***> Sent: Thursday, September 23, 2021 4:12:07 PM To: dsccommunity/xFailOverCluster ***@***.***> Cc: Ilian Nilsson ***@***.***>; Mention ***@***.***> Subject: Re: [dsccommunity/xFailOverCluster] xCluster*: Added support for distinguished cluster name (#258) @IlleNilsson<https://github.com/IlleNilsson> Perhaps you could review the failed Unit Tests in this Pull Request (look at the Details of each failed stage of Review requested) and remedy the failures of your request and then push your code again for a new review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#258 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AID6F4ECB7KGL3W4IX5R7VLUDMYTPANCNFSM43EHIDZA>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

dennisl68-castra · 2021-09-23T15:00:56Z

@johlju The build isn't available anymore so I can't review the failed tests...
I'll try to find some of the issues by hand and then pushing the changes to trigger new builds and tests...

dennisl68-castra · 2021-09-26T08:02:58Z

If you can add unit tests to the changes in the PR so they changes is being tested. That would help as I need that to merge the code.

Ok, so what's missing now is additional unit tests for the added functionality?
I think I can have a resource available for that in a couple of weeks on our side (I don't know how to write proper Unit tests myself yet)...

dennisl68-castra · 2021-11-16T18:47:47Z

As we will drop the use of xCluster, I will not pursue this Pull Request.

johlju

I have added a unit test for this change that can be extended, see other comment.

Reviewed 1 of 12 files at r1, 10 of 14 files at r3, all commit messages.
Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @IlleNilsson and @johlju)

a discussion (no related file):
This code do have two disadvantage

it is possible to create two clusters with the same name in the same configuration, e.g. by passing 'CLUSTER' and 'CN=CLUSTER'
it is possible to manually move the cluster to another location at it will not be moved back to the desired location since the location (DN) is not enforced with this change.

The first one we might have to live with, but can we resolve the second, and if so, is this the correct resource to enforce location?

a discussion (no related file):
Since we do not have integration tests, has anyone run the changed resources so it do work?

source/Modules/FailoverClusterDsc.Common/FailoverClusterDsc.Common.psm1 line 14 at r3 (raw file):

        Distinguished Name to be converted to a Simple Name
#>
function Convert-DistinguishedNameToSimpleName

Suggest changing the function to this. But on top of that this function must also handle any special characters that are all allowed in a distinguished name and make tests that validates those, see more information: https://ldap.com/ldap-dns-and-rdns/

It should probably also fail if a DN was passed but did not start with CN=.

Suggestion:

function Convert-DistinguishedNameToSimpleName
{
    [Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSUseDeclaredVarsMoreThanAssignments', 'returnValue')]
    [CmdletBinding()]
    [OutputType([System.String])]
    param
    (
        [Parameter(Mandatory = $true)]
        [System.String]
        $DistinguishedName
    )

    $returnValue = $DistinguishedName

    if ($DistinguishedName -match '^([c|C][n|N])=(.*?(?=,))')
    {
        $returnValue = $matches[2]
    }

    return $returnValue
}

johlju

Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @IlleNilsson and @johlju)

a discussion (no related file):

Previously, johlju (Johan Ljunggren) wrote…

This code do have two disadvantage

it is possible to create two clusters with the same name in the same configuration, e.g. by passing 'CLUSTER' and 'CN=CLUSTER'

it is possible to manually move the cluster to another location at it will not be moved back to the desired location since the location (DN) is not enforced with this change.

The first one we might have to live with, but can we resolve the second, and if so, is this the correct resource to enforce location?

What if we have DN in a separate property called OrganizationUnitDistinguishedName that holds the path to the OU? Then we resolve first item above, by combining the property Name and OrganizationUnitDistinguishedName we get the full DN, and then we correctly can only have one instance of one cluster name.
We will also be able to enforce the location if the property OrganizationUnitDistinguishedName is passes. If not passed then location is not enforced.

…Common

johlju · 2022-06-19T17:27:47Z

This PR is ready for someone to continue running with it. Let me know when it needs another review. 🙂

stale · 2022-07-10T05:08:22Z

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

johlju added the needs review The pull request needs a code review. label Apr 19, 2021

johlju self-requested a review April 19, 2021 18:20

dennisl68-castra mentioned this pull request Sep 23, 2021

How do I run Pester Unit tests locally before performing a Pull Request? #262

Closed

dennisl68-castra force-pushed the main branch from 7ec86bd to 2bd55e8 Compare September 24, 2021 16:45

johlju force-pushed the main branch 2 times, most recently from 9fc6c16 to eac0ffd Compare June 19, 2022 14:58

johlju self-requested a review June 19, 2022 16:19

johlju requested changes Jun 19, 2022

View reviewed changes

johlju self-requested a review June 19, 2022 16:26

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 Jun 19, 2022

IlleNilsson and others added 8 commits June 19, 2022 19:23

Added support for distinguished cluster name

8459a0d

Updated CHANGELOG with new feature

8e81876

Moved common functions to separate module

3bca268

refactor: ♻️ Added trailing lines

160c770

fix: 📝 Added a minimal strings.psd1 file in en-US to FailoverCluster.…

279e8d0

…Common

chore: 🎨 Fixed formating requirements

cc94b1d

Rename helper module to match new module name

8926f7d

Update CHANGELOG.md

9918ac3

johlju added 4 commits June 19, 2022 19:23

Fix named parameter

4e3e9d3

Remove changes in resources that does not use helper function

bfdd5ff

Add unit test

84a135a

Fix localization file name

85cac4b

johlju force-pushed the main branch from 410fa1f to 85cac4b Compare June 19, 2022 17:24

Fix changelog after release

bd656ae

stale bot added the abandoned The pull request has been abandoned. label Jul 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xCluster*: Added support for distinguished cluster name #258

xCluster*: Added support for distinguished cluster name #258

IlleNilsson commented Apr 18, 2021 •

edited by johlju

Loading

codecov bot commented Apr 18, 2021 •

edited

Loading

johlju commented Apr 22, 2021

johlju commented Apr 22, 2021

dennisl68-castra commented Sep 23, 2021

IlleNilsson commented Sep 23, 2021 via email

dennisl68-castra commented Sep 23, 2021

IlleNilsson commented Sep 23, 2021 via email

dennisl68-castra commented Sep 23, 2021 •

edited

Loading

dennisl68-castra commented Sep 26, 2021

dennisl68-castra commented Nov 16, 2021

johlju left a comment

johlju left a comment

johlju commented Jun 19, 2022

stale bot commented Jul 10, 2022

xCluster*: Added support for distinguished cluster name #258

Are you sure you want to change the base?

xCluster*: Added support for distinguished cluster name #258

Conversation

IlleNilsson commented Apr 18, 2021 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov bot commented Apr 18, 2021 • edited Loading

Codecov Report

johlju commented Apr 22, 2021

johlju commented Apr 22, 2021

dennisl68-castra commented Sep 23, 2021

IlleNilsson commented Sep 23, 2021 via email

dennisl68-castra commented Sep 23, 2021

IlleNilsson commented Sep 23, 2021 via email

dennisl68-castra commented Sep 23, 2021 • edited Loading

dennisl68-castra commented Sep 26, 2021

dennisl68-castra commented Nov 16, 2021

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jun 19, 2022

stale bot commented Jul 10, 2022

IlleNilsson commented Apr 18, 2021 •

edited by johlju

Loading

codecov bot commented Apr 18, 2021 •

edited

Loading

dennisl68-castra commented Sep 23, 2021 •

edited

Loading