Add PostgreSQL as DB Option #275
Conversation
We add a database type, which is defaulted to sqlite
nickmurray47
force-pushed
the
nickmurray/add-postgres-db-option
branch
from
6e5f395
to
816d823
Compare
We need to swap out datetime with timestamptz in order to properly allow for a postgres DB provider
|
to-do:
|
Users can select database type using database.type configuration when calling NewRepository()
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| Select(strings.Join(selectClauses, ", ")). | ||
| Where(strings.Join(whereClauses, " AND "), whereNamedParameters). | ||
| Group(groupClause). | ||
| Order(orderClause). |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
|
Hey Nick, I'm worried that this PR might end up requiring a long-lived branch if we start making the "common" repository changes and adding the Postgres repository stuff now |
Yes, I was actually going to stop here and ping you for a review. Do you still want to move the "common" + postgres changes out? I just re-tested with SQlite as default and everything appeared operational. |
|
Honestly I’ll just worried that there are changes in the main branch that
may not be reflected in yours. Do you have time for a chat on Monday
morning?
…On Sat, Oct 14, 2023 at 4:03 PM Nicholas Murray ***@***.***> wrote:
I'm worried that this PR might end up requiring a long-lived branch if we
start making the "common" repository changes and adding the Postgres
repository stuff now
Can we move that to another PR, and merge just the Factory pattern + model
changes?
Yes, I was actually going to stop here and ping you for a review. Do you
still want to move the "common" + postgres changes out? I just re-tested
with SQlite as default and everything appeared operational.
—
Reply to this email directly, view it on GitHub
<#275 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGZXYZ3ARHF4OKK6BBNDEDX7MK4PANCNFSM6AAAAAA52XS73A>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Yeah, can chat Monday morning!
re: will talk further on this in chat, but any preference on merge commit vs. rebase? |
|
I think the merge commit is fine. |
Picked up on most recent change with DeleteSource func
nickmurray47
changed the title
|
Merged main and made sure we picked up on most recent commit c590663537faab12d4416edd3f7756b678ee5460 to |
| @@ -47,6 +51,22 @@ PLEASE DO NOT EDIT BY HAND | |||
| `, "\n"), "\n") | |||
|
|
|||
| func main() { | |||
| // Read config file for database type | |||
| appconfig, err := config.Create() | |||
There was a problem hiding this comment.
Compile time vs runtime.
The user's configuration file will not be present during compilation time, so the database type choice has not been made yet.
We need to generate both Sqlite & Postgres compatible structs (in different directories).
The generated utils.NewFhirResourceModelByType fn should take in the DatabaseRepositoryType as a parameter, and conditionally point to a different set of generated structs.
We'll make this change in a follow up PR.
Testing steps:
go mod vendorconfig.yamlto havepostgresas database typefasten-onprem/backend/pkg/models/databaserungo run generate.goand verify that all the database models have swapped outdatetimefortimestamptzDiscussion with @AnalogJ and @nickmurray47 about followup work