Connect-DbaInstance: Fix problems with DAC

dataplat / dbatools

🚀 SQL Server automation and instance migrations have never been safer, faster or freer

https://dbatools.io

MIT License

2.39k stars 787 forks source link

Connect-DbaInstance: Fix problems with DAC #9376

Closed andreasjordan closed 3 weeks ago

andreasjordan commented 1 month ago

Type of Change

[x] Bug fix (non-breaking change, fixes #9253 )
[ ] New feature (non-breaking change, adds functionality, fixes # )
[ ] Breaking change (affects multiple commands or functionality, fixes # )
[ ] Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
[ ] Adding code coverage to existing functionality
[ ] Pester test is included
[ ] If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
[ ] Unit test is included
[ ] Documentation
[ ] Build system

Purpose

We need to prevent the cache to be used for DAC.

We need to close new connections if we do not return them.

Because of test refactoring, the diff of the tests is a bit hard to read - sorry for that.

andreasjordan commented 1 month ago

@wsmelton - If you have time, please have a look at the tests. Maybe you have additional suggestions.

potatoqualitee commented 3 weeks ago

good call, and what a ton of work! thank you very much 🙇🏼

potatoqualitee commented 3 weeks ago

Do you have any concerns about the changes that were made, @andreasjordan ? For example, did you have to change the substance of any tests to for them to work?

andreasjordan commented 3 weeks ago

There was no need to change any test (see that the tests on the first commit all succeded).

I then added new tests and refactored the tests to have a better coverage. There are two new tests that are not active because they fail ('keeps the same database context' fails in context 'connection is properly made using a dot' and 'connection is properly made using a connection object'). We have not tested this before and it's more an edge case. I will try to find a way to get it to work later or change the documentation of the command.

potatoqualitee commented 3 weeks ago

excellent, thanks again! 💯

00-schaudhary commented 1 week ago

Hi,

Is this change available in the latest update or is it still pending merge? If already available, I can update and test.

andreasjordan commented 1 week ago

Yes, it's included in the current version.

00-schaudhary commented 1 week ago

@andreasjordan - I updated dbatools to 2.1.18. Everything seems to be working fine now. Thank you for fixing this!

andreasjordan commented 1 week ago

Thanks for the feedback.