-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/PostgreSql Script #5
Conversation
Added PostgreSqlScript resource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 7 files at r1, 1 of 3 files at r2.
Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @chtoone, @jcwalker, @jeffotterpohl, @regedit32, and @themitchk)
README.md, line 39 at r2 (raw file):
| ---- | ---- | ---- | ---- | ---- | | DatabaseName | Key | String | Specifies the name of the _PostgreSQL_ database. | | | SetFilePath | Key | String | Path to the T-SQL file that will perform _Set_ action | |
I would add a period to unify all the descriptions.
README.md, line 42 at r2 (raw file):
| GetFilePath | Key | String | Path to the T-SQL file that will perform _Get_ action | | | SetFilePath | Key | String | Path to the T-SQL file that will perform _Test_ action | | | Credential | Required | PsCredential | The credentials to authenticate with, using _Postgres Authentication_. | |
Credential is not required but write.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 25 at r2 (raw file):
The credentials to authenticate with, using PostgreSQL Authentication. .PARAMETER PsqlLocation Location of the psql executable. Defaults to "C:\Program Files\PostgreSQL\12\bin\psql.exe"
Missing period.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 54 at r2 (raw file):
[System.Management.Automation.Credential()]
This allows you to pass in a username as a string and have an interactive prompt for the password, which I’ll demonstrate later in the post. I tested this out and it does not work in DSC. This can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 7 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @chtoone, @jcwalker, @jeffotterpohl, @regedit32, and @themitchk)
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 152 at r3 (raw file):
$previousErrorActionPreference = $ErrorActionPreference $ErrorActionPreference = "Stop" $DbExists = $false;
I believe acronyms of two letters should be capitalized for camelCase.
Same with $db below
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 153 at r3 (raw file):
$ErrorActionPreference = "Stop" $DbExists = $false; $DbList = Invoke-Command -ScriptBlock {& $PsqlLocation -lqt 2>&1}
I would add a verbose statement before this saying it is "Listing databases" or something similar. It helps when debugging. It may be nice to put the command being ran in the statement.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 166 at r3 (raw file):
if ($DbExists -eq $false -and $CreateDatabase) { Invoke-Command -ScriptBlock {
I would add a verbose statement before this saying it is "Creating databases" or something similar. It helps when debugging. It may be nice to put the command being ran in the statement.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 170 at r3 (raw file):
} } Invoke-Command -ScriptBlock {
I would move the verbose statement from above to here,
Write-Verbose -Message ($script:localizedData.ExecutingSetScript -f $SetFilePath,$DatabaseName)
```'
It may be nice to put the command being ran in the statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @chtoone, @jeffotterpohl, @regedit32, and @themitchk)
source/PostgreSqlDsc.psd1, line 85 at r3 (raw file):
,
The comma isn't needed since the new resource is on a newline.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 151 at r3 (raw file):
$previousErrorActionPreference = $ErrorActionPreference $ErrorActionPreference = "Stop"
Why are we doing this vs adding a '-ErrorAction Stop' at the end the cmdlets?
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 169 at r3 (raw file):
& $PsqlLocation -d 'postgres' -c "CREATE DATABASE $DatabaseName" } }
Style guidelines suggest a newline after a closing brace.
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#two-newlines-after-closing-brace
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 198 at r3 (raw file):
`
Did we want to use backticks ( GetResult
) or single quotes ('GetResult') to surround GetResult
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.schema.mof, line 6 at r3 (raw file):
[Key, Description("Specifies the name of the _PostgreSQL_ database.")] String DatabaseName; [Key, Description("Path to the T-SQL file that will perform _Set_ action.")] String SetFilePath; [Key, Description("Path to the T-SQL file that will perform _Get_ action. Any values returned by the T-SQL queries will also be returned when calling _Get_ (for example by using the cmdlet `Get-DscConfiguration`) through the `'GetResult'` property.")] String GetFilePath;
Are the backticks needed?
tests/Unit/DSCResources/DSC_PostgreSqlScript.tests.ps1, line 107 at r3 (raw file):
Set-TargetResource @NoCreateParams Assert-VerifiableMock Assert-MockCalled Invoke-Command -Exactly -Times 2 -Scope It
We should use the parameter name -CommandName on all the Assert-MockCalled(s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 8 files reviewed, 14 unresolved discussions (waiting on @chtoone, @jcwalker, @jeffotterpohl, @rchristman89, and @regedit32)
README.md, line 39 at r2 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
I would add a period to unify all the descriptions.
Done.
README.md, line 42 at r2 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
Credential is not required but write.
Done.
source/PostgreSqlDsc.psd1, line 85 at r3 (raw file):
Previously, jcwalker (Jason Walker) wrote…
,
The comma isn't needed since the new resource is on a newline.
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 25 at r2 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
Missing period.
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 54 at r2 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
[System.Management.Automation.Credential()]
This allows you to pass in a username as a string and have an interactive prompt for the password, which I’ll demonstrate later in the post. I tested this out and it does not work in DSC. This can be removed.
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 151 at r3 (raw file):
Previously, jcwalker (Jason Walker) wrote…
$previousErrorActionPreference = $ErrorActionPreference $ErrorActionPreference = "Stop"
Why are we doing this vs adding a '-ErrorAction Stop' at the end the cmdlets?
Something with the way we are using Invoke-Command with the psql command line tool won't catch specific errors when using -ErrorAction Stop. If you set the EAP to stop, it does. I can show some examples if you're interested.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 152 at r3 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
I believe acronyms of two letters should be capitalized for camelCase.
Same with $db below
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 153 at r3 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
I would add a verbose statement before this saying it is "Listing databases" or something similar. It helps when debugging. It may be nice to put the command being ran in the statement.
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 166 at r3 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
I would add a verbose statement before this saying it is "Creating databases" or something similar. It helps when debugging. It may be nice to put the command being ran in the statement.
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 169 at r3 (raw file):
Previously, jcwalker (Jason Walker) wrote…
Style guidelines suggest a newline after a closing brace.
https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#two-newlines-after-closing-brace
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 170 at r3 (raw file):
Previously, rchristman89 (Ryan Christman) wrote…
I would move the verbose statement from above to here,
Write-Verbose -Message ($script:localizedData.ExecutingSetScript -f $SetFilePath,$DatabaseName) ```' It may be nice to put the command being ran in the statement. </blockquote></details> Done. ___ *[source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 198 at r3](https://reviewable.io/reviews/dsccommunity/postgresqldsc/5#-MKfHjED2dI1UEmnwCcj:-MKfUJajAOh1cb0c8aZ-:b-896fix) ([raw file](https://github.com/dsccommunity/postgresqldsc/blob/acadc69eedcc26b051f12737c063fff344ad3caf/source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1#L198)):* <details><summary><i>Previously, jcwalker (Jason Walker) wrote…</i></summary><blockquote> > ``` > ` > ``` Did we want to use backticks ( `GetResult`) or single quotes ('GetResult') to surround GetResult </blockquote></details> Done. ___ *[source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.schema.mof, line 6 at r3](https://reviewable.io/reviews/dsccommunity/postgresqldsc/5#-MKfJOEk9yKHXdh1xG8b:-MKfUMIW9RorD4Rl654W:ba3dfob) ([raw file](https://github.com/dsccommunity/postgresqldsc/blob/acadc69eedcc26b051f12737c063fff344ad3caf/source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.schema.mof#L6)):* <details><summary><i>Previously, jcwalker (Jason Walker) wrote…</i></summary><blockquote> Are the backticks needed? </blockquote></details> Removed ___ *[tests/Unit/DSCResources/DSC_PostgreSqlScript.tests.ps1, line 107 at r3](https://reviewable.io/reviews/dsccommunity/postgresqldsc/5#-MKfJA4l6ZcvoiQfxhIJ:-MKfUP9vDI0pdeVb03Zm:b-bu91xm) ([raw file](https://github.com/dsccommunity/postgresqldsc/blob/acadc69eedcc26b051f12737c063fff344ad3caf/tests/Unit/DSCResources/DSC_PostgreSqlScript.tests.ps1#L107)):* <details><summary><i>Previously, jcwalker (Jason Walker) wrote…</i></summary><blockquote> We should use the parameter name -CommandName on all the Assert-MockCalled(s) </blockquote></details> Updated <!-- Sent from Reviewable.io -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r4.
Reviewable status: 3 of 8 files reviewed, 6 unresolved discussions (waiting on @chtoone, @jcwalker, @jeffotterpohl, @rchristman89, and @regedit32)
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 151 at r3 (raw file): Previously, themitchk wrote…
No that's fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @chtoone, @jeffotterpohl, @rchristman89, and @regedit32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @chtoone, @jeffotterpohl, @rchristman89, and @regedit32)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r1, 2 of 5 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @chtoone, @jeffotterpohl, and @themitchk)
README.md, line 41 at r5 (raw file):
SetFilePath
TestFilePath
. I think we should also describe what the T-SQL script is expected to do in each Get/Set/Test. For example, should the Test script return boolean, error, or null if in desired state or not.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 76 at r5 (raw file):
Quoted 8 lines of code…
$returnValue = @{ DatabaseName = $DatabaseName SetFilePath = $SetFilePath GetFilePath = $GetFilePath TestFilePath = $TestFilePath GetResult = [System.String[]] $getResult } return $returnValue
Missing the value for CreateDatabase
. Get-DscConfiguration will throw if the resource does not return all settings defined in the schema.mof
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 202 at r5 (raw file):
Any script that does not throw an error is evaluated to true.
I noticed in the other function help, it has a different message for this parameter and says "Any script that does not throw an error or returns null is evaluated to true." I noticed we're not checking the output for null, so perhaps the other messages need to be updated. Or alternatively, maybe we should check the result, which would match the behavior for SqlServerDsc/SqlScript resource.
tests/Unit/DSCResources/DSC_PostgreSqlScript.tests.ps1, line 57 at r5 (raw file):
Mock Invoke-Command {return '<Script Output Sample>'}
We should use named parameters in all places. Here we're missing -CommandName
and -MockWith
. Same applies to the other mocks throughout, so I won't comment on each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 8 files reviewed, 4 unresolved discussions (waiting on @chtoone, @jcwalker, @jeffotterpohl, @rchristman89, and @regedit32)
README.md, line 41 at r5 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
SetFilePath
TestFilePath
. I think we should also describe what the T-SQL script is expected to do in each Get/Set/Test. For example, should the Test script return boolean, error, or null if in desired state or not.
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 76 at r5 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
$returnValue = @{ DatabaseName = $DatabaseName SetFilePath = $SetFilePath GetFilePath = $GetFilePath TestFilePath = $TestFilePath GetResult = [System.String[]] $getResult } return $returnValue
Missing the value for
CreateDatabase
. Get-DscConfiguration will throw if the resource does not return all settings defined in the schema.mof
Done.
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 202 at r5 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
Any script that does not throw an error is evaluated to true.
I noticed in the other function help, it has a different message for this parameter and says "Any script that does not throw an error or returns null is evaluated to true." I noticed we're not checking the output for null, so perhaps the other messages need to be updated. Or alternatively, maybe we should check the result, which would match the behavior for SqlServerDsc/SqlScript resource.
Good catch. Updated help text, as psql returns weird things and it's not trivial to parse a boolean out. If we need to adjust this later based on feedback we can
tests/Unit/DSCResources/DSC_PostgreSqlScript.tests.ps1, line 57 at r5 (raw file):
Previously, regedit32 (Reggie Gibson) wrote…
Mock Invoke-Command {return '<Script Output Sample>'}
We should use named parameters in all places. Here we're missing
-CommandName
and-MockWith
. Same applies to the other mocks throughout, so I won't comment on each
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 8 files reviewed, 4 unresolved discussions (waiting on @chtoone, @jcwalker, @jeffotterpohl, @rchristman89, and @regedit32)
source/DSCResources/DSC_PostgreSqlScript/DSC_PostgreSqlScript.psm1, line 202 at r5 (raw file):
Previously, themitchk wrote…
Good catch. Updated help text, as psql returns weird things and it's not trivial to parse a boolean out. If we need to adjust this later based on feedback we can
Done.
Pull Request
[PostgreSqlDsc] Added PostgreSqlScript resource
Pull Request (PR) description
Allows execution of T-SQL scripts against a PostgreSql Database
Added
Task list
build.ps1 -ResolveDependency
).and comment-based help.
This change is