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

SqlTraceflag: Correctly loads the SMO assemblies #1682

Merged
merged 24 commits into from
Jan 30, 2021

Conversation

Fiander
Copy link
Contributor

@Fiander Fiander commented Jan 29, 2021

Pull Request (PR) description

SMO was not imported correctly. Fixed by importing sql smo.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry to the change log under the Unreleased section of the
    file CHANGELOG.md. Entry should say what was changed and how that
    affects users (if applicable), and reference the issue being resolved
    (if applicable).
  • Resource documentation updated in the resource's README.md.
  • Resource parameter descriptions updated in schema.mof.
  • Comment-based help updated, including parameter descriptions.
  • Localization strings updated.
  • Examples updated.
  • Unit tests updated. See DSC Community Testing Guidelines.
  • Integration tests updated (where possible). See DSC Community Testing Guidelines.
  • Code changes adheres to DSC Community Style Guidelines.

This change is Reviewable

@Fiander
Copy link
Contributor Author

Fiander commented Jan 29, 2021

@johlju
I have updated the two functions by adding
Import-SQLPSModule 4>$null

This imports the module, but supresses the verbose message about loading the module.
I think it would make sense to also do that in the Connect-SQL function.
That would also make the test logs a lot smaller.

In the same function we could also remove the
-Verbose
part in
Write-Verbose -Message ( $script:localizedData.ConnectingUsingImpersonation -f $connectUsername, $LoginType ) -Verbose
That way, the Write-Verbose follows what Connect-SQL is been called with.

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #1682 (bb48262) into main (8a040bf) will increase coverage by 0%.
The diff coverage is 100%.

Impacted file tree graph

@@         Coverage Diff          @@
##           main   #1682   +/-   ##
====================================
  Coverage    97%     97%           
====================================
  Files        38      38           
  Lines      6259    6261    +2     
====================================
+ Hits       6102    6104    +2     
  Misses      157     157           
Flag Coverage Δ
unit 97% <100%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...SCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1 96% <100%> (+<1%) ⬆️

@johlju johlju added the needs review The pull request needs a code review. label Jan 29, 2021
@johlju
Copy link
Member

johlju commented Jan 29, 2021

@Fiander those -Verbose are due to a bug in LCM running resources (which most likely will never be fixed), but there is a way around that. We are tracking it in issue #1564. Let's fix the verbose problem when we fix it in that issue.

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.

Thank you for fixing this issue. Just tiny comments then this is ready to merge.

Reviewed 2 of 10 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Fiander)

a discussion (no related file):
Please add a mock for Import-SQLPSModule in the unit test for the respective function.



CHANGELOG.md, line 9 at r4 (raw file):

Remove full stop (.) here, there is already (correctly) a full stop after the issue link.


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 36 at r4 (raw file):

#Import SqlServer module but suppress verbose message.

We should have a space after the #, # Import....


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 37 at r4 (raw file):

 4>$null

Let us remove this so this won't be a problem when we fix issue #1564.


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 151 at r4 (raw file):

#Import SqlServer module but suppress verbose message.

We should have a space after the #, # Import....


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 152 at r4 (raw file):

4>$null

Let us remove this so this won't be a problem when we fix issue #1564.

@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 Jan 29, 2021
@johlju johlju changed the title SqlTraceflag: fix for 1680 SqlTraceflag: Correctly loads the SMO assemblies Jan 29, 2021
@johlju
Copy link
Member

johlju commented Jan 30, 2021

Kicked off the tests again.

@johlju johlju self-requested a review January 30, 2021 09:08
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.

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @Fiander and @johlju)


tests/Unit/DSC_SqlTraceFlag.Tests.ps1, line 170 at r5 (raw file):

-MockWith {return}

We can skip this part if we should not return anything. It is enough with Mock -CommandName Import-SQLPSModule


tests/Unit/DSC_SqlTraceFlag.Tests.ps1, line 357 at r5 (raw file):

 -MockWith {return}

We can skip this part if we should not return anything. It is enough with Mock -CommandName Import-SQLPSModule

Copy link
Contributor Author

@Fiander Fiander 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: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @Fiander and @johlju)


CHANGELOG.md, line 9 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Remove full stop (.) here, there is already (correctly) a full stop after the issue link.

Done.


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 36 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
#Import SqlServer module but suppress verbose message.

We should have a space after the #, # Import....

Done.


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 37 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
 4>$null

Let us remove this so this won't be a problem when we fix issue #1564.

Done.


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 151 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
#Import SqlServer module but suppress verbose message.

We should have a space after the #, # Import....

Done.


source/DSCResources/DSC_SqlTraceFlag/DSC_SqlTraceFlag.psm1, line 152 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
4>$null

Let us remove this so this won't be a problem when we fix issue #1564.

Done.


tests/Unit/DSC_SqlTraceFlag.Tests.ps1, line 170 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
-MockWith {return}

We can skip this part if we should not return anything. It is enough with Mock -CommandName Import-SQLPSModule

Done.


tests/Unit/DSC_SqlTraceFlag.Tests.ps1, line 357 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
 -MockWith {return}

We can skip this part if we should not return anything. It is enough with Mock -CommandName Import-SQLPSModule

Done.

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 2 of 3 files at r5, 1 of 1 files at r6.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Fiander)

@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jan 30, 2021
@johlju johlju merged commit 511b365 into dsccommunity:main Jan 30, 2021
@johlju
Copy link
Member

johlju commented Jan 30, 2021

@Fiander thank you for this fix! Much appreciated you took the time! 😃

I will shortly push a PSSA rule soon that will help detect these kind of issues in the future.

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.

SqlTraceFlag: Assembly not loaded
2 participants