Implement extra status column in version table #196
Conversation
|
Automatic tests are done, still need to test locally. Any ideas on how to avoid breaking current grate implementations are welcome! |
|
I've just had a quick look and this PR looks really good for first timer 👍
That's definitely a concern here, can you take a leaf out of the pattern we use at runtime and apply the new column only if it's needed? ie:
|
|
Thank you! I thought about getting the list of columns from the version table and adding it if needed, however i wanted to avoid querying this all the time as we should avoid database trips that don't provide anything. Do we have a way of flagging a step as "Done" for further runs? So it would only run the "Check status column in version table" sql the first time it runs with the new version? |
|
Struggling a bit with the testing of the feature for all supported databases, but still wanted to show the progress I've made. Not sure if i like how i did it here, i have a feeling it's to complex for just a simple column, but feel free to chime in! |
|
I've been pretty busy and haven't had a chance to look at this, but:
I wouldn't stress too much about this. While it's laudable (and we never want to be too inefficient) we are a database migration tool, it's expected that we're going run a lot of queries against the server.
You could write it to the |
|
Thanks a lot for this effort, @DjKillerMemeStar ! I think this is where we see a small deficiency in grate compared to RoundhousE, where we have our roots from. RoundhousE used NHibernate for the data migration tables, which I thought was a bit of an overkill, but this particular feature would definitely be easier to implement using a tool that specifically handles table versioning out-of-the-box. I'm not keen on re-introducing the dependency on NHibernate, as it seems a bit of a huge dependency, so we need to handle this in some other way. Some kind of "schema version" on the grate tables could be one solution, but we would of course handle existing databases, both with old and new "schema versions". Could you please explain a bit about what you need the "Status" column for? What is your particular use case? |
|
Hi @erikbra Me personally i don't have a use case for the "Status" column, i do however see a use in having it. For example to know what migration (version) went wrong. The main reason i wanted to do this is because i'm interested in supporting grate and solving this issue #112. I agree with your NHibernate point, it's a bit much just for table versions. |
This reverts commit 7d620da. Try again
|
I had to take a step back, because i was making it a bit to difficult for myself but i found a nice solution and i like how the code is written! I just need to test it locally and then we are done! |
Add status to version table
…rsion_Table Revert "Add status to version table"
|
Validated locally on an version without status column and it gets added correctly! |
|
I like this approach much better. Now we just need the Sqlite tests passing and we can get it into a pre-release for you. |
|
@wokket wierd thing is that "it works on my machine" 🤔 I'll have a look into this after my holiday! |
|
This looks very nice, @DjKillerMemeStar ! Thank you. We must try to sort out the tests failing, though. It might be a concurrency issue. I know some of the tests might not be as fool-proof to changes as one might want. I'll try to look into the failures as well, when I get a chance. |
|
I'd love to, but I'm too dumb to work out how 🤣 If you just touch a file and push it should re-trigger the CI? |
|
Hi, @DjKillerMemeStar , I've been off on vacation, so I'm just looking at this now. I tried to check out the PR, and create a branch in this repo from it, as I couldn't manage to run a build on the PR branch either. However, it seems something strange has happened to the branch? Look at the "Changes" tab on this PR, it says "0 changes", compared to erikbra/main. Do you know what has happened here? |
|
@DjKillerMemeStar - did you get a chance to look into this? The PR still says "0 changes" against main |
|
@erikbra Yeah i've looked into it but i have no idea what happend, i think i might just do a new branch and PR and see if that fixes it. |
|
@erikbra I've figured it out! I reverted the changes i did on my own fork, that is why this PR had no files changed. Stupid of me! But blame it on the fact that i don't use Github/fork alot :D But this should be fine now! |
|
I've tested this locally on a real code base with existing SQL scripts, and it seems to be working great, @DjKillerMemeStar ! Great work, thanks! I'm ready to merge this into main, adding it to a 1.4 release, which I'd like to get out soon :) I see that we do have a couple of conflicts with main now, as other PRs have been merged in, changing main from where you started. Are you comfortable resolving those conflicts, or would you like me to assist in some way? |
|
@DjKillerMemeStar i'm on holifay until the middle of September so it might be a while untill i get to it. So it may be beter if you can have a look real quick, if you don't mind ofcourse. Otherwise it will have to wait and we might get more issues. My code is uittested so you should be able to merge pretty confident. Sorry for the issues and time delat with this one, i'll try to do beter on the next! |
erikbra
force-pushed
the
Add_Status_To_Version_Table
branch
from
4ff2d78 to
485a9da
Compare
|
Thanks a lot for your efforts, @DjKillerMemeStar , and enjoy your vacation! After a few stumbles and errors I managed to et the conflicts fixed :) |
No description provided.