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

SqlServerConfiguration: Added localization and improved verbose messages #1021

Merged
merged 4 commits into from
Jan 17, 2018

Conversation

johlju
Copy link
Member

@johlju johlju commented Jan 15, 2018

Pull Request (PR) description

This Pull Request (PR) fixes the following issues:
Fixes #1014
Fixes #605

Task list:

  • Change details added to Unreleased section of CHANGELOG.md?
  • Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
  • Examples appropriately updated?
  • New/changed code adheres to Style Guidelines?
  • Unit and (optional) Integration tests created/updated where possible?

This change is Reviewable

@johlju johlju added the needs review The pull request needs a code review. label Jan 15, 2018
@codecov-io
Copy link

codecov-io commented Jan 15, 2018

Codecov Report

Merging #1021 into dev will increase coverage by <1%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##            dev   #1021    +/-   ##
=====================================
+ Coverage    97%     97%   +<1%     
=====================================
  Files        32      32            
  Lines      3899    3916    +17     
=====================================
+ Hits       3812    3830    +18     
+ Misses       87      86     -1

@johlju
Copy link
Member Author

johlju commented Jan 15, 2018

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.schema.mof, line 9 at r1 (raw file):

    [Required, Description("The desired value of the SQL configuration option.")] SInt32 OptionValue;
    [Write, Description("Determines whether the instance should be restarted after updating the configuration option.")] Boolean RestartService;
    [Write, Description("The length of time, in seconds, to wait for the service to restart. Default is 120 seconds.")] UInt32 RestartTimeout;

We should fix this in the README.md as well.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jan 16, 2018

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.psm1, line 161 at r1 (raw file):

    if ($option.IsDynamic -eq $true)
    {
        Write-Verbose -Message (

This message should be above the if-block. Here it should instead say that a restart was not necessary.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.psm1, line 201 at r1 (raw file):

    .PARAMETER RestartService
    *** Not used in this function ***

All descriptive text should be indented one step more. Throughout.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.psm1, line 262 at r1 (raw file):

    }

    ## return whether the value matches the desired state

Only one # for comments. Throughout.


Comments from Reviewable

- Fixed minor typos in comment-based help.
- Now the verbose message say what option is changing and to what value (issue dsccommunity#1014).
- Change the type of the parameter from SInt32 to UInt32.
- Added localization (issue dsccommunity#605).
@johlju johlju force-pushed the update-SqlServerConfiguration branch from c019f52 to f127bf1 Compare January 16, 2018 15:32
@johlju
Copy link
Member Author

johlju commented Jan 16, 2018

Reviewed 1 of 2 files at r2.
Review status: 3 of 7 files reviewed at latest revision, 5 unresolved discussions.


CHANGELOG.md, line 91 at r3 (raw file):

  - Now the verbose message say what option is changing and to what value
    ([issue #1014](https://github.com/PowerShell/SqlServerDsc/issues/1014)).
  - Change the type of the parameter from SInt32 to UInt32.

Should say the RestartTimeout parameter?


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.psm1, line 161 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This message should be above the if-block. Here it should instead say that a restart was not necessary.

Done.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.psm1, line 201 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

All descriptive text should be indented one step more. Throughout.

Done.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.psm1, line 262 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Only one # for comments. Throughout.

Done.


DSCResources/MSFT_SqlServerConfiguration/MSFT_SqlServerConfiguration.schema.mof, line 9 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

We should fix this in the README.md as well.

Done.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jan 16, 2018

Review status: 2 of 7 files reviewed at latest revision, 5 unresolved discussions.


CHANGELOG.md, line 91 at r3 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Should say the RestartTimeout parameter?

Done.


Comments from Reviewable

@johlju
Copy link
Member Author

johlju commented Jan 16, 2018

Reviewed 1 of 2 files at r2, 3 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@johlju johlju merged commit 54df342 into dsccommunity:dev Jan 17, 2018
@vors vors removed the needs review The pull request needs a code review. label Jan 17, 2018
@johlju johlju deleted the update-SqlServerConfiguration branch January 18, 2018 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants