Refactor #48
Refactor #48
Conversation
| image: docker.io/postgres:latest | ||
| environment: | ||
| POSTGRES_DB: ${DB_NAME} | ||
| POSTGRES_USER: ${DB_USER} | ||
| POSTGRES_PASSWORD: ${DB_PASSWORD} | ||
| start_period: 3s | ||
| healthcheck: |
There was a problem hiding this comment.
Awesome that you got this working! Was it just a syntax thing?
There was a problem hiding this comment.
Thanks :) I actually ended up switching to docker-compose 😅
There was a problem hiding this comment.
Ugh, I hope this wasn't all caused by trusting ChatGPT that damn old timer!
There was a problem hiding this comment.
Nah I was trying to get act working which required docker, so I installed docker, and then when I came back to this I was like "wait a second why am I suffering" and tried it and it Just Worked. Like it or not, Docker's tooling is way more mature than podman's >:[
meshdb/db/setup.py
Outdated
|
|
||
| from sqlalchemy import create_engine | ||
| from ..db.database import create_db_engine | ||
|
|
||
|
|
||
| def setup_db(): |
There was a problem hiding this comment.
Should we maybe call this initialize_db or something to signify it normally only needs to happen once?
meshdb/views.py
Outdated
There was a problem hiding this comment.
Maybe we should rename this "routes.py" since we are not serving any interface views?
| user_datastore = SQLAlchemyUserDatastore(db, authmodels.User, authmodels.Role) | ||
| app.security = Security(app, user_datastore) | ||
|
|
||
| # test code to create user |
There was a problem hiding this comment.
Not caught up on what this is doing, but should we put the testing code in a different file? In normal operation it seems like this step will not be necessary?
There was a problem hiding this comment.
Good Question. We could probably break that out into its own test. I think Quincy just left that in there to make it easier to ensure the thing was working properly, for now.
meshdb/__init__.py
Outdated
| app.config["WTF_CSRF_ENABLED"] = False | ||
|
|
||
| # Configure Database | ||
| from meshdb.db.setup import setup_db |
There was a problem hiding this comment.
Was this related to the circular import? Should we move it to the top?
There was a problem hiding this comment.
I dunno. We could move it to the top and I doubt anything would change, but that feels clearer to me for whatever reason. If we do, however, then maybe we should also move the security imports?
|
|
||
| db.init_app(app) | ||
|
|
||
| import meshdb.auth.authmodels as authmodels |
There was a problem hiding this comment.
Should this be moved to the top as well?
|
This is good, but I don't think that our database structure is quite right yet. It seems odd that we have to create multiple database engines in different places, and I'm also not super solid on what This answer intrigues me: Generally this concern is driven by my instinct that there should be one "database" object or whatever that gets passed around or referenced or whatever, and the API should be talking to that object. That, however, raises another issue: What pattern are we going for? Is that Object Oriented™? Is there a better pattern? Am I totally off-base here? Have we considered using something like this: https://github.com/realpython/flask-boilerplate? |
|
@WillNilges It looks like db.init_app() is not currently defined. |
I'm pretty sure it's some kind of sqlalchemy thing. Remember Andrew's comments about the overloaded names? Also, maybe it's like, fine that the API "owns" the db engine, just seems a little odd to me. |
|
@WillNilges. Those are interesting examples. It seems like it's more common to have all this init.py code in app.py. |
|
@WillNilges The second example does a weird thing and passes the DB config variables in through app. I don't like that for us since I think we want to separate the DB stuff from app for testing etc. So no change for us on that front. |
|
@andybaumgar the DB settings defined for the app is for auth, and the other one is for member data. From what I can tell, unless we want to keep the auth tables in the same DB (I have no idea if this is best practice or not, I'll defer to you guys) we need to define two different DBs. Open to different ways of doing it, just wanted to give background. |
|
@quoncc From some googling it looks like it's normal to use the same DB for everything. |
|
@quoncc I think we can split up these two lines: The user_datastore could go in the database initialization code, and the security could stay along with the other app code. That way all database setup is separate from app setup. |
|
Alright, if keeping auth data in the same DB is a normal practice, I have an idea. We can get rid of the non flask-sqlalchemy database engine, and move all of the models into the db engine that auth is using. This should simplify everything greatly and I probably should've done it from the get-go. Only thing is we need to make sure we're able to call the DB before we initialize the app. |
|
Just did some reading, we can create models before initializing the app, but db.session can't be called without an active application context. Maybe this limitation is OK, but for unit testing the DB we'd need the app to be active as well. I'll defer to you guys. |
|
@quoncc Thanks for researching that. I think that's a fine limitation. Creating a user seems like an "app" thing meaning that it requires logic beyond just database reads and writes. |
|
I'm a little worried about the number of different packages that are required in the Flask ecosystem. I wonder if we can find an example of an app that implements everything we are doing. It seems like our use case is pretty standard. |
|
I mean, this is a pretty standard stack, I feel like we're in a good middle ground with customization vs simplicity. I don't really feel the need to find a more drilled down library. |
|
Anyways, I'm approving this PR. Once this gets merged I can work towards collapsing the DBs to one engine. Hopefully when that's done we can start actually writing some endpoints |
|
@quoncc I agree it could be good to break down this large PR into a smaller one, but I think we should align on some bigger picture Flask ideas before we refactor too much. It seems like the "app factory" pattern is important for a number of libraries. For example it seems like the migrations system relies on that. I think we should try to understand all the best practices before we write too much code. Does the current PR run? I think we still have the |
|
Yeah, it totally runs. The overload is still there, but there's gotta be a way to get rid of that |
|
I am tempted to copy pasta one of these standard boiler plate repos because why reinvent the wheel |
|
Merging this re: the convo we had at the gym |
Thanks Andrew
I got the thing running... It was trying to infinitely start up workers when I used
meshdb:wsgi, but it is my understanding thatapp.run()is not recommended for production, actually.So, referring to this: https://flask.palletsprojects.com/en/2.3.x/deploying/gunicorn/, I changed the ENTRYPOINT of the Dockerfile. I did have to move the database initialization into
create_app, but it is my understanding that it's fine, since it's within thecreate_app()function? Either way, please scrutinize this because I demonstrably have no idea what I am doing.Oh, I also got the docker compose working again.