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

SmbShare: Bugfix to enable change of the path when the share already exists. #286

Merged
merged 9 commits into from
Nov 16, 2019

Conversation

danielboth
Copy link
Member

@danielboth danielboth commented Oct 17, 2019

Pull Request (PR) description

This changes enables the SmbShare resource to change the path on an existing share. To ensure a user does this consciously, a parameter Force is added to the resource. Force needs to be true for the resource to drop and recreate the share on the new path.

If the user did not set Force to true, a warning will be displayed stating the resource cannot get into desired state without setting Force to true.

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 in the 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

@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #286 into dev will increase coverage by <1%.
The diff coverage is 90%.

Impacted file tree graph

@@         Coverage Diff          @@
##            dev   #286    +/-   ##
====================================
+ Coverage    88%    88%   +<1%     
====================================
  Files        14     14            
  Lines      1469   1478     +9     
====================================
+ Hits       1294   1302     +8     
- Misses      175    176     +1

@danielboth
Copy link
Member Author

Hi @PlagueHO, on another branch I have a fix for #284 (https://github.com/danielboth/ComputerManagementDsc/tree/SmbShare-ScopeName-support). That one builds on the features I've added in this branch, would you like to review those in one go, or that we first merge this one?

@PlagueHO PlagueHO self-requested a review October 23, 2019 19:55
@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Oct 23, 2019
@PlagueHO
Copy link
Member

Hi @danielboth - can you resolve the conflicts on this one?

My feeling is to complete this PR first (as it is a bug fix) and then we'll get the feature one through.

@danielboth
Copy link
Member Author

Good, I got the same feeling (the reason I splitted them to start with), so let's get this one through first. Just resolved the merge conflict.

@PlagueHO PlagueHO closed this Nov 14, 2019
@PlagueHO PlagueHO reopened this Nov 14, 2019
@PlagueHO
Copy link
Member

Closing/Reopening to kick CI again.

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.

Reviewed 6 of 8 files at r1, 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielboth)


DSCResources/MSFT_SmbShare/MSFT_SmbShare.schema.mof, line 18 at r3 (raw file):

    [Write, Description("Specifies which accounts is granted read permission to access the SMB share.")] String ReadAccess[];
    [Write, Description("Specifies if the SMB share should be added or removed."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
    [Write, Description("Specifies if the SMB share is allowed to be dropped and recreated.")] Boolean Force;

This text should match the value in the PSM1. Can you add the part in the parenthesis from the parenthesis?


Examples/Resources/SmbShare/4-SmbShare_RecreateShare_Config.ps1, line 3 at r3 (raw file):

<#PSScriptInfo
.VERSION 1.0.0
.GUID d0847694-6a83-4f5b-bf6f-30cb078033bc

Can you generate a new GUID? This one looks like it's already used by one of the other examples.

Copy link
Member Author

@danielboth danielboth 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: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @PlagueHO)


DSCResources/MSFT_SmbShare/MSFT_SmbShare.schema.mof, line 18 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This text should match the value in the PSM1. Can you add the part in the parenthesis from the parenthesis?

Done.


Examples/Resources/SmbShare/4-SmbShare_RecreateShare_Config.ps1, line 3 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you generate a new GUID? This one looks like it's already used by one of the other examples.

Is this GUID used? Can you explain why it's important for this one to be unique?

Anyway, updated now, just like to know the why.

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.

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


Examples/Resources/SmbShare/4-SmbShare_RecreateShare_Config.ps1, line 3 at r3 (raw file):

Previously, danielboth (Daniël) wrote…

Is this GUID used? Can you explain why it's important for this one to be unique?

Anyway, updated now, just like to know the why.

The Guid represents the unique id of the script (not the filename as you might expect). The intent was that this was how PS Gallery would uniquely identify a script. Whether it does or not remains to be seen (it may just use the filename) - but it is still best to follow the intent of the field.

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:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@PlagueHO PlagueHO merged commit 1a5f96b into dsccommunity:dev Nov 16, 2019
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.

SmbShare: If the Share exists but with the wrong Path, the resource does not update the path
3 participants