-
Notifications
You must be signed in to change notification settings - Fork 4
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
added initial files for qc #39
base: main
Are you sure you want to change the base?
Conversation
work in progress on PR |
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 plan to run a build from this branch shortly, but I wanted to post my initial questions and comments!
docker/query-connector/vs_dump.sql
Outdated
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.
How long does the db-creation
script take?
Is there another place this dump is stored, like in the query connector repo? If so, we could reference the raw file in that repo.
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.
It doesn't take long, the dump is also stored in the dibbs-query-connector
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.
@rin-skylight @shanice-skylight We aren't going to need the dump in this repo because the provision.sh
script clones the dibbs-query-connector
repo since it's in that repo as linked; we need to make sure the docker file mount source is correct.
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.
@alismx Can you reference where in the script it clones dibbs-query-connector
, I can't find it?
@@ -0,0 +1,49 @@ | |||
services: | |||
# PostgreSQL DB for custom query and value set storage | |||
db: |
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.
Is this required for the build to succeed? Is the database going to be bundled into the VM for partners to launch that way? My understanding was that we expect a partner to launch a database somewhere(RDS), and they would configure the app to connect to that.
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.
The VM includes a database preloaded with all value sets, concepts, and foreign key mappings extracted from a fresh eRSD pull and processed through our creation functions. We anticipate that partners will eventually connect their own database.
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 think it's worth discussing whether the database is fully necessary for this VM. We're trying to keep the appliance as lightweight as possible, and it has a tiny disk size of only 8 GB. If we expect a partner to connect their own DB, we run the risk of our bundled postgres instance becoming a maintenance and security issue by just hanging out. There's a chance it will be reintroduced by new compose files on a version update...would a partner want that if they've already cut it out of their build?
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 think for now, we should bundle it in for partners, but it's certainly possible they will want to use their own.
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.
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'm not sure we can determine the final size of this database at the moment. Regarding the ERSD and UMLS codes, they are updated once or twice a year. @nickclyde I suggest treating this as a research or brainstorming topic, but for now, we could leave it as is. Here's the ticket for it.
provisioner "shell" { | ||
only = ["qemu.iso"] | ||
scripts = [ | ||
"scripts/provision.sh" |
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.
When I ran the build it ran the provision script setup for the ecr-viewer.
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.
Good catch,thanks
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.
you might need to change line 90 "scripts/provision.sh" in your qc-ubuntu.pkr.hcl file to reference your new changes "scripts/qc-provision.sh"
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!!!
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.
This is a great start, but I would like to make sure we don't introduce duplicate code. I think this PR should integrate the variable-based changes that Alis demonstrated for the team in their most recent PR. If this one needs to wait for a bit, let's do that. Getting it done right is more important than speed.
@@ -0,0 +1,49 @@ | |||
services: | |||
# PostgreSQL DB for custom query and value set storage | |||
db: |
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 think it's worth discussing whether the database is fully necessary for this VM. We're trying to keep the appliance as lightweight as possible, and it has a tiny disk size of only 8 GB. If we expect a partner to connect their own DB, we run the risk of our bundled postgres instance becoming a maintenance and security issue by just hanging out. There's a chance it will be reintroduced by new compose files on a version update...would a partner want that if they've already cut it out of their build?
|
||
# Next.js app | ||
query-connector: | ||
image: ghcr.io/cdcgov/dibbs-query-connector/query-connector:latest |
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.
Rather than using latest
, we will need to pin this to a specific version. This prevents the stack from auto-updating, and allows us to build specific versions.
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.
Currently we only tag the packages with latest and main tags so I added a ticket to update the workflow to add incremental versions.
@@ -0,0 +1,2 @@ | |||
NODE_ENV=production | |||
DATABASE_URL=postgres://postgres:pw@db:5432/qc_db |
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.
We should revisit the DATABASE_URL
value if we expect customers to attach their own DB.
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.
Currently its an option for customers to attach their own DB but its not a requirement.
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.
Thanks Shanice!!
@@ -0,0 +1,49 @@ | |||
services: | |||
# PostgreSQL DB for custom query and value set storage | |||
db: |
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 think for now, we should bundle it in for partners, but it's certainly possible they will want to use their own.
9ef386a
to
8109c6a
Compare
This PR includes:
A
docker-compose
file to deploy the following containers: database, application, and Portainer.The
vs_dump.sql
file, which initializes all value sets, concepts, and foreign key mappings extracted from a fresh eRSD pull and processed through our creation functions.Environment variable files for both the application and database containers.
A provisioning script (
qc-provision.sh
) specific to query connector.A Packer file tailored for query connector.
In the future, the separate provisioning and Packer files can be consolidated into a single file, leveraging GitHub workflows to determine which application to build the virtual image for.