-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add PostgreSQL as DB Option #275
Conversation
We add a database type, which is defaulted to sqlite
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()
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
user-provided value
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 vendor
config.yaml
to havepostgres
as database typefasten-onprem/backend/pkg/models/database
rungo run generate.go
and verify that all the database models have swapped outdatetime
fortimestamptz
Discussion with @AnalogJ and @nickmurray47 about followup work