-
Notifications
You must be signed in to change notification settings - Fork 225
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
SqlServerDsc: Using dbatools as a preferred module #1904
SqlServerDsc: Using dbatools as a preferred module #1904
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1904 +/- ##
====================================
- Coverage 92% 92% -1%
====================================
Files 91 91
Lines 7725 7732 +7
====================================
+ Hits 7122 7127 +5
- Misses 603 605 +2
|
33f43cf
to
50fee59
Compare
k, it's updated on my end 👍🏼 |
@potatoqualitee now the build fail on https://dev.azure.com/dsccommunity/SqlServerDsc/_build/results?buildId=7777&view=logs&j=0e1de302-4f5f-5cc3-ef9c-00ac523cd866&t=1c0b58bc-930f-54e7-60af-0caf301f2f32&l=1582
|
That is the strangest thing and so far I cannot replicate. Do you have a guide for building and testing? I tried to follow along but got lost. |
I will see if I can make an easier repro than running the integration tests in CI. I might not have time for a few days though. I let you know. 😊 |
c780509
to
b08d606
Compare
@potatoqualitee I see a lot of I wonder if the module tries to do something interactive that is not allowed in the session that LCM runs. I tried to use it outside of the DSC resource and there it imports as expected, so it must have to be something related to running the DSC resources. |
Yes, you can use Also, we do use decompression during our import, perhaps that's prohibited? |
We always clone the repo with the following
Unsure. Haven't heard of any restrictions other than interactive. Need to fins out what row it fails on in dbatools when it is imported, so probably have to run a DSC resource in a lab and then fill dbatools .psm1 with verbose messages or something so I can find where it fails 🙂 It feels SqlServerDsc fails here when it tries to import dbatools on the line below. Running the same command in the console it works as expected. |
So this failed in a I tried removing the message that outputs during import just in case that was preventing it from working. It is run just after the module is installed, We can see below that the install worked, but then it failed to find the dbatools command. Probably because the module could not imported like we saw previously. Ignore that it say
|
okie dok, I removed the compression import. please try now with preview4 (sorry, i dont understand the implication of the Script comment 😅 ) |
Cool. Will test tomorrow. Script resource just simplifies making a repro, but haven’t gotten that far. Hopefully I have time this weekend to make an easy repro if your latest fix does not work. |
(I can't see the error right now because I'm on mobile so I'm making assumptions.) Yay, still failed so I won't have to remove the compression 😅 Happy to work with you this weekend to see what's going on. |
It also fails in AppVeyor so that rules out the build workers of Azure DevOps and GitHub Actions. I will try to make some time to create an easy repro. |
I know very little of what y'all doing and how dbatools work beyond the title of this issue, but I thought I'd point out that IF dbatools is using ScriptsToProcess to do something before it loads, that won't work in DSC (wmf5.1), and I don't know for Machine Config (but there's bigger chance it works there). |
Renamed the title to better describe what this goal is, if possible (this is just a PoC). I wanted to see how far dbatools module works, because it supports assemblies that is not supported (yet) by SqlServer module. It would be opt-in for a user. Though it probably won't be able to be used in other areas since it lack the necessary commands that Sqlserver has - and SqlServer and dbatools might not be able to be used concurrently since both have (or can diverge to) different SMO assemblies since they are sourced from the SMO repository at different times. It would have been better if both dbatools and SqlServer sourced the same assembly versions at the same time, but the Community (@potatoqualitee) is faster here than the SQL Server team those updating SqlServer module (like once a year if we are lucky). |
The dbatools module has
|
I will look into making a repro this morning. |
No, that should be ok. |
Repro: Run this preferably on a lab machine since it changes stuff to be able to run LCM that is normally not configured. All must be run as elevated user in Windows PowerShell. I ran it on a Windows 11 22H2, but would of course work on server flavors too. I ran it on a clean Windows 11 so there are more steps that might be need but you all know this (just a reminder to myself). 🙂
|
Should result in the error being shown in the output
|
It seems it fails on line 206 and 216 in this image See output.
If running the command manually if fails with the same error since the folder does not exist, Unclear why it cuts of the full path. PS > Get-ChildItem -Path "C:\Program Files\WindowsPowerShell\Modules\dbatools\2.0.0/private/functions/" -Recurse -Filter *.ps1
Get-ChildItem : Cannot find path 'C:\Program Files\WindowsPowerShell\Modules\dbatools\2.0.0\private\f' because it does not exist.
At line:1 char:1
+ Get-ChildItem -Path "C:\Program Files\WindowsPowerShell\Modules\dbato ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : ObjectNotFound: (C:\Program File...2.0.0\private\f:String) [Get-ChildItem], ItemNotFoundException
+ FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetChildItemCommand @potatoqualitee The solution should probably be to evaluate if the path exist before trying to fetch files? |
Testing for paths passed the errors, using this code:
resulted in
I came passed that error, but now if fails on something else... Gonna see if I can find what it fails on now. |
Ah, there a more of the same isuee, justbelow there are one more, that need to check if path exist.
|
It also seems the module is import over and over again. The .psm1 is run several times, then it fails.
|
Okay, it seems it loads several times because the .psm1 tries to dogfood its own commands before it is fully imported. Removing code that have Dba commands solves that. Commands that should be dogfood maybe need to be in a dbatools.common module or something? Commented code: #if (-not ([Dataplat.Dbatools.Message.LogHost]::LoggingPath)) {
# [Dataplat.Dbatools.Message.LogHost]::LoggingPath = Join-DbaPath $script:AppData "PowerShell" "dbatools"
#}
...
$loadedModuleNames = (Get-Module sqlserver, sqlps -ErrorAction Ignore).Name
if ($loadedModuleNames -contains 'sqlserver' -or $loadedModuleNames -contains 'sqlps') {
#if (Get-DbatoolsConfigValue -FullName Import.SqlpsCheck) {
# Write-Warning -Message 'SQLPS or SqlServer was previously imported during this session. If you encounter weird issues with dbatools, please restart PowerShell, then import dbatools without loading SQLPS or SqlServer first.'
# Write-Warning -Message 'To disable this message, type: Set-DbatoolsConfig -Name Import.SqlpsCheck -Value $false -PassThru | Register-DbatoolsConfig'
#}
}
...
#if (Get-DbatoolsConfigValue -FullName Import.EncryptionMessageCheck) {
if ($false) {
$trustcert = Get-DbatoolsConfigValue -FullName sql.connection.trustcert
$encrypt = Get-DbatoolsConfigValue -FullName sql.connection.encrypt
if (-not $trustcert -or $encrypt -in "Mandatory", "$true") {
# keep it write-host for psv3
Write-Message -Level Output -Message '
/ / / /
| O | | O |
| |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -| |
...
Set-DbatoolsConfig -Name Import.EncryptionMessageCheck -Value $false -PassThru |
Register-DbatoolsConfig'
}
} Then it doesn't try to re-import itself, and fails on another location... digging further.
|
Adding a try catch statement around this command shows that is too tries to dogfood itself before it is fully imported.
|
3c30d4d
to
6372e2a
Compare
oh yeah, it'd be much better to use the SMO equivalents of those queries. Considering so many parameters would have to be added, I agree that going another route would be better. It's been a pleasure working with you! |
I made separate jobs that tests dbatools so that both options are tested. As it turns out it seems SQLPS can coexist with dbatools (at least when dbatools is imported first) so the resource SqlScript and SqlScriptQuery worked an called All tests pass. Merging this. |
@potatoqualitee Same, been a pleasure! Thank you for great work in dbatools. 🙂Personally I don't use dbatools as a daily driver, but have colleagues that do. In my current role maintaining this module is the closest to managing SQL Server I come now days 🙂 |
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 7 of 7 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @johlju)
Amazing, merged! 🚀 This is so cool. I actually hadn't worked much in prod with SQL for a few years but now I'm back to being a DBA and will get to work on dbatools more. How/when can I share the info about dbatools being an option in SqlServerDsc? Unsure about your release cycle. |
It is live on the gallery already as a 16.3.0-preview0004. All merges to main are automatically deployed (thanks to the pipeline from the Sampler-project). Though it took a bit longer because the test pipeline failed with intermittent error (due to not enough resources in Azure DevOps) so had to re-run the failed jobs this afternoon. |
I will push a full release now and when. I will probably push a full release after PR #1916 is merged since it is a problem for users with SQL 2022. |
Pull Request (PR) description
This Pull Request (PR) fixes the following issues
None.
Task list
file CHANGELOG.md. Entry should say what was changed and how that
affects users (if applicable), and reference the issue being resolved
(if applicable).
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)