feat: add support for custom template databases#194
feat: add support for custom template databases#194munjalpatel wants to merge 2 commits intostripe:mainfrom
Conversation
|
|
bplunkett-stripe
left a comment
bplunkett-stripe
left a comment
There was a problem hiding this comment.
Looks really solid to me! Some small suggestions. Thanks for making these changes!
| { | ||
| opt: "timeout=15m", | ||
| expectedErrContains: "could not find key", | ||
| expectedErrContains: "could not find key", // "pattern" missing |
There was a problem hiding this comment.
nit: can we remove these. I understand why they were included, but I prefer not to have these comments, since they might quickly become outdated
| } | ||
| } | ||
|
|
||
| func TestPlanFlagsTemplateDBDefault(t *testing.T) { |
There was a problem hiding this comment.
It's probably okay to not include these, since we don't have analog tests. The functionality it is testing is pretty straightforward too, i.e., flag parsing
| metadataTable: DefaultOnInstanceMetadataTable, | ||
| rootDatabase: "postgres", | ||
| logger: log.SimpleLogger(), | ||
| templateDatabase: DefaultTemplateDatabase, |
There was a problem hiding this comment.
Instead of making this a parameter of the factory, let's make this a parameter of the Create function, since one factory doesn't need to be restricted to one type of template
| &flags.templateDb, | ||
| "template-db", | ||
| "template0", | ||
| "Template database to use when creating temporary databases", |
There was a problem hiding this comment.
Maybe add to this doc string: "This is used to derive implicit schema objects for the target schema"
|
|
||
| cmd.Flags().StringVar( | ||
| &flags.templateDb, | ||
| "template-db", |
There was a problem hiding this comment.
nit: name schema-template-db
|
@bplunkett-stripe Thanks for the suggestions on this PR! I’m happy to incorporate improvements. However, in my testing, I noticed an issue: when we create a new database from a template that already has extensions preloaded, and our SQL migrations don’t include Because of that, I’m not sure how much value this PR provides unless there’s a more straightforward way to ignore everything inherited from the template database (tables, functions, extensions, etc.). Do you have any thoughts on how we might address this? |
Hmmm I'm not sure why that would be occurring. @munjalpatel, could you:
|
2 participants
Description
This PR introduces a new flag,
--template-db, allowing users to specify the template database used when creating a temporary database. Previously,template0was hard-coded, limiting the ability to validate migrations in scenarios requiring a different template DB.Motivation
We frequently use various Postgres extensions that rely on different template databases for initialization, and we want to leverage migration validation against a temporary database. The prior hard-coded
template0prevented using migration validation with those extensions. By making the template configurable, we can accommodate more specialized Postgres environments and maintain robust validation flows.Testing
--template-dbflag.