Render job fails on local multilanguage site

I am using the CLI to render thumbs on my development machine (locally) via the render job.
This worked fine in a single-language site, however after enabling multi-language setup the rendered job stop working (rendering failed error).
The problem seems caused by the default language code that is appended to the site's URL by Kirby in multilanguage mod (en in my case):

The current value of Janitor's renderSiteUrl option is set to return site()->url() when running in CLI mode. That would be http://localhost:8000 in my case.
If I set renderSiteUrl to return http://localhost:8000/en, the render job succeeds.

site()->url() ends with the language code as far as i can tell (kirby 3.8.0) in both CLI and webserver.
what is different in my code if you set it explicitly it uses a Remote request instead of $page->render()/site()->visit()-combination.
my guess is that the cli does not have the right url because its not running on the webserver. it will just be /.
to confirm try this:
janitor --tinker

echo site()->url();

should return /.
to fix for the CLI use set the config value to this
'bnomei.janitor.renderSiteUrl' => fn() => '/',


adding a printer for query expressions - its a debug-only feature - makes it easier to analyze the queries

merged ad98141231c8871ed090b039dd657031854aa5c8


Something wrong w/ Darwin EE tgz

FYI, something seems wrong w/ the cpd-cli file in the 3.5.2 Darwin EE release of cpd-cli. The cpd-cli file in the tgz file is also a .tar.gz file itself and that file then seems to contain a new set of plugins/ and LICENCES, but no cpd-cli file thus making it unusable.

@tknorris We had to repackage it, please try now

confirmed fixed


Replace node-sass with sass

As per: https://github.com/dlmanning/gulp-sass/pull/786 but without the extraneous .idea and yarn.lock files

Why did you use old sass version 1.29.x when the current is 1.34.1?
Best use latest if we're already upgrading to sass over node-sass

Thanks all, close this to focus efforts on #802 which is more rounded at this point in time.


eventmux: use type key rather than anonymous struct in mux_test.go

This removes the risk for accidental key clashes.

#103
#102
#101
#100
#99  👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @jespert and the rest of your teammates on  Graphite

Merge activity

Oct 1, 7:03 AM EDT: @jespert merged this pull request with Graphite.


Add acceptance testing infrastructure with PollyJS.

Replaces #18 (I accidentally deleted my fork, and that made #18 an orphaned PR that I can no longer push to :weary: ).
Addressed all comments mentioned in that PR.

looks like the PR is missing a recording or two

@hjdivad - Ugh, I had to change things a bit to exclude user-agent from the matchers as well. @octokit/rest always appends Node version and platform information into the userAgent (even when the Octokit instance has a custom userAgent). See https://github.com/octokit/rest.js/issues/907#issuecomment-422217573 for a quick summary.

OK, I think this is finally ready for another review. Sorry for the delays.

@rwjblue I almost missed the superb branch name!

😄


New version DelayEmbeddings: 1.0.3

Registering: DelayEmbeddings
Repository: https://github.com/JuliaDynamics/DelayEmbeddings.jl
Version: v1.0.3
Commit: ebbd4446e7678ccc7c55ecf3c4320ace1fa8b314
Proposed by: @Datseris
Reviewed by: @Datseris
Reference: https://github.com/JuliaDynamics/DelayEmbeddings.jl/commit/ebbd4446e7678ccc7c55ecf3c4320ace1fa8b314#commitcomment-33412890

https://github.com/JuliaRegistries/General/pull/537#issuecomment-489368731


DE<>GIS: FacDB Collab - Metadata and Socrata Push

When we last spoke, we decided that with COLP pushing to -> Socrata, we should start widening the process out to include other products. Facilities Database was seen as a natural choice, since it has some complications, and we just built it, so it's fresh in our minds.
For this issue, DE would work with someone from GIS to create the metadata, and set up the push to Socrata.

(per comment here) closing this in favor of issues for batches of datasets


Agent on imported k3s cluster spamming for plans

The agent is getting a wait time of zero so it's hammering the rancher server for plans.
image: rancher/rancher-agent:master-2433-head

Verified with latest master-head - commit id: 547c8dd3d

On a k3s imported cluster

kubectl logs cattle-node-agent-w9smr  -n cattle-system | grep "plan monitor"
time="2020-03-14T03:07:45Z" level=info msg="Starting plan monitor, checking every 120 seconds"


Remove default features of http-types

Can you explain why?

@heaths I'm not familiar with the source code that much, so my change can break the lib, but at least the tests I run passes.
I use azure sdk in a project and saw useless (to me!) and old dependencies which are attached to the not-actively-maintained http-types. And this change is to avoid including them, if I understood correctly.

OK, now I investigated more :/
http-types has some 3-year-old changes on master which are not yet released! So my change here is meaningless and wrong!
Rust ecosystem should think about this old http-types package with ~10M downloads!

Related: #1644


Offer a way to override the control flow info provided by the architecture plugin

Consider the following case, in an obfuscated CTF challenge, the binary sets up an exception handler and then intentionally triggers an exception. The exception handler kicks in and fixes the instruction pointer. (I will attach a sample later.)
In this case, there is no way that binary ninja can figure out the control flow without user input. There are a few possible routes to fix this. However, I am thinking that we need to offer a way for the user to override the control flow info returned by the arch plugin (i.e., get_instruction_info). This way, not only the user can redirect the control flow in this case, they can even make more complex changes to the control flow according to their need.
A relevant feature is set_user_indirect_branches, which allows user to specify the call destination when there is an unresolved indirect branch, e.g., jmp rax. This can be thought as a special case of what I am proposing -- in addition to specifying the target of indirect branches, we also allow the user to change the control flow at any place.
Previously, in some cases, the way to deal with it is to patch the code to reflect the actual control flow. However, this is not ideal since patching might have adverse effect (e.g., not enough space), or the arch itself does not support patching at all. For these cases, if we can let the user to override the control flow from the UI/API, then there is no need to patch the code.
Note, there is a similar case where the disassembly wrongly continues after a function call. The case can be solved by marking the callee as no-return. It does not need the capacity this issue is asking for.

Relate to https://github.com/Vector35/binaryninja-api/issues/1024 and https://github.com/Vector35/binaryninja-api/discussions/2977
We should revisit these issues and think of whether we can improve the workflow or user experience

On a second thought, I think this is somehow relevant to the ArchitectureHook. We might even be able to develop a plugin on top of an arch hook


kitty: darwin-fix

Description of changes

Things done

Built on platform(s)

[x] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

@leiserfg
It definitely fixed the build error for me, though I can't completely validate the PR because I'm hitting https://github.com/NixOS/nixpkgs/issues/312692 in my environment (this is in test phase, past what you are fixing here).

