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.

Sign up for GitHub

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

Jump to bottom

Invoke-query updated to allow smo object #1380

Merged
merged 6 commits into from
Jun 30, 2019
Merged

Invoke-query updated to allow smo object #1380

merged 6 commits into from
Jun 30, 2019

Conversation

kungfoome
Copy link

@kungfoome kungfoome commented Jun 24, 2019 •
edited
Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Fixes #1355
Closes #1356

Task list

  • Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • 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

@codecov-io
Copy link

codecov-io commented Jun 24, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##            dev   #1380    +/-   ##
=====================================
+ Coverage    98%     98%   +<1%     
=====================================
  Files        36      36            
  Lines      5344    5354    +10     
=====================================
+ Hits       5238    5248    +10     
  Misses      106     106

@johlju johlju added the needs review The pull request needs a code review. label Jun 25, 2019
johlju
johlju requested changes Jun 25, 2019
View reviewed changes
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfu71186)

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1617 at r2 (raw file):

SQLServer Server1 -SQLInstanceName MSSQLSERVER

These parameters should not be needed when piping using an *.Server object?

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1668 at r2 (raw file):

# If we don't have an smo object, then we try to use credentials

This comment seem misplaced to me, should it be moved or just removed?

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1671 at r2 (raw file):

$smoConnectObject

Can we leave the previous variable name $serverObject? It is the actual *.server object that is returned (not a connect object that should be used to get a server object?).

Throughout.

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1675 at r2 (raw file):

Paramaters

Parameters Typo. 🙂

Throughout.

Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 1532 at r2 (raw file):

Context 'Execute a query with piped SMO server object' {

It is not really using piping in this context block, it using the SqlServerObject parameter.

Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 1553 at r2 (raw file):

Context 'Execute a query with results' {

Here it is using piping so maybe the description in the context block should say that?

@johlju johlju added in progress The issue is being actively worked on by someone. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. in progress The issue is being actively worked on by someone. labels Jun 25, 2019
kungfoome
kungfoome commented Jun 27, 2019
View reviewed changes
Copy link
Author

@kungfoome kungfoome 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: all files reviewed, 6 unresolved discussions (waiting on @johlju)

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1617 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
SQLServer Server1 -SQLInstanceName MSSQLSERVER

These parameters should not be needed when piping using an *.Server object?

Done.

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1668 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
# If we don't have an smo object, then we try to use credentials

This comment seem misplaced to me, should it be moved or just removed?

Done.

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1671 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$smoConnectObject

Can we leave the previous variable name $serverObject? It is the actual *.server object that is returned (not a connect object that should be used to get a server object?).

Throughout.

Done.

Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 1675 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Paramaters

Parameters Typo. 🙂

Throughout.

Done.

Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 1532 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Context 'Execute a query with piped SMO server object' {

It is not really using piping in this context block, it using the SqlServerObject parameter.

oops. context descriptions got flipped.

Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 1553 at r2 (raw file):

oops. context descriptions got flipped.

@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 27, 2019
johlju
johlju requested changes Jun 27, 2019
View reviewed changes
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kungfu71186)

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

- Changes to common module: Invoke-Query

There was a release yesterday, so could you please rebase, then make sure this change log entry is moved back to the Unreleased section.

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jun 27, 2019
@kungfoome
Copy link
Author

@johlju hopefully good now

@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 30, 2019
johlju
johlju approved these changes Jun 30, 2019
View reviewed changes
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju johlju merged commit 943c878 into dsccommunity:dev Jun 30, 2019
@johlju johlju removed the needs review The pull request needs a code review. label Jun 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johlju johlju johlju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Common Resource: Invoke-Query
3 participants
@kungfoome @codecov-io @johlju