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

DFSReplicationGroup - update PrimaryMember to write-only but not test #138

Merged
merged 8 commits into from
Apr 18, 2024

Conversation

Borgquite
Copy link
Contributor

@Borgquite Borgquite commented Apr 5, 2024

Pull Request (PR) description

Make PrimaryMember a write-only property that should not test

This Pull Request (PR) fixes the following issues

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 Reviewable

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96%. Comparing base (142b485) to head (eead69e).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #138   +/-   ##
===================================
  Coverage    95%    96%           
===================================
  Files         8      8           
  Lines      1233   1252   +19     
===================================
+ Hits       1183   1202   +19     
  Misses       50     50           
Files Coverage Δ
...pMembership/DSC_DFSReplicationGroupMembership.psm1 97% <ø> (-1%) ⬇️

@PlagueHO PlagueHO self-assigned this Apr 6, 2024
@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Apr 6, 2024
@Borgquite
Copy link
Contributor Author

Hey @PlagueHO - sorry, me again. Hope this is a quick one, let me know if you have any comments/suggestions :) Ta!

@PlagueHO
Copy link
Member

Hi @Borgquite - sorry about t the delay. Been at a vibe come conference all week so will try and get into this over the weekend!

@Borgquite
Copy link
Contributor Author

No worries @PlagueHO - any free time you have much appreciated :)

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.

Just minor style nit picks.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Borgquite)


CHANGELOG.md line 8 at r1 (raw file):

## [Unreleased]

### Fixed

Can you add a blank line after this one?


source/DSCResources/DSC_DFSReplicationGroupMembership/DSC_DFSReplicationGroupMembership.psm1 line 537 at r1 (raw file):

                ) -join '' )

            # See https://techcommunity.microsoft.com/t5/storage-at-microsoft/the-primary-member-in-dfs-replication/ba-p/423127

Could you change to a <# #> comment block?


source/DSCResources/DSC_DFSReplicationGroupMembership/DSC_DFSReplicationGroupMembership.schema.mof line 16 at r1 (raw file):

    [Write, Description("Specify if this content path should be read only.")] Boolean ReadOnly;
    [Write, Description("Specify if a member computer deletes files and folders immediately following inbound replication.")] Boolean RemoveDeletedFiles;
    [Write, Description("Used to configure this as the Primary Member. Every folder must have at least one primary member for initial replication to take place.  Only set during initial creation of membership.")] Boolean PrimaryMember;

Can you change to single space before Only?


source/DSCResources/DSC_DFSReplicationGroupMembership/README.md line 7 at r1 (raw file):
Can you move NB to be on a new paragraph and use:

`

Note: ...
`

Copy link
Contributor Author

@Borgquite Borgquite left a comment

Choose a reason for hiding this comment

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

No worries - all done :)

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


CHANGELOG.md line 8 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add a blank line after this one?

Done.


source/DSCResources/DSC_DFSReplicationGroupMembership/DSC_DFSReplicationGroupMembership.psm1 line 537 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you change to a <# #> comment block?

Done.


source/DSCResources/DSC_DFSReplicationGroupMembership/DSC_DFSReplicationGroupMembership.schema.mof line 16 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you change to single space before Only?

Done.


source/DSCResources/DSC_DFSReplicationGroupMembership/README.md line 7 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you move NB to be on a new paragraph and use:

`

Note: ...
`

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.

Sorry minor clarifications :D

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Borgquite)


source/DSCResources/DSC_DFSReplicationGroupMembership/DSC_DFSReplicationGroupMembership.psm1 line 537 at r1 (raw file):

Previously, Borgquite (Chris Hill) wrote…

Done.

Apologies, I meant like this:
<#
See https://techcommunity.microsoft.com/t5/storage-at-microsoft/the-primary-member-in-dfs-replication/ba-p/423127
Do not flag as a change required as flag is cleared after initial sync
#>


source/DSCResources/DSC_DFSReplicationGroupMembership/README.md line 7 at r1 (raw file):
Ah, I see Reviewable helpfully converted by example to MD.

Can you prepend:

> Note: The PrimaryMember flag is automatically cleared by DFS once an initial
> replication sync takes place, so is not tested by this resource.

This will make the MD format it.

Copy link
Contributor Author

@Borgquite Borgquite left a comment

Choose a reason for hiding this comment

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

No worries - all OK now?

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


source/DSCResources/DSC_DFSReplicationGroupMembership/DSC_DFSReplicationGroupMembership.psm1 line 537 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Apologies, I meant like this:
<#
See https://techcommunity.microsoft.com/t5/storage-at-microsoft/the-primary-member-in-dfs-replication/ba-p/423127
Do not flag as a change required as flag is cleared after initial sync
#>

Done.


source/DSCResources/DSC_DFSReplicationGroupMembership/README.md line 7 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Ah, I see Reviewable helpfully converted by example to MD.

Can you prepend:

> Note: The PrimaryMember flag is automatically cleared by DFS once an initial
> replication sync takes place, so is not tested by this resource.

This will make the MD format it.

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:

Thanks @Borgquite !

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Borgquite)

@PlagueHO PlagueHO merged commit 7f1a53d into dsccommunity:main Apr 18, 2024
11 checks passed
@Borgquite Borgquite deleted the fix-primarymember branch April 19, 2024 10:15
@Borgquite
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Mission Aviation Fellowship International"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review The pull request needs a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xDFSReplicationGroup - test-targetresource always returns PrimaryMember as true
2 participants