-
Notifications
You must be signed in to change notification settings - Fork 225
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
xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership: Localization updates #663
xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership: Localization updates #663
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #663 +/- ##
====================================
+ Coverage 97% 97% +<1%
====================================
Files 30 30
Lines 3228 3232 +4
====================================
+ Hits 3143 3147 +4
Misses 85 85 |
I will try to get to this on on Sunday, or Monday at the latest. |
Will wait to review the rest of the comments until you respond to my responses :) Review status: all files reviewed at latest revision, 12 unresolved discussions. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 79 at r2 (raw file): Previously, johlju (Johan Ljunggren) wrote…
Without the -Verbose flag, it did not show verbosity when the Pester tests were executed. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 135 at r2 (raw file): Previously, johlju (Johan Ljunggren) wrote…
I had considered using that, but I don't think this is an invalid operation. To me this is more of a security error in the fact that permissions are missing. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 520 at r2 (raw file): Previously, johlju (Johan Ljunggren) wrote…
For the ArgumentName? The parameter 'ServerObject' is covered in the error message. Maybe I'm not understanding how New-InvalidArgumentException is intended to be used. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/en-US/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.strings.psd1, line 11 at r2 (raw file): Previously, johlju (Johan Ljunggren) wrote…
Yeah, I was trying to differentiate between not find any databases and not finding a subset. Concur that it's redundant. Comments from Reviewable |
Reviewed 2 of 2 files at r3. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 79 at r2 (raw file): Previously, randomnote1 (Dan Reist) wrote…
That is the correct behaviour when Pester runs. It's wrong of us to have forced the verbose messages before. They will be shown correctly if the user adds -Verbose to for example Start-DscConfiguration. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 135 at r2 (raw file): Previously, randomnote1 (Dan Reist) wrote…
Then this looks good to me. Leave it as-is. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 520 at r2 (raw file): Previously, randomnote1 (Dan Reist) wrote…
That particular helper function will return a standard parameter error message saying that the specific argument/parameter (which is provided in ArgumentName) was not correct and the error message should explain why and how to resolve. https://msdn.microsoft.com/en-us/library/sxykka64(v=vs.110).aspx Comments from Reviewable |
Reviewed 3 of 3 files at r4. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 79 at r2 (raw file): Previously, randomnote1 (Dan Reist) wrote…
I haven't thought about that. I haven't seen anyone do that, testing verbose output, maybe because it's optional, and because it is not a specific code path. Do you see a good reason to add that? Otherwise I think we leave that out for now. 😄 Comments from Reviewable |
@randomnote1 Waiting for you to reply or acknowledge that last comment. After that we can merge. |
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 79 at r2 (raw file): Previously, johlju (Johan Ljunggren) wrote…
Verbose messages are how we convey information to the end user. Often time the verbose messages combined with the error messages allow the end user to more easily troubleshoot their configurations. Additionally, with the focus on localization, it might be smart to test the localization paths. I don't really want to add the additional work, but it should ensure higher quality work in the end. At the very least, something to discuss on the calls! Comments from Reviewable |
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful. DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 79 at r2 (raw file): Previously, randomnote1 (Dan Reist) wrote…
I agree with you. And I don't mind the extra work, if it gives us higher quality. Could you please submit an issue to DscResources repo so we could reference that in the calls? In the end it's probably up to each repo. But maybe we could get a common test guideline around this. Comments from Reviewable |
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful. Comments from Reviewable |
DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership/MSFT_xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.psm1, line 79 at r2 (raw file): Previously, johlju (Johan Ljunggren) wrote…
Opened issue PowerShell/DscResources#290 Comments from Reviewable |
Pull Request (PR) description
Updated localization for xSQLServerAlwaysOnAvailabilityGroupDatabaseMembership.
This Pull Request (PR) fixes the following issues:
Fixes #660
Task list:
This change is