Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed error action and added example scripts #9

Merged
merged 14 commits into from
Dec 17, 2020

Conversation

rchristman89
Copy link
Contributor

@rchristman89 rchristman89 commented Dec 11, 2020

Pull Request (PR) description

Fixing the output causing Test in SqlScript to not report back errors.

Task list

  • The PR represents a single logical change. i.e. Cosmetic updates should go in different PRs.
  • Added an entry under the Unreleased section of in the CHANGELOG.md as per format.
  • Local clean build passes without issue or fail tests (build.ps1 -ResolveDependency).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

Copy link
Contributor

@themitchk themitchk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 8 files at r1.
Reviewable status: 4 of 8 files reviewed, 1 unresolved discussion (waiting on @rchristman89)


source/Examples/Resources/PostgreSqlScript/TestSqlFiles/test.sql, line 20 at r1 (raw file):

BEGIN
IF NOT exists (SELECT * FROM information_schema.tables Where table_schema = 'public' AND table_name = 'Users')
THEN RAISE INFO 'hi';

Should Probably change this message from 'hi'.

Copy link
Contributor

@themitchk themitchk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 8 files at r1.
Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @rchristman89)

Copy link

@jcwalker jcwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thought on the using Start-Process

@@ -125,14 +128,14 @@ function Set-TargetResource
Write-Verbose -Message ($script:localizedData.CreatingDatabase -f $DatabaseName)
Invoke-Command -ScriptBlock {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any benefit to using Start-Process here? When calling an .exe from PowerShell I like to use Start-Process -Wait so ensure the script will not progress until the .exe is finished. I have found in the past that PowerShell will call the .exe and then keep on going. This would also allow you to inspect the exitCode to verify success. It might also remove the requirement to change the built-in variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can try it and see how it goes.

Copy link
Contributor Author

@rchristman89 rchristman89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @jcwalker and @themitchk)


source/DSCResources/DSC_PostgreSqlDatabase/DSC_PostgreSqlDatabase.psm1, line 129 at r1 (raw file):

Previously, rchristman89 (Ryan Christman) wrote…

We can try it and see how it goes.

Done.


source/Examples/Resources/PostgreSqlScript/TestSqlFiles/test.sql, line 20 at r1 (raw file):

Previously, themitchk wrote…

Should Probably change this message from 'hi'.

Done.

@rchristman89 rchristman89 merged commit 14863b2 into master Dec 17, 2020
@rchristman89 rchristman89 deleted the bugfixwithErrorOutput branch December 17, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants