Script action Target-* property write-out logic incorrect
Closed this issue · 4 comments
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
I've forked the main repo to fix this issue. Once that's done, I'll post here so you can take the code.
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.
@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.
I merged the code changes in commit 7a6bffb. Thank you for your contribution and your advice on approach.