-
Notifications
You must be signed in to change notification settings - Fork 6
Add schema migrations format #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
2d131e8 to
896390c
Compare
|
The handwritten |
|
@samczsun I agree that we should keep the comments. They are still inside |
330a0a5 to
9a5ef8b
Compare
9a5ef8b to
55d0cbb
Compare
f3a6499 to
a7e3013
Compare
marcocastignoli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple questions regarding the schema name. Good job! I like the approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in the new version we are forcing as schema public, would it be possible to keep it without the schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is not possible with pg_dump which is used by dbmate for creating the file (or at least there is no clean solution, see https://stackoverflow.com/questions/49108981/postgresql-pg-dump-without-schema-name).
Would you be fine with dumping to v1? I think if you load the db from the dump you can also rename the schema afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "no clean solution", do you mean for example running a replaceAll("public.", "")? Maybe that's enough and it works, idk.
I honestly don't know if it's better to use v1 vs public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "no clean solution", do you mean for example running a replaceAll("public.", "")? Maybe that's enough and it works, idk.
changing the file only via string matching seems error-prone to me. I think we should not change the dump like this. Besides, we would have to remove the SELECT pg_catalog.set_config('search_path', '', false); from the top of the dump. It's a security feature which is added so the dump is only loaded on the predefined schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okok, let's try to keep it like this for now and deal with it in the future only if we encounter the problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do agree that dumping to public does not feel ideal. We can easily dump to another schema name by specifying the search path on dbmate, but I ran into the problem that no extensions are dumped when doing so (which might be fixable if I dig deeper). However, I agree that we can leave it like it is now, since you would probably load the dump into a fresh db anyway if you use it.
marcocastignoli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Closes #31
Closes argotorg/sourcify#2199
This sets up the structure for schema migrations, which we need for managing changes to the schema. I introduced a tool called (dbmate)[https://github.com/amacneil/dbmate] for this purpose. The reasons for dbmate are mainly that it uses raw SQL files, it is language and framework agnostic and writes a full schema file automatically.
The changes include:
database.sqlto be an automatically generated file by dbmatedatabase.sqlas the one comitted (via git diff)contract_deploymentstable argotorg/sourcify#2199)