grate
grate copied to clipboard
Clean up sql server meta data
Fixes #243 (now even passes testing!)
Thanks for looking into this mate, some clean up is welcome!
I do have a couple of questions/concerns that I'll raise here without really knowing whether they're showstoppers from @erikbra 's point of PoV or not.
- By moving to 'modern' field types like
datetime2
we are losing support for old versions of Sql Server (2005?). MS clearly don't support that any more, and I wouldn't want to make changes just to support version that old, but I'm not sure we should be deliberately breaking support either. That said grate was meant to be the 'modern' RoundhousE.... - By excluding (say) read-only databases you've highlighted an existing bug (we try to write to a read-only database) and it's changed to a different bug (we try and create a duplicate of the read-only database). What was the scenario you hit that brought about this change?
Sorry it's taking some time to review all this stuff, time is precious for me atm :(
- Personally I'd say that 2005 is too old to be supported - it's not unreasonable to say that 1.40 is the last version to support 2005.
- It didn't make sense to me to try and consider read-only databases. Which issue is this other bug in - I could take a look at it and see if I can fix that as well.
I'm on a bit of a tear - having settled on grate
for our new deployment process I'm adding in functionality that grate doesn't have that I need. First off with the Dependency management (I raised a discussion on that - I also Cc'd you on it as well so you can chime in). Next up is having grate
able to handle executables as well as Sql Scripts.
Right now I'm going to be deploying on SQL Server, but after the new year I may well find myself needing to target Greenplum (a PostgreSql variant) - and I need both these two new features I'm working on. It's up to y'all if you're interested in consuming my upstream changes!
Sorry there's no existing bug logged, it's not something I'd considered either until I was was thinking about the impacts of your change.
@wokket So the issue is just ensuring we don't try to recreate an existing database?
If so, then that should be an easy fix. Let me take a butcher's at that in the AM tomorrow and see if I can clean that up with a pre-check.
If I nail that, how you feel about the PR then (@erikbra aside)?
Thanks a lot for you contribution, @RachelAmbler ! I appreciate more people becoming active in the development of grate!!
I don't have any problems replacing ntext with nvarchar(max). I know ntext is obsoleted, even if it still works. But, maybe better to change it, that't the point of obsoletion, isn't it, to give people (us) time to move away from it :)
I have a small question regarding the change in casing on the SQL server commands. Is there any special reason you have changed the keywords from CAPITAL to PascalCase? I tried to find some style guides for T-SQL here, but I couldn't find any by googling. Are you aware of any? Technically, it doesn't matter. I just wonder if there was a particular reason for the change.
Good catch on the "
instead of the brackets around reserved names. The former required
QUOTED_IDENTIFIER` to be set to ON if it should work, I hadn't seen that one.
This has been laying here a long time, and not been merged. The source code is restructured quite a lot since this PR, and #501 solved much of this, so I'll close this for now. Please open new issues if there are things left that needs updating/fixing, @RachelAmbler