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

Script action Target-* property write-out logic incorrect #10

Closed alexmaines closed 7 years ago

alexmaines commented 7 years ago

When using the Script action to make a script by, eg, comparing two dacpacs, a TargetDatabaseName is still required.
Sample commandline: sqlpackage /Action:Script /SourceFile:NewDatabase.dacpac /TargetFile:OldDatabase.dacpac /OutputPath:deployment.sql /TargetDatabaseName:NewDatabase

Although it is not clear why SqlPackage requires TargetDatabaseName in this scenario, it does. So, the series of "if" statements around this in the Runner, whereby TargetFile is in disjunction with the other collections of Target- settings, means that the command above cannot be generated by your code.

Happy to fix if you don't have time and you're willing to grant me access. Will ensure good unit test coverage in that case.

Regards Alex

alexmaines commented 7 years ago

I've forked the main repo to fix this issue. Once that's done, I'll post here so you can take the code.

alexmaines commented 7 years ago

Okay, that's done. and unit tests amended/extended. Please let me know if you want the code pushed back from the fork (or just merge my commit).

I feel I should say that I think the approach, of seeking to constrain the use of the Settings object passed to the tool, is mistaken. SqlPackage itself already embodies the allowed combinations of settings and should the programmer specify a incorrect combination, the tool will return a non-0 code and report the error. I'd recommend making the wrapper as dumb as possible and leave the tool itself to validate the input. At least that way, the validation rules stay in one place.

RLittlesII commented 7 years ago

@alexmaines First. Thanks for taking the time to identify and resolve this issue. Second. If you could submit a Pull Request that would be preferable.

To your point on the approach. I go back and forth with, safe guarding the user or letting the tool handle itself. In most scenarios I prefer to just let the tool resolve itself. In this case I was attempting to respect the rules that shouldn't allow individual Target or Source information if a file or connection string is provided. This isn't the first time I've been bitten by this, so I am learning my lesson of let the user figure it out for themselves.

RLittlesII commented 7 years ago

I merged the code changes in commit 7a6bffb57b29d179cf7cf2084274bbc1a61111d2. Thank you for your contribution and your advice on approach.