search

cake-contrib / Cake.SqlPackage

Cake AddIn that extends Cake for creating and deploying SQL Server databases and DACPAC packages.
https://cakebuild.net/extensions/cake-sqlpackage
MIT License
11 stars 9 forks source link

(GH-17) Expose ProcessSettings for SqlPackage to Cake alias methods #16

Closed remingtron closed 5 years ago

remingtron commented 5 years ago

My team is currently using the SqlPackagePublish method to initialize our integration test databases. This works great, but because our database schema is extremely large, it results in a lot of noise in our build logs. And because the Quiet option on SqlPackage.exe doesn't actually work, there's not currently a good way to disable this output. This PR makes it possible to suppress this standard output via ProcessSettings.RedirectStandardOutput, and also provides the ability to make any other tweaks a user would like by providing ProcessSettings to the Cake alias methods.

As I was making the change, I also tried to DRY some things up by creating a base Execute method on SqlPackageRunner, since all of the DeployReport/DriftReport/etc. methods were identical. I also created setup logic for the test classes, and removed a couple stray tests that appeared to have drifted into the wrong test classes (e.g. https://github.com/northwoodspd/Cake.SqlPackage/commit/ad2fb65e462cea76898f2354d49f558befc9e4b3#diff-066d048ee3f063a68f097fbbce6dfab5L413).

Lastly, to get the package to build, I ended up having to bump the version of Cake. I tried going back to an old version of Cake.Recipe, but I couldn't find one available that was compatible with the pinned version of Cake.

Thanks for your work on this library. We've found it very helpful, and hoped this PR could help make it even a tiny bit more for useful for others.

RLittlesII commented 5 years ago

@remingtron Thanks for taking the time to add this functionality.

A few comments. I noticed that you moved the test fixture instantiation to the constructor. I know it violates DRY principles, but I am not a purist in that space. I actually prefer to have it explicitly instantiated in each test. There by hopefully isolating each test fixtures state.

Those tests were explicitly added due to a previous contributor pointing out a bug, they exist to show that you can send in the parameter, the code will respect it, even though it doesn't belong.

There was a breaking change in Cake.Recipe. I know which version to pin it to and I can set that up, I am always cautious about releasing features tied directly to underlying dependency upgrades as I have had those problems in the past. I will release the next version with your changes, and then bump the cake version to give you a package that is on the latest Cake version.

I'll push to develop with the correct Cake.Recipe version this evening. Everything else looked good. Thank You.

remingtron commented 5 years ago

@RLittlesII - thanks for taking a look at this so quickly.

Re: the test fixture instantiation- I agree with your comment about DRY principles, especially with regards to test code if it obfuscates the readability of the test. Part of the reason I had moved in this case though is to make sure the thing under test is actually the correct type. The test I linked to before was actually creating an instance of SqlPackageExportFixture in SqlPackageScriptRunnerTests. Because of this, I thought they might have just been accidentally duplicated via copy/paste or something, so I went ahead and removed them. If they're really supposed to be there though, I can put them back and correct them to use the appropriate fixture.

I can also move the instantiation logic back down into each test if that's your preference. I haven't used xUnit.net before, but looking at the documentation, it says that the constructor logic gets run before each test, so you wouldn't need to worry about bleed over between them (https://xunit.github.io/docs/shared-context).

Let me know how you'd like to proceed, or if there's anything else you'd like me to address. Thanks!

RLittlesII commented 5 years ago

@remingtron If you wouldn't mind creating an issue just for tracking. I'll ping you once I'v completed the Cake.Recipe work and you can just rebase from there and we'll get this in asap. Thanks for you patience!

RLittlesII commented 5 years ago

Resolves #17

RLittlesII commented 5 years ago

@remingtron The package v0.8.0 just hit nuget.

remingtron commented 5 years ago

@RLittlesII - Awesome! Thanks for getting that integrated so quickly!

Just for my own understanding (since I haven't done any development with Cake before), will those new methods automatically show up in the API docs here? Wasn't sure if there was some batch process that generates those on some interval, or if those are maintained separately.

RLittlesII commented 5 years ago

@remingtron You're welcome. Thank you for the contribution.

The API docs may require a rebuild of the Cake site. There is some dll magic which at compile time of the website will load the latest nuget package and display whatever XML comments are present in the source. So call it eventually they will show up.