Skip to content
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

Kirkman central db #6

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Kirkman central db #6

wants to merge 9 commits into from

Conversation

hhheath
Copy link
Member

@hhheath hhheath commented Feb 19, 2020

Drew is a maniac.

@Hodapp87
Copy link
Member

A thought: If we're trying to restrict api/app to API endpoints, and there may be uses of the DB that are independent of this (e.g. a business logic layer, or another daemon that looks for queued tasks), might it make sense to put it one level higher in the hierarchy outside of app?

@hhheath
Copy link
Member Author

hhheath commented Feb 19, 2020

@Hodapp87 is that something that currently happens? I have no issue with moving the DB connection a to a higher level, I just want to better understand what we currently have?

Could we have to connections, one for business logic or queued tasks, and one for api endpoints? I'm totally green to this, or even if that is a real question, only exploring options.

@Hodapp87
Copy link
Member

It happens in the Perl in (I think) things like queue_runner.pl, and the logic for that may need to hang around. Flask is very request-oriented and likes to occupy the entire event loop, so it's plenty conceivable that for any sort of background task, we'd need a separate entry point distinct from api/app. Everything else in the module looks fine to me - except that it might make more sense to relocate so that it remains centralized.

@hhheath
Copy link
Member Author

hhheath commented Feb 20, 2020

I just committed (as seen above) the code that I wrote last night.

I pulled the DB connection out of /app/ and made it available.

It pulls it's on config (inside of /db/config.py) but uses creds in app/creds.py

Not sure if we want to duplicate credentials, but that's how is now. Happy to move it anywhere we deem necessary.

@hhheath
Copy link
Member Author

hhheath commented Feb 26, 2020

@Hodapp87 Thoughts now that I've moved the DB connection outside of /app/? I think we're ready to merge it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants