Migrations for cr3 location triggers #1290
Conversation
…s for calculating location_id
…function (should have done this in earlier PR)
roseeichelmann
requested review from
frankhereford,
patrickm02L and
chiaberry
| - "!include public_find_location_for_noncr3_collision.yaml" | ||
| - "!include public_find_location_for_cr3_collision.yaml" |
There was a problem hiding this comment.
I ran into an inconsistent metadata warning when I applied the Hasura metadata locally. The first line added back here is a function that was dropped in #1286 and the second one is being dropped in this PR. These two lines can go since Hasura doesn't need to worry about tracking or not tracking them anymore.
| DROP FUNCTION find_cr3_mainlane_crash; | ||
| DROP FUNCTION find_location_for_cr3_collision; | ||
| DROP FUNCTION find_location_id_for_cr3_collision; |
There was a problem hiding this comment.
Just to make sure that we can migrate down and then back up again, these drop statements could use IF EXISTS since they aren't added back in the down migrations. I ran into this in my PR too.
There was a problem hiding this comment.
thanks for catching this!
There was a problem hiding this comment.
This works great! I also tested nulling out latitude, longitude, latitude_primary, and longitude_primary because you caught that on mine, and I saw the location id clear out. 🚀
|
@mddilley thanks for catching those things, just pushed up those updates! |
|
@roseeichelmann awesome, just tested and there is a small bug with the syntax |
|
@mddilley ahh sorry i should have tested the migration again before pushing up lol but i just fixed that and tested the down/up and metadata, shoulddd be good to go now |
There was a problem hiding this comment.
Rose, I'm curious what you think about if we need to keep these migration-like files in here with proper graphql-engine migration support in place. Like, I think now if I wanted to find what the latest and greatest version of this function is, I would probably get into the root of the repo and do:
find atd-vzd/migrations/default -name 'up.sql' -exec grep -l 'update_cr3_location' {} + | sort -rThe top line in the response is the most recent up.sql that mentions the string and going backwards is its history.
I totally think I may be missing something, and I'm super curious to what your reaction is.
CC/ @mddilley too - I'm curious if you have a take on this as well. Thanks to you both!!!
There was a problem hiding this comment.
I may be misunderstanding what your comment is Frank, are you saying you don't see the need for this file because you could execute that command to find what the latest version is?
I do not work much in VZ, but I do find it easier that in moped we have the views in their own files to be able to easily see changes between migrations, and in moped the top of the file has what up.sql file is the latest aka
-- latest version 1694809058492_list_view_optimize
There was a problem hiding this comment.
Hi Chia, thanks for your comment. I'm sorry for not being very clear, but I think you've hit on it exactly. I'll try to add some more details, but first, I'll say that leaving the file in is totally fine if it's useful for devs. 👍
To your question about the top of the file -- what that command will do is tell you what files to look in, but not actually show you the contents of the file. You still need to open it up manually to read it.
The reason why I like to look in the migrations for the current state of the database is that they are what was actually applied to the database, so I feel like they have a very high likelihood of being accurate. We've struggled in the past with the VZ repo to keep all the "migration-like" files up to date when a change went in, mostly because changes were hand applied and it was easy for people to forget to update the views/triggers and what-have-you when they made the change, especially if it was an iterative process.
Now, that said, we've gotten a heck of a lot better at it, so if we continue to excel at that recording of the view and the files are helpful, then I'm all in for keeping them.
Thanks for asking!
There was a problem hiding this comment.
Ah yes, I understood that it would tell you what file to look in, but not the contents. In moped we manually add that line to the top of the git tracked file so devs can see what the latest migration is, and having the file is to help at a glance see changes in the PR from one migration to another.
There was a problem hiding this comment.
Yeah im cool with leaving this file out of the repo, I honestly was kinda just following Mikes lead and saw that he left his trigger function in his similar PR for the blueform crashes so figured I would do the same thing, but I agree that it's not necessary and maybe will lead to confusion now? What do you think @mddilley
There was a problem hiding this comment.
hey y'all - late to the party, but I think that we can leave it in for now and decide later so this can 🚢. I agree with Chia that the Moped views folder is great for seeing a diff during reviews, and we've succeeded at keeping files up to date over time in that project. I created cityofaustin/atd-data-tech#13950 to bring to a future refinement meeting.
Associated issues
Closes cityofaustin/atd-data-tech#13055
Testing
URL to test:
Local
Steps to test:
hasura migrate apply --down 1 --envfile .env.localShip list