I pulled your PR into my branch with an additional fix for the package (disabling some failing tests for darwin), and I can confirm that this works in my environment. (The branch for your reference: https://github.com/booxter/nixpkgs/tree/fix-kitty-and-darwin-tests)

I believe this is the relevant code causing the nerd fonts build failure: https://github.com/kovidgoyal/kitty/blob/8eb0b556b7651036f804b034c3960cd52a04c5e3/setup.py#L857
The error is not generated by fontconfig or fc-match.

I believe this is the relevant code causing the nerd fonts build failure: https://github.com/kovidgoyal/kitty/blob/8eb0b556b7651036f804b034c3960cd52a04c5e3/setup.py#L857
The error is not generated by fontconfig or fc-match. On macOS, the path to the font file appears to be resolved manually in the python setup/build script itself.

The font is either finded by fontconfig, or from a local folder, I just copied it to the folder and that works in all platforms.

Well, I guess @booxter already fixed the build in https://github.com/NixOS/nixpkgs/pull/338033#issuecomment-2316342258

 nix run -L github:booxter/nixpkgs/fix-kitty-and-darwin-tests\#kitty

worked for me, which is a combination of this PR and #338070 , however, at runtime kitty failed to select the correct font (which looks like a regression, since it works fine now):
[0.235] The font FiraCode Nerd Font was not found, falling back to Menlo

 nix run -L github:booxter/nixpkgs/fix-kitty-and-darwin-tests\#kitty

worked for me, which is a combination of this PR and #338070 , however, at runtime kitty failed to select the correct font (which looks like a regression, since it works fine now):
[0.235] The font FiraCode Nerd Font was not found, falling back to Menlo

I don't think it's related to this PR 'cause it's just adding a font file to a folder, it does not change the behavior of kitty in any way.

@leiserfg that error seems like what's described in this issue.

@rvolosatovs just use the regular FiraCode, kitty now comes with Nerd Font builtin.

This pr fixes the issue I originally raised on my device, thanks!

Thanks for fixing this 👍🏻

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/prs-already-reviewed/2617/1932

Result of nixpkgs-review pr 338033 run on aarch64-darwin 1

4 packages built:

kitty
kitty.kitten
kitty.shell_integration
kitty.terminfo

Result of nixpkgs-review pr 338033 run on x86_64-linux 1

4 packages built:

kitty
kitty.kitten
kitty.shell_integration
kitty.terminfo

Result of nixpkgs-review pr 338033 run on x86_64-darwin 1

4 packages built:

kitty
kitty.kitten
kitty.shell_integration
kitty.terminfo

Sorry @natsukium I'll follow the convention next time, I didn't know there was one for refactors.


[Feature Request] Support web3signer as an account manager

In a world where most of the existing validators have been created with hd wallets, this is currently impossible to migrate them to Vouch with slashing protection.
We can migrate them to local wallet accounts in Vouch, and under normal operations Vouch does not produce slashing events, but having a robust slashing protection would be better (especially if we benefit from it with the current validator client we are running and want to migrate to Vouch).
Supporting web3signer as an account manager would make it possible to migrate existing non-ditributed validators to Vouch while retaining slashing protection.
It would help increase Vouch's adoption.

If you have an HD seed you can create an HD wallet with the relevant accounts using ethdo and serve it using dirk to provide slashing protection.  Does this address the issue?

I spent some time looking at the web3signer, and think that rather than supporting it a better way to go will be to provide the ability to shard an existing private key.  This would leverage Dirk's slashing protection and not require shared infrastructure.

Great discussion, very interested in this feature being available!


MB-14347 Add closeout office column to moves

Jira ticket for this change
Summary
This PR adds a field to the Move table to allow association of a closeout office. It also includes test data (in devseed) to create a move with this field populated (Move Locator = CLSOFF).
Note: This story calls out not to name this field anything with the word 'location' in it. I went with closeoutOffice since the type of this field is a transportation office. Please let me know if anyone has opinions on this naming, I am happy to rename it if anyone feels strongly about it.
Testing/Validation
There are no changes visible in the app UI, this will be more of a code/db change type review. There is test data that can be loaded that will populate this new field in the database (see move w/ locator CSLOFF)

To Verify

[ ] New migration adds new column as expected
[ ] Move model is updated to include CloseoutOffice
[ ] Test data is present with the new field populated

Messages

:book:
:link: MB-14347

  Generated by :no_entry_sign: dangerJS against 432145a8e2e2aaedb1ab63be6e81c9a60897e664

Great feedback items @reggieriser, thank you! Both have now been addressed 😁


[Roadmap] Generate FIWARE Data Models Schema in JSON-LD as it happens with schema.org

See https://schema.org/version/3.9/schema.jsonld as a representation of the schema of Schema.org in JSON-LD.
It would be good to do the same with the Schema of FIWARE Data Models as @jason-fox did within the Tutorials see https://github.com/FIWARE/tutorials.Relationships-Linked-Data#relationships-with-linked-data
It has to be decided what would be the source for generating such schema in JSON-LD. One option is to use the current (annotated) JSON Schemas to do so.
We should not forget to make the JSON-LD content generated ready for multi-language.

this is how it is going to be solved


Input inside a dropdown is not working (can't type)

[x] I have searched the Issues to see if this bug has already been reported
[x] I have tested the latest version

Steps to reproduce
Code given bellow.
Current behavior
When I put a input inside the Dropdown header I can't type in that input box. But if I put it outside I can type. I can only increase using the up / down arrow in the right side of the input but I want to be able to type also.
Expected behavior
I should be able to type there. not only up/down arrow.
Context
So I have a dropdown and people can select area as numbers.

  <Dropdown className={styles.dropdown} label={'Area'}>
              <Dropdown.Header>

                  Area (sqft)

                  <TextInput
                    id="minimum-area"
                    placeholder="Minimum sqft"
                    onChange={(e: any) => {
                      setMinArea(e.target.value as number)
                      console.log(e.target.value)
                    }}
                    value={minArea}
                    sizing="sm"
                    step={100}
                    min={0}
                    type="number"
                  />

                  <TextInput
                    id="maximum-area"
                    onChange={(e: any) => setMaxArea(e.target.value as number)}
                    placeholder="Maximum sqft"
                    value={maxArea}
                    sizing="sm"
                    step={100}
                    min={0}
                    type="number"
                  />

              </Dropdown.Header>
              <Dropdown.Item className="hover:bg-transparent">

                  <Button
                    onClick={(e: any) => {
                      setMinArea(0)
                      setMaxArea(500)
                    }}
                    className="w-[46%]"
                  >
                    Reset
                  </Button>
                  <Button className="w-[46%]">Done</Button>

              </Dropdown.Item>
            </Dropdown>

Very similar issue here...
In my case, I have a component inside Dropdown.Item with its own state management, but changing this component state just doesn't work 🤡

Hi there, this combination of components is definitely not a use case I was aware of!
We use a third-party library called Floating UI to manage most of the logic in the Dropdown so I'm inclined that this is an issue with focus trap being controlled by our usage of Floating UI.
Your usage seems reasonable here so it definitely should work. Thanks for submitting this. We can take a look at this, but definitely feel free to try and contribute if you have the time - it would speed things along. The file in question is Dropdown.tsx.

The issue seems to be related to Floating UI as you said. The opened pull request should do the trick.

Folks, check the discussion at #941 - it seems that there is a misunderstanding related to the Dropdown and Popover components.

Same, I'm still having this issue.

Having this issue as well. Please let us know if there are any updates!

I am usingonKeyDown={(e) => e.stopPropagation()}. Is working for me.

![Screenshot 2023-10-27 at 15 13 33](https://github.com/themesberg/flowbite-react/assets/33840821/dbe0fd17-81f4-4151-ac98-e08dd933b0be)


chore: remove experimental for svelteFeatures.runes option parserOptions

Pull Request Test Coverage Report for Build<PHONE_NUMBER>0
Details

0 of 0   changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 89.862%

Totals

Change from base Build<PHONE_NUMBER>3:
0.0%

Covered Lines:
2998

Relevant Lines:
3217

💛  - Coveralls


BREAKING CHANGE: Updated xSQLServerConfiguration to support clustered instances (Fixes #144)

The xSQLServerConfiguration resource only supported changing configuration values for the local computer, not for clustered instances. This PR makes the following changes:

Updated MOF and resource parameters to be consistent with other resources [BREAKING CHANGE]
Now using helper function Connect-SQL
Created new internal restart function that supports clusters
Removed unused internal functions and objects
Created unit tests per template (Issue #92)
Updated README to reflect changes

This change is

Hi @nabrond, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Brandon Adams). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@nabrond Could you add '(Fixes #144)' to the PR title? Then it will auto-close that issue on merge.

Reviewed 3 of 5 files at r1, 1 of 1 files at r2.
Review status: 4 of 5 files reviewed at latest revision, 8 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 7 at r2 (raw file):

Import-Module $currentPath\..\..\xSQLServerHelper.psm1 -Verbose:$false -ErrorAction Stop

Function Get-TargetResource

function should be all lowercase

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 12 at r2 (raw file):

    [OutputType([System.Collections.Hashtable])]
    param(
        # Hostname of the SQL Server to be configured

Could you move these comments up above the function in help comment form

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 20 at r2 (raw file):

        [parameter(Mandatory = $false)]
        [System.String]
        $SQLInstanceName = "MSSQLSERVER",

Single quotes here

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 108 at r2 (raw file):

    if ($option.IsDynamic -eq $true)
    {
        New-VerboseMessage -Message "Configuration option has been updated."

single quotes

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r2 (raw file):

        [parameter(Mandatory = $false)]
        [System.String]
        $SQLInstanceName = "MSSQLSERVER",

single quotes

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r2 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context "Validate returned properties" {

single quotes

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r2 (raw file):

            $result = Get-TargetResource @desiredState

            It "Property: SQLServer" {

single quotes for all of these 'It' statements as well.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 132 at r2 (raw file):

    #region Non-Exported Function Unit Tests
    InModuleScope $script:DSCResourceName {
        Describe "Testing Restart-SqlService" {

single quotes

Comments from Reviewable

@johlju - If you get a chance would you mind looking over this PR for functionality?

@mbreakey3 absolutely I will do that

Reviewed 1 of 5 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 7 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> function should be all lowercase

OK

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 12 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Could you move these comments up above the function in [help comment form](https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#functions-have-comment-based-help)

OK

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 20 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Single quotes here

OK

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 108 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> single quotes

OK

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> single quotes

OK

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> single quotes

OK

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> single quotes for all of these 'It' statements as well.

OK

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 132 at r2 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> single quotes

OK

Comments from Reviewable

Awesome work!
I don't have a cluster in my lab at home, so have to verify the cluster bits tomorrow at work.
So I will continue the review tomorrow. But it put together some comments to start with. :smile:

Reviewed 2 of 5 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r3 (raw file):

        $SQLServer,

        [parameter(Mandatory = $false)]

Please change all [paramater to [Parameter (upper case 'P')
There are more than one that needs to change.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):

        [System.Boolean]
        $RestartService = $false

This parameter could be remove from the Get-method since it does not do anything. In the returned hash table you could just return $null. Less to test in the tests too.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 61 at r3 (raw file):

    if(!$option)
    {
        throw "Specified option '$OptionName' was not found!"

Please use New-TerminatingError here (helper function)

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 64 at r3 (raw file):

    }

    $returnValue = @{

You can return the hashtable directly instead of assigning it to a variable and returning the variable.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 128 at r3 (raw file):

    if(!$option)
    {
        throw "Specified option '$OptionName' was not found!"

Please use New-TerminatingError here (helper function). That will make the string message to be reused from the Get-method also. And allow localization in the future.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r3 (raw file):

    $sql.Configuration.Alter()

    if ($option.IsDynamic -eq $true)

Could we instead check if the running value $option.RunValue is different than config value, and if so restart the service if the user asked us to?
I think some config values are dynamic even if IsDynamic is set to false, and we don't want to restart the service unnecessary. Correct me if I'm wrong.
See remarks:
https://msdn.microsoft.com/en-us/library/microsoft.sqlserver.management.smo.configproperty.runvalue.aspx

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 145 at r3 (raw file):

    else
    {
        Write-Warning 'Configuration option has been updated. SQL Server restart is required!'

Could we change the test to something like "Configuration option has been updated. Manual restart of SQL Server  is required for the change to take effect!"
Bonus points :smile: : Create a new helper function like New-TerminatingError, i.e New-WarningMessage, to be able to use localization strings.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 193 at r3 (raw file):

    )

    $state = Get-TargetResource -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName -OptionName $OptionName -OptionValue $OptionValue -RestartService $RestartService

Could you use splatting on these parameters instead?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

        ## Get the cluster resources
        New-VerboseMessage -Message 'Getting cluster resource for SQL Server'
        $SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = 'SQL Server' AND Name LIKE '%$($ServerObject.ServiceName)%'"

Could you change this to $sqlService = .... And all other occurrences.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

        New-VerboseMessage -Message 'Getting cluster resource for SQL Server Agent'
        $AgentService = Get-WmiObject -Namespace root/MSCLuster -Class MSCluster_Resource -Filter "Type = 'SQL Server Agent' AND Name LIKE '%$($ServerObject.ServiceName)%'"

Could you change this to $agentService = .... And all other occurrences.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r3 (raw file):

        ## Stop the SQL Server resource
        New-VerboseMessage -Message 'SQL Server resource --> Offline'
        $SqlService.TakeOffline(120)

Could we make this a parameter in the set-method instead of using a hard-coded value?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 236 at r3 (raw file):

        ## Start the SQL Agent resource
        New-VerboseMessage -Message 'SQL Server Agent --> Online'
        $AgentService.BringOnline(120)

Could we make this a parameter in the set-method instead of using a hard-coded value? Same parameter as above would work.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 241 at r3 (raw file):

    {
        New-VerboseMessage -Message 'Getting SQL Service information'
        $SqlService = Get-Service -DisplayName "SQL Server ($($ServerObject.ServiceName))"

Could we get the service using the service name instead´of the display name?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):

        New-VerboseMessage -Message 'Getting SQL Service information'
        $SqlService = Get-Service -DisplayName "SQL Server ($($ServerObject.ServiceName))"
        $AgentService = $SqlService.DependentServices | Where-Object { $_.StartType -ne ''}

Should we not start this only when StartType is `Automatic´?
$SqlService.DependentServices | Where-Object { $_.StartType -eq 'Automatic' }

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 249 at r3 (raw file):

        ## Start the SQL Server Agent service
        if ($AgentService)

If $AgentService contains more than one dependent service it will start all. But the message will say it only starts Agent. Think we would either change the message to "Starting dependent services" or loop thur each so we can tell which dependent service is started.
I know that default, today, it will only contain one dependent service.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 28 at r3 (raw file):

    OptionName = "user connections"
    OptionValue = 500
    RestartService = $False

Change to $false

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 38 at r3 (raw file):

{
    #region Not in the desired state
    Describe 'The system is not in the desired state' {

If you change this to Describe "$($script:DSCResourceName)" it's easier to see what resource is tested.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r3 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context 'Validate returned properties' {

And change the context to 'The system is not in the desired state'

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r3 (raw file):

            $result = Get-TargetResource @desiredState

            It 'Property: SQLServer' {

Can you change all It-blocks messages to start with 'Should...'
You can add all but one of the It-blocks together in an It-block like this
It 'Should return the same values as passed as parameters' {
    $result.SQLServer | Should Be $desiredState.SQLServer
    $result.SQLInstanceName | Should Be $desiredState.SQLInstanceName
    $result.OptionName | Should Be $desiredState.OptionName
}

It 'Should not return the same value as passed for the parameter OptionValue' {
    $result.OptionValue | Should Not Be $desiredState.OptionValue
}

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 85 at r3 (raw file):

        }

        It 'Test method returns false' {

This one should be in the same context as above It-blocks

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 89 at r3 (raw file):

        }

        It 'Set method calls Connect-SQL' {

This one should be in the same context as above It-blocks

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r3 (raw file):

        It 'Set method calls Connect-SQL' {
            ## attempt to bring the system into the desired state
            Get-TargetResource @desiredState

You don't call Set-method. And you should Assert that the mocks for setting the value and restart service is not called here.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 94 at r3 (raw file):

            # Check that our mock was called at least 3 times
            ##Assert-MockCalled -CommandName Connect-SQL -Times 1 -Scope Describe

Remove this comment if it should not be used.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 98 at r3 (raw file):

        }
    }
    #endregion Not in the desired state

You should have a test to test when OptionName is missing and throws.

Comments from Reviewable

Sorry it took a day extra for me to review the cluster bits in this code. To much work at work so didn't have time until now. :)
I will review the tests once all these changes are done.

Review status: all files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 57 at r3 (raw file):

    ## get the configuration option
    $option = $sql.Configuration.Properties | where { $_.DisplayName -eq $optionName }

Please change whereto Where-Object

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 124 at r3 (raw file):

    ## get the configuration option
    $option = $sql.Configuration.Properties | where {$_.DisplayName -eq $optionName}

Please change whereto Where-Object

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):

        ## Get the cluster resources
        New-VerboseMessage -Message 'Getting cluster resource for SQL Server'
        $SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = 'SQL Server' AND Name LIKE '%$($ServerObject.ServiceName)%'"

These two Get-WmiObject for SQL Server and SQL Server Agent seems to only work for named instances in my tests.

If the default instance is used then no WMI object will be returned. In that case the ServiceName property will contain the value 'MSSQLSERVER', but the Name property of the cluster resource will be 'SQL Server'
The wildcard serach could potentially return two resource objects. If an instance is called 'SQL2008', and another is called 'SQL2008NEW' then both will be returned when trying to change the instance named 'SQL2008'. Change the code to something similar like below would solve that.

$sqlServerTypeName = 'SQL Server'
$SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = '$sqlServerTypeName' AND Name LIKE '$sqlServerTypeName ($($ServerObject.ServiceName))'"

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):

        New-VerboseMessage -Message 'Getting cluster resource for SQL Server Agent'
        $AgentService = Get-WmiObject -Namespace root/MSCLuster -Class MSCluster_Resource -Filter "Type = 'SQL Server Agent' AND Name LIKE '%$($ServerObject.ServiceName)%'"

Same problems exist for the SQL Server Agent WMI query as for the SQL Server WMI query.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

class MSFT_xSQLServerConfiguration : OMI_BaseResource
{
    [Key, Description("The SQL Server for the configuration option.")] String SQLServer;

Add to the description of the SQLServer property in the schema and README.md something like "... or the DNS name of the cluster group".
Could you also please align these descriptions between the Schema, README.md and the comment based help in the private functions, so they all are the same descriptions.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please change all `[paramater` to `[Parameter` (upper case 'P')
> There are more than one that needs to change.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This parameter could be remove from the Get-method since it does not do anything. In the returned hash table you could just return $null. Less to test in the tests too.

Correct me if I am wrong, but aren't all TargetResource methods in a Resource to have the same parameters? See https://msdn.microsoft.com/en-us/powershell/dsc/authoringResourceMOF#writing-the-resource-script

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 57 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please change `where`to `Where-Object`

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 61 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please use New-TerminatingError here (helper function)

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 64 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> You can return the hashtable directly instead of assigning it to a variable and returning the variable.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 124 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please change `where`to `Where-Object`

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 128 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please use New-TerminatingError here (helper function). That will make the string message to be reused from the Get-method also. And allow localization in the future.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could we instead check if the running value `$option.RunValue` is different than config value, and if so restart the service if the user asked us to?
> I think some config values are dynamic even if IsDynamic is set to false, and we don't want to restart the service unnecessary. Correct me if I'm wrong.
>
> See remarks:
> https://msdn.microsoft.com/en-us/library/microsoft.sqlserver.management.smo.configproperty.runvalue.aspx
>

The IsDynamic property comes directly from the sys.configurations table's is_dynamic column. I believe this remark is simply noting that if you update the ConfigValue for a non-dynamic configuration item, it will not be reflected in the RunValue property until after the instance is restarted.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 145 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could we change the test to something like "Configuration option has been updated. Manual restart of SQL Server  is required for the change to take effect!"
>
> _Bonus points :smile: : Create a new helper function like `New-TerminatingError`, i.e `New-WarningMessage`, to be able to use localization strings._

Updated wording; will look into creating a helper function.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 193 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could you use splatting on these parameters instead?

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could you change this to `$sqlService = ...`. And all other occurrences.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could you change this to `$agentService = ...`. And all other occurrences.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Same problems exist for the SQL Server Agent WMI query as for the SQL Server WMI query.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could we make this a parameter in the set-method instead of using a hard-coded value?

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 236 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could we make this a parameter in the set-method instead of using a hard-coded value? Same parameter as above would work.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 241 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could we get the service using the service name instead´of the display name?

I think with the information supplied, this is the best method to identify the service.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Should we not start this only when StartType is `Automatic´?
>
> `$SqlService.DependentServices | Where-Object { $_.StartType -eq 'Automatic' }`

Checks for dependent services that are running. I believe this is better than checking for start-up type as there could be viable scenarios where an automatic service is turned off and should not be restarted automatically.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 249 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> If `$AgentService` contains more than one dependent service it will start all. But the message will say it only starts Agent. Think we would either change the message to "Starting dependent services" or loop thur each so we can tell which dependent service is started.
> _I know that default, today, it will only contain one dependent service._

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Add to the description of the `SQLServer` property in the schema and README.md something like "... or the DNS name of the cluster group".
>
> Could you also please align these descriptions between the Schema, README.md and the comment based help in the private functions, so they all are the same descriptions.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 28 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Change to $false

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 38 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> If you change this to `Describe "$($script:DSCResourceName)"` it's easier to see what resource is tested.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 60 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> And change the context to 'The system is not in the desired state'

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 64 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Can you change all It-blocks messages to start with 'Should...'
> You can add all but one of the It-blocks together in an It-block like this
> ```
> It 'Should return the same values as passed as parameters' {
>     $result.SQLServer | Should Be $desiredState.SQLServer
>     $result.SQLInstanceName | Should Be $desiredState.SQLInstanceName
>     $result.OptionName | Should Be $desiredState.OptionName
> }
>
> It 'Should not return the same value as passed for the parameter OptionValue' {
>     $result.OptionValue | Should Not Be $desiredState.OptionValue
> }
>
> ```
>

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 85 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This one should be in the same context as above It-blocks

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 89 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This one should be in the same context as above It-blocks

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> You don't call Set-method. And you should Assert that the mocks for setting the value and restart service is not called here.

I think this was just bad commenting in my code. Get will never call Restart-SqlService.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 94 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Remove this comment if it should not be used.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 98 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> You should have a test to test when OptionName is missing and throws.

Done.

Comments from Reviewable

Thanks for the feedback @johlju. Fixing these things and writing the tests exposed other issues that had be fixed along the way. I think at this point, all your comments have been answered.

Review status: 1 of 6 files reviewed at latest revision, 29 unresolved discussions.

Comments from Reviewable

Review status: 1 of 6 files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> These two `Get-WmiObject` for SQL Server and SQL Server Agent seems to only work for named instances in my tests.
> * If the default instance is used then no WMI object will be returned. In that case the `ServiceName` property will contain the value `'MSSQLSERVER'`, but the `Name` property of the cluster resource will be `'SQL Server'`
> * The wildcard serach could potentially return two resource objects. If an instance is called 'SQL2008', and another is called 'SQL2008NEW' then both will be returned when trying to change the instance named 'SQL2008'. Change the code to something similar like below would solve that.
>
> ```
> $sqlServerTypeName = 'SQL Server'
> $SqlService = Get-WmiObject -Namespace root/MSCluster -Class MSCluster_Resource -Filter "Type = '$sqlServerTypeName' AND Name LIKE '$sqlServerTypeName ($($ServerObject.ServiceName))'"
> ```

Rewrote these WMI calls to better identify the SQL instances.

Comments from Reviewable

Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Correct me if I am wrong, but aren't all TargetResource methods in a Resource to have the same parameters? See https://msdn.microsoft.com/en-us/powershell/dsc/authoringResourceMOF#writing-the-resource-script

They all must have the same Mandatory parameters but Set and Test are the only ones that need to have all of the same parameters.

Comments from Reviewable

Reviewed 1 of 5 files at r1, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> They all must have the same Mandatory parameters but Set and Test are the only ones that need to have all of the same parameters.

Unless there is a style-based or otherwise compelling reason, I would like to keep this parameter as-is. In addition to the consistency between functions, I appreciate the ease of calling Get-TargetResource from Test-TargetResource.
$state = Get-TargetResource @PSBoundParameters

versus
$params = @{
	SQLServer = $SQLServer
	SQLInstanceName = $SQLInstanceName
	OptionName = $OptionName
	OptionValue = $OptionValue
}

$state = Get-TargetResource @params

Comments from Reviewable

Review status: all files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Unless there is a style-based or otherwise compelling reason, I would like to keep this parameter as-is. In addition to the consistency between functions, I appreciate the ease of calling Get-TargetResource from Test-TargetResource.
>
>     $state = Get-TargetResource @PSBoundParameters
>
> versus
>
> 	$params = @{
> 		SQLServer = $SQLServer
> 		SQLInstanceName = $SQLInstanceName
> 		OptionName = $OptionName
> 		OptionValue = $OptionValue
> 	}
>
> 	$state = Get-TargetResource @params

I understand, you can leave as is. Just add a comment for that param in the help comment above that it's not used in this funciton

Comments from Reviewable

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> I understand, you can leave as is. Just add a comment for that param in the help comment above that it's not used in this funciton

Done.

Comments from Reviewable

Will verify the cluster code tomorrow (I will try to get an opportunity).
And I still have to review the tests, which I have not done fully yet.

Reviewed 1 of 5 files at r1, 4 of 5 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

@mbreakey3 I think this would be good to add to the style guideline maybe? The part that if unused non-mandatory parameters are added to the Get-method then to the comment based help "add a comment for that param in the help comment above that it's not used in this function".
Could you look at get that into a guideline? Or I could submit an issue also.
@mbreakey3 I raised this question in another PR as well, see PR https://github.com/PowerShell/xSQLServer/pull/147. It would be awesome if you could read and comment on that too, and see if that is something that needs to be added to a guideline as well.
If you don't have any other comments than what was said here, then on that PR I will add a comment to do the same as was done here, until there is a guideline that says differently. :)

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 57 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 61 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 64 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 124 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 128 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 134 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> The IsDynamic property comes directly from the sys.configurations table's is_dynamic column. I believe this remark is simply noting that if you update the ConfigValue for a non-dynamic configuration item, it will not be reflected in the RunValue property until after the instance is restarted.

Yes, you are correct. Read this one wrong. Stand corrected. :)

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 145 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Updated wording; will look into creating a helper function.

It would be awesome if you could create the helper function, would be appreciated. But for now this looks good to me.
LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 193 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 225 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 236 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 241 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> I think with the information supplied, this is the best method to identify the service.

I think the ServiceName would not change between versions of SQL, but higher risk that the DisplayName could change. But lets leave it as is.
LGTM.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Checks for dependent services that are running. I believe this is better than checking for start-up type as there could be viable scenarios where an automatic service is turned off and should not be restarted automatically.

Ah, good point! Could you please add that as a comment? I think that it would be good for next coder to know your thought here.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 249 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 58 at r5 (raw file):

    ## get the configuration option
    $option = $sql.Configuration.Properties | Where-Object { $_.DisplayName -eq $optionName }

$optionName should be $OptionName since it is referencing a parameter, not a local variable.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):

    {
        New-VerboseMessage -Message 'Configuration option has been updated, restarting instance...'
        Restart-SqlService -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Could we add a parameter to the Set-method (Timeout parameter to the schema) so that Restart-SqlService can be called with a user provided timeout value?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 211 at r5 (raw file):

    .EXAMPLE
    $server = Connect-SQL -SQLServer $env:ComputerName

This is not needed in the example since the function calls Connect-SQL?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 212 at r5 (raw file):

    .EXAMPLE
    $server = Connect-SQL -SQLServer $env:ComputerName
    Restart-SqlService -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Could this line maybe be without the variables, and add the timeout variable?
Restart-SqlService -SQLServer localhost -SQLInstanceName MSSQLSERVER -Timeout 60

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r5 (raw file):

    )

    if (!$ServerObject)

This check is not necessary anymore since you remove the parameter. Change so the check is done after you assign the $serverObject.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 234 at r5 (raw file):

    if (!$ServerObject)
    {
        $ServerObject = Connect-SQL -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

Change $ServerObject to $serverObject

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 247 at r5 (raw file):

        ## Stop the SQL Server resource
        New-VerboseMessage -Message 'SQL Server resource --> Offline'

Maybe add which resource (the name) you are bringing offline too?  Could you please change the message to something like this "Bringing the resource SQL Server  offline"
Since it could take a while, I also think the user would be interested in a message 'Resource ... is offline' after the TakeOffline() has run? What do you think?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 251 at r5 (raw file):

        ## Start the SQL Agent resource
        New-VerboseMessage -Message 'SQL Server Agent --> Online'

Could you do the same for BringOnline() as you do for the TakeOffline() part? (a message before and after bringing online the resource, since it could take a while)

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):

"... or the DNS name of the cluster group"

I don't see this change :) My thought is that the VCO differ from the host name, so I think it should say both. Are you of a different opinion?

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r5 (raw file):

            }

            It 'Test method returns false' {

This It-block should start with 'Should...'

Comments from Reviewable

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

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 242 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Ah, good point! Could you please add that as a comment? I think that it would be good for next coder to know your thought here.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 58 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> `$optionName` should be `$OptionName` since it is referencing a parameter, not a local variable.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could we add a parameter to the Set-method (Timeout parameter to the schema) so that Restart-SqlService can be called with a user provided timeout value?

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 211 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This is not needed in the example since the function calls `Connect-SQL`?

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 212 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could this line maybe be without the variables, and add the timeout variable?
> ```
> Restart-SqlService -SQLServer localhost -SQLInstanceName MSSQLSERVER -Timeout 60
> ```

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 232 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This check is not necessary anymore since you remove the parameter. Change so the check is done after you assign the `$serverObject`.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 234 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Change `$ServerObject` to `$serverObject`

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 247 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Maybe add which resource (the name) you are bringing offline too?  Could you please change the message to something like this "Bringing the resource SQL Server  offline"
> Since it could take a while, I also think the user would be interested in a message 'Resource ... is offline' after the `TakeOffline()` has run? What do you think?

Done. I do not think there is value to adding another message between `TakeOffline()` and `BringOnline()` since nothing else occurs between their calls.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 251 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could you do the same for `BringOnline()` as you do for the `TakeOffline()` part? (a message before and after bringing online the resource, since it could take a while)

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> >  "... or the DNS name of the cluster group"
>
> I don't see this change :) My thought is that the VCO differ from the host name, so I think it should say both. Are you of a different opinion?

I think that "hostname" and "DNS name" are synonymous.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This It-block should start with 'Should...'

Done. Corrected a few others as well.

Comments from Reviewable

Reviewed 4 of 5 files at r6.
Review status: 6 of 7 files reviewed at latest revision, 17 unresolved discussions.

README.md, line 354 at r6 (raw file):

  - Added support for clustered SQL instances
  - BREAKING CHANGE: Updated parameters to align with other resources (SQLServer / SQLInstanceName)
* Created unit tests for resource

Could you add for which resource a test was added? See example under <IP_ADDRESS> below

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

Awesome! Please add the parameter to the README.md as well :smile:

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 247 at r5 (raw file):
Previously, nabrond (Brandon) wrote…
> Done. I do not think there is value to adding another message between `TakeOffline()` and `BringOnline()` since nothing else occurs between their calls.

Agree. LGTM

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 59 at r6 (raw file):

    )

    if (! $sql)

There is a blank space here between exclamation mark and the variable, which is not done in other places. Remove the blank space here or change the other places?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r6 (raw file):

Default is 'MSSQLServer'

Please change to 'MSSQLSERVER' as the default instance name in the description (same as on the parameters).
Please change that in all of the comment based help comments, the README.md and the schema.mof.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 263 at r6 (raw file):

    $serverObject = Connect-SQL -SQLServer $SQLServer -SQLInstanceName $SQLInstanceName

    if ($serverObject.IsClustered)

Wouldn't be a good idea to check whether $serverObject is actually assigned before accessing properties from it? If it not set, then the Else-block would be called. It think it would be better to throw an error if the $serverObject does not contain the expected object.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.schema.mof, line 4 at r3 (raw file):
Previously, nabrond (Brandon) wrote…
> I think that "hostname" and "DNS name" are synonymous.

Okay. Let's leave it as is. :)
LGTM

Comments from Reviewable

@nabrond Thanks for adding the function New-WarningMessage. One step closer to get all strings localized 😄

Review status: 6 of 7 files reviewed at latest revision, 11 unresolved discussions.

README.md, line 354 at r6 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could you add for which resource a test was added? See example under <IP_ADDRESS> below

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 140 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Awesome! Please add the parameter to the README.md as well :smile:

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 59 at r6 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> There is a blank space here between exclamation mark and the variable, which is not done in other places. Remove the blank space here or change the other places?

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 228 at r6 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> > Default is 'MSSQLServer'
>
> Please change to 'MSSQLSERVER' as the default instance name in the description (same as on the parameters).
> Please change that in all of the comment based help comments, the README.md and the schema.mof.

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 263 at r6 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Wouldn't be a good idea to check whether `$serverObject` is actually assigned before accessing properties from it? If it not set, then the Else-block would be called. It think it would be better to throw an error if the `$serverObject` does not contain the expected object.

That logic seems to be handled in the Connect-SQL function around line 48.

Comments from Reviewable

Verified the cluster code. Seems perfect. Nice job!
Still haven't reviewed the tests, will do as soon as possible.

Reviewed 3 of 3 files at r7.
Review status: 6 of 7 files reviewed at latest revision, 6 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 263 at r6 (raw file):
Previously, nabrond (Brandon) wrote…
> That logic seems to be handled in the Connect-SQL function around line 48.

You are right. `Connect-SQL` throws if it is not set.
LGTM.

Comments from Reviewable

Review status: 6 of 7 files reviewed at latest revision, 1 unresolved discussion.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> @mbreakey3 I think this would be good to add to the style guideline maybe? The part that if unused non-mandatory parameters are added to the Get-method then to the comment based help "add a comment for that param in the help comment above that it's not used in this function".
> Could you look at get that into a guideline? Or I could submit an issue also.
>
> @mbreakey3 I raised this question in another PR as well, see PR https://github.com/PowerShell/xSQLServer/pull/147. It would be awesome if you could read and comment on that too, and see if that is something that needs to be added to a guideline as well.
> If you don't have any other comments than what was said here, then on that PR I will add a comment to do the same as was done here, until there is a guideline that says differently. :)

@johlju - Yeah, if you could open an issue on this in [DscResources](https://github.com/PowerShell/DscResources) that would be great

Comments from Reviewable

Reviewed 2 of 5 files at r6, 3 of 3 files at r7.
Review status: 6 of 7 files reviewed at latest revision, 7 unresolved discussions.

xSQLServerHelper.psm1, line 151 at r7 (raw file):

        $WarningType,

        [Parameter(Mandatory = $false)]

You can take out this line since Mandatory = $false is the default

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):

# Load Common Code
Import-Module $currentPath\..\..\xSQLServerHelper.psm1 -Verbose:$false -ErrorAction Stop

This should use 'Join-Path' instead of '' Also, Verbose:$false can be removed

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 112 at r7 (raw file):

        $SQLServer,

        [Parameter(Mandatory = $false)]

remove this line:
[Parameter(Mandatory = $false)]

since it's the default value

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 178 at r7 (raw file):

    The desired value of the SQL configuration option

    .PARAMETER RestartService

Any parameter that's not being used in Test-TargetResource, should have a comment here: 'Not used in this function'

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 248 at r7 (raw file):

    (
        [Parameter(Mandatory = $true)]
        [string]

[String]

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 277 at r7 (raw file):

        ## Start the SQL Agent resource
        New-VerboseMessage -Message 'Bringing the SQL Server resources online.'

Can you add a more descriptive Verbose message so that the user knows this is a new Service that is being brought online rather than the one that was just brought offline?

Comments from Reviewable

@mbreakey3 could you do a review loop thru the test in this PR also?

Reviewed 1 of 5 files at r6.
Review status: all files reviewed at latest revision, 22 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 48 at r3 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> @johlju - Yeah, if you could open an issue on this in [DscResources](https://github.com/PowerShell/DscResources) that would be great

Done so. :smile: Closing this comment.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 86 at r7 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        Context 'The system is not in the desired state' {

You should add a context block for testing when the system is in desired state (for the Get-method)

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r7 (raw file):

            It 'Should return the same values as passed for property SQLServer' {
                $result.SQLServer | Should Be $desiredState.SQLServer

Those It-blocks that used $desireState.<property> you could gather them into one It-block. To something like It 'Should return the same values as passed as parameters'

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r7 (raw file):

            }

            It 'Should cause Test-TargetResource to return false' {

This It-block should be moved to the Describe-block that tests the Test-method

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 115 at r7 (raw file):

            It 'Should call Connect-SQL mock when getting the current state' {
                Get-TargetResource @desiredState

Two options here

You should not need this Get-TargetResource, since you called it after the Context block. And if you remove Get-TargetResource I think you should change the Assert-MockCalled scope to -Scope Context
If you want to keep Get-TargetResource, on the next line you should change the Assert-MockCalled scope to -Scope It.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):

            It 'Should call New-TerminatingError mock when a bad option name is specified' {
                { Get-TargetResource @invalidOption } | Should Throw

Please add a check so that only the correct Throw is caught (tested OK), like  | Should Throw "This is a test". Throw that message you are testing from your mock.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 121 at r7 (raw file):

            It 'Should call New-TerminatingError mock when a bad option name is specified' {
                { Get-TargetResource @invalidOption } | Should Throw
                Assert-MockCalled -ModuleName $script:DSCResourceName -CommandName New-TerminatingError -Scope Describe -Times 1

Change the Assert-MockCalled scope to -Scope It.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 150 at r7 (raw file):

        } -ModuleName $script:DSCResourceName -Verifiable

        ## Test-TargetResource should return true when in the desired state

Need to add a test when Test-TargetResource it is not in desired state also

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 214 at r7 (raw file):

            }

            It 'Should warn about restart when required, but not requested' {

How do we know it did warn? Think we need to Mock and Assert the New-WarningMessage here?

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 220 at r7 (raw file):

            It 'Should call Connect-SQL to get option values' {
                Assert-MockCalled -ModuleName $script:DSCResourceName -CommandName Connect-SQL -Scope Describe -Times 3

Could you change scope to -Scope Context here?

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 279 at r7 (raw file):

                Mock -CommandName Start-Service {} -Verifiable

                It 'Should not throw an exception when restarting a default instance' {

I don't see how the function would ever throw? And you are mocking everything so it can never throw. Maybe add a mock Get-Service, Get-WmiObject and Restart-Service that will throw if passed a value that was "not expected"?
Also add mocks to the cluster part here also (Get-WmiObject) where the mock throws. So if it is hit, then an error will be thrown and the test will fail.
Do the same for the cluster part below.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 283 at r7 (raw file):

                }

                It 'Should not throw an exception when restarting an instance with no SQL Agent' {

Same as previous comment

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 288 at r7 (raw file):

                It 'Should use mock methods a specific number of times' {
                    Assert-MockCalled -CommandName Get-Service -Scope Context -Exactly -Times 2

Assert these in each It-block instead, with scope set to -Scope It

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 344 at r7 (raw file):

                } -Verifiable -ParameterFilter { $Query -match "^ASSOCIATORS OF" }

                It 'Should not throw an exception when restarting a default instance' {

Please do the same as for the previous context block

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 348 at r7 (raw file):

                }

                It 'Should not throw an exception when restarting a named instance' {

Please do the same as for the previous context block

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 353 at r7 (raw file):

                It 'Should use mock methods a specific number of times' {
                    Assert-MockCalled -Scope Context -CommandName Get-WmiObject -Exactly -Times 4

Assert these in each It-block instead, with scope set to -Scope It

Comments from Reviewable

Reviewed 1 of 5 files at r6.
Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 61 at r7 (raw file):

try
{
    #region Test getting the current state

If you're going to surround your Describe blocks with regions, it shoud be '#region Get-TargetResource tests', '#region Set-TargetResource tests', '#region Test-TargetResource tests'

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 234 at r7 (raw file):

            Context 'Restart-SqlService standalone instance' {

                Mock -ModuleName $script:DSCResourceName -CommandName Connect-SQL -MockWith {

could you list the CommandName attribute first when creating Mocks? It makes it easier to know what's being Mocked

Comments from Reviewable

Review status: 4 of 7 files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

xSQLServerHelper.psm1, line 151 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> You can take out this line since Mandatory = $false is the default

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> This should use 'Join-Path' instead of '\' Also, Verbose:$false can be removed

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 112 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> remove this line:
> ```powershell
> [Parameter(Mandatory = $false)]
> ```
> since it's the default value

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 178 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Any parameter that's not being used in Test-TargetResource, should have a comment here: 'Not used in this function'

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 248 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> [String]

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 277 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Can you add a more descriptive Verbose message so that the user knows this is a new Service that is being brought online rather than the one that was just brought offline?

Added a listing of resources that are being acted upon to the verbose message.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 61 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> If you're going to surround your Describe blocks with regions, it shoud be '#region Get-TargetResource tests', '#region Set-TargetResource tests', '#region Test-TargetResource tests'

The #regions came from the template. They do nothing in my editor, so I removed them. Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 86 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> You should add a context block for testing when the system is in desired state (for the Get-method)

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 91 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Those It-blocks that used `$desireState.` you could gather them into one It-block. To something like `It 'Should return the same values as passed as parameters'`

Consolidated these It blocks. Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 110 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> This It-block should be moved to the Describe-block that tests the Test-method

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 115 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Two options here
>  1. You should not need this `Get-TargetResource`, since you called it after the Context block. And if you remove `Get-TargetResource` I think you should change the `Assert-MockCalled` scope to `-Scope Context`
>  2. If you want to keep `Get-TargetResource`, on the next line you should change the `Assert-MockCalled` scope to `-Scope It`.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please add a check so that only the correct Throw is caught (tested OK), like ` | Should Throw "This is a test"`. Throw that message you are testing from your mock.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 121 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Change the `Assert-MockCalled` scope to `-Scope It`.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 150 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Need to add a test when Test-TargetResource it is not in desired state also

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 214 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> How do we know it did warn? Think we need to Mock and Assert the New-WarningMessage here?

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 220 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Could you change scope to `-Scope Context` here?

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 234 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> could you list the CommandName attribute first when creating Mocks? It makes it easier to know what's being Mocked

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 279 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> I don't see how the function would ever throw? And you are mocking everything so it can never throw. Maybe add a mock `Get-Service`, `Get-WmiObject` and `Restart-Service` that will throw if passed a value that was "not expected"?
> Also add mocks to the cluster part here also (`Get-WmiObject`) where the mock throws. So if it is hit, then an error will be thrown and the test will fail.
>
> Do the same for the cluster part below.

I renamed these tests and adjusted how I am validating that the function is working. The test for exception is needed just to ensure the expected information is coming out of all mocks. The only way I could think to test this function is by ensuring the correct combination of mocks is used.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 283 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Same as previous comment

Done. See previous reply.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 288 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Assert these in each It-block instead, with scope set to `-Scope It`

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 344 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please do the same as for the previous context block

Done. I renamed these tests and adjusted how I am validating that the function is working. The test for exception is needed just to ensure the expected information is coming out of all mocks. The only way I could think to test this function is by ensuring the correct combination of mocks is used.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 348 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please do the same as for the previous context block

Done. See previous comment.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 353 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Assert these in each It-block instead, with scope set to `-Scope It`

Done.

Comments from Reviewable

@nabrond you have to rebase your branch, there is conflicts with dev branch.
git checkout dev
git upstream dev
git merge upstream/dev
... Resolve any conflicts ...
git add ... file in conflicts
git commit
git push

Actually the correct steps to rebase is to use git rebase. PowerShell team does not want us to use git merge.
The correct steps are listed here: https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts

Thanks for the tip @johlju you're the best 👍
I add this link in my favorites.

Merge conflict has been resolved.

Review status: 3 of 7 files reviewed at latest revision, 23 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 2 files at r8, 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 25 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

Could you change this to use similar formatting as [this file](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_ServiceResource.Tests.ps1#L5) rather than using piping?

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r9 (raw file):

{
    [CmdletBinding()]
    [OutputType([hashtable])]

These capitalized types were fine as they were. You don't need the 'System...' but the type should still be capitalized. As in 'Boolean' and 'Int32'

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 268 at r9 (raw file):

        New-VerboseMessage -Message 'Getting active cluster resource SQL Server Agent'
        $agentService = Get-WmiObject -Namespace root/MSCluster -Query "ASSOCIATORS OF {$sqlService} WHERE ResultClass = MSCluster_Resource" | Where-Object { ($_.Type -eq "SQL Server Agent") -and ($_.State -eq 2) }

Could you split this up into 2+ lines to make it more readable?

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 61 at r7 (raw file):
Previously, nabrond (Brandon) wrote…
> The #regions came from the template. They do nothing in my editor, so I removed them. Done.

Thanks! Sorry about that, we've recently been in the process of updating our templates, so I forget what we have in there currently.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 449 at r9 (raw file):

                        $mock | Add-Member -MemberType ScriptMethod -Name TakeOffline -Value { param ( [int] $TimeOut ) }
                        $mock | Add-Member -MemberType ScriptMethod -Name BringOnline -Value { param ( [int] $Timeout ) }

could you ensure that the $TimeOut variable capitalization is consistent? Either $Timeout or $TimeOut

Comments from Reviewable

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Could you change this to use similar formatting as [this file](https://github.com/PowerShell/PSDscResources/blob/dev/Tests/Unit/MSFT_ServiceResource.Tests.ps1#L5) rather than using piping?

Since this is multiple directories up, are you expecting something like this?
Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `
                               -ChildPath '..\xSQLServerHelpers.psm1') `
                               -Force

Comments from Reviewable

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

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 35 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> These capitalized types were fine as they were. You don't need the 'System...' but the type should still be capitalized. As in 'Boolean' and 'Int32'

Done.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 268 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Could you split this up into 2+ lines to make it more readable?

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 449 at r9 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> could you ensure that the $TimeOut variable capitalization is consistent? Either $Timeout or $TimeOut

Done.

Comments from Reviewable

Reviewed 1 of 2 files at r10.
Review status: 6 of 7 files reviewed at latest revision, 18 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):
Previously, nabrond (Brandon) wrote…
> Since this is multiple directories up, are you expecting something like this?
>
> ````powershell
> Import-Module -Name (Join-Path -Path (Split-Path $PSScriptRoot -Parent) `
>                                -ChildPath '..\xSQLServerHelpers.psm1') `
>                                -Force
> ````

Almost:
```powershell
Import-Module -Name (Join-Path -Path (Split-Path (Split-Path $PSScriptRoot -Parent) -Parent) `
                               -ChildPath 'CommonResourceHelper.psm1') `
                               -Force
^I think that should do it.
We're trying to get rid of all of the '\' since that's not guaranteed to work on all OS's

---

*Comments from [Reviewable](https://reviewable.io:443/reviews/powershell/xsqlserver/143)*
<!-- Sent from Reviewable.io -->

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Almost:
> ```powershell
> Import-Module -Name (Join-Path -Path (Split-Path (Split-Path $PSScriptRoot -Parent) -Parent) `
>                                -ChildPath 'CommonResourceHelper.psm1') `
>                                -Force
>
> ```
> ^I think that should do it.
> We're trying to get rid of all of the '\' since that's not guaranteed to work on all OS's

Sorry, replace 'CommonResourceHelper.psm1' with 'xSQLServerHelpers.psm1'

Comments from Reviewable

Review status: 5 of 8 files reviewed at latest revision, 16 unresolved discussions.

DSCResources/MSFT_xSQLServerConfiguration/MSFT_xSQLServerConfiguration.psm1, line 5 at r7 (raw file):
Previously, mbreakey3 (Mariah) wrote…
> Sorry, replace 'CommonResourceHelper.psm1' with 'xSQLServerHelpers.psm1'

Done.

Comments from Reviewable

Reviewed 1 of 1 files at r11.
Review status: 6 of 7 files reviewed at latest revision, 16 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 2 files at r8, 1 of 3 files at r9, 1 of 2 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):
Previously, nabrond (Brandon) wrote…
> Done.

Sorry I was unclear. You didn't have to add another It-block. Please just replace `Should Throw` with `Should Throw 'ConfigurationOptionNotFound'`. And remove the It-block below. Concatenating the two It-blocks will result in the same test.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 20 at r11 (raw file):

$defaultState = @{
    SQLServer = "CLU01"

Please change to ' on all of these strings in all of the hash tables.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 82 at r11 (raw file):

                        Properties = @(
                            @{
                                DisplayName = "user connections"

Use ' around this string

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 119 at r11 (raw file):

                        Properties = @(
                            @{
                                DisplayName = "user connections"

Use ' around this string

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 156 at r11 (raw file):

                        Properties = @(
                            @{
                                DisplayName = "user connections"

Use ' around this string

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 193 at r11 (raw file):

                    Properties = @(
                        @{
                            DisplayName = "user connections"

Please surround the string with '

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 225 at r11 (raw file):

                    Properties = @(
                        @{
                            DisplayName = "user connections"

Please surround the string with '

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 244 at r11 (raw file):

                    Properties = @(
                        @{
                            DisplayName = "show advanced options"

Please surround the string with '

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 295 at r11 (raw file):

                Mock -CommandName Connect-SQL -MockWith {
                    return @{
                        Name = "MSSQLSERVER"

Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 394 at r11 (raw file):

                Mock -CommandName Connect-SQL -MockWith {
                    return @{
                        Name = "MSSQLSERVER"

Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 456 at r11 (raw file):

                It 'Should restart SQL Server and SQL Agent resources for a clustered default instance' {
                    { Restart-SqlService -SQLServer "CLU01" -SQLInstanceName "MSSQLSERVER" } | Should Not Throw

Please surround the string with '.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 466 at r11 (raw file):

                It 'Should restart SQL Server and SQL Agent resources for a clustered named instance' {
                    { Restart-SqlService -SQLServer "CLU01" -SQLInstanceName "NAMEDINSTANCE" } | Should Not Throw

Please surround the string with '.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 476 at r11 (raw file):

                It 'Should not try to restart a SQL Agent resource that is not online' {
                    { Restart-SqlService -SQLServer "CLU01" -SQLInstanceName "STOPPEDAGENT" } | Should Not Throw

Please surround the string with '.

Comments from Reviewable

Review status: 6 of 7 files reviewed at latest revision, 13 unresolved discussions.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 120 at r7 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Sorry I was unclear. You didn't have to add another It-block. Please just replace `Should Throw` with `Should Throw 'ConfigurationOptionNotFound'`. And remove the It-block below. Concatenating the two It-blocks will result in the same test.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 20 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please change to ' on all of these strings in all of the hash tables.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 82 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Use ' around this string

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 119 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Use ' around this string

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 156 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Use ' around this string

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 193 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 225 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 244 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 295 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 394 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '. Do this for all the strings below as well (those without variables in them of course).

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 456 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 466 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '.

Done.

Tests/Unit/MSFT_xSQLServerConfiguration.Tests.ps1, line 476 at r11 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
> Please surround the string with '.

Done.

Comments from Reviewable

Reviewed 1 of 1 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@mbreakey3 Can you take a glance at this and merge when you feel it looks okay?

Reviewed 1 of 1 files at r12.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable


Annotations RESTful interface

@brianhelba I've added IDs to the shapes and added work-arounds for the lack of transactions in Mongo.  I think this is done, though there are no tests.
I wonder if we should allow element IDs to be arbitrary strings.  Currently, they need to be of the same form as Mongo IDs (24 hex digits), and are assigned from the Mongo IDs if they are not specified.  I could see a client finding it useful to use their own ids, in which case we could make this an arbitrary string (it is still required to be unique), or a string with strong limitations.

@manthey LGTM.
Next, can you:

add tests that show how to interact with the REST API to create, find, or update annotations
add tests that show how the backend analytics code interact with the Annotation API to create, find, or update annotations. Do we do something like importing the Annotation class and interact with its member functions to achieve that?

@cdeepakroy I just added an ellipse to the schema.

Initially, I am expecting that the analytics code will interact with the annotations much like other Girder models.  That is, calling createAnnotation, find, load, and save from the annotations model.  Eventually there will be a method to filter the returned elements of the annotation to spatial regions (or perform downsampling).  For now, it is expected that you get and set all of them at once.

I think to begin with, the backend analytics code will largely generate annotations. Maybe when we delve into implementing the active learning stuff, we might have to do find/search operations in specific regions/tiles.

Might want to think about a streamlined way to ingest machine-generated annotations in bulk. Many of these will share significant metadata tags that would be repeated up to 1M times per slide.

From: Deepak Roy Chittajallu<EMAIL_ADDRESS>Sent: Tuesday, April 12, 2016 12:26:00 PM
To: DigitalSlideArchive/large_image
Cc: Cooper, Lee
Subject: Re: [DigitalSlideArchive/large_image] Annotations RESTful interface (#50)
I think to begin with, the backend analytics code will largely generate annotations. Maybe when we delve into implementing the active learning stuff, we might have to do find/search operations in specific regions/tiles.

You are receiving this because you commented.
Reply to this email directly or view it on GitHubhttps://github.com/DigitalSlideArchive/large_image/pull/50#issuecomment-208991048

This e-mail message (including any attachments) is for the sole use of
the intended recipient(s) and may contain confidential and privileged
information. If the reader of this message is not the intended
recipient, you are hereby notified that any dissemination, distribution
or copying of this message (including any attachments) is strictly
prohibited.
If you have received this message in error, please contact
the sender by reply e-mail message and destroy all copies of the
original message (including attachments).

@cooperlab As part of writing tests, I'll write some for performance and try out sets up to the millions of elements.  If what we have isn't performant enough, we'll adjust it.


Printing does not work out-of-the-box

I installed the hplip package as I have an HP printer, I managed to install my printer, but the test page would not print.
To get it to print, I had to do the following:

Go to print settings
Go to Server settings
Tick the boxes for "Publish shared printers ... , Tick "Allowed remote admin, Tick "Allow user to cancel ....

Then I could print.  :)
I love hat you have done so far. Keep up the great work.

@probonopd
I am using an HP-Officejet-Pro-8600. It is connected on the network. I have always needed to install hplip drivers in Linux or BSD to get this printer to work.
I un-ticked the boxes for the server settings and the printer still works now, so not sure what went on there. I will try and repeat.

Maybe we should preinstall hplip then. Can you test with a helloSystem Live ISO that sudo pkg install hplip (and maybe waiting some time) makes your printer work even without changing the checkboxes?

It would be great if you preinstall hplip . I will test as you say with the live ISO.

What happens if you don't check any checkboxes but run sudo service cupsd restart?

Please try https://github.com/helloSystem/ISO/releases/tag/experimental-12.2 - does it work out of the box there?

I tried that release and it printing works out-of-the-box ... great!
I noticed in that version:

Dock items don't work i.e Preferences/Utilities
Used to be able to click on "Search" top bar and start typing a name to get to a program fast, no longer works, typing does nothing.

I presume this is still a work in progress.
Still don't like the new windows, where is the nice subtle transparencies I was getting with the openbox system?  :)

Please open new tickets for those.