-
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
Use Flask custom CLI commands for custom scripts #138
Conversation
This reverts commit 941cb06.
@chouinar ok, I made the changes we discussed and created follow-on tickets for:
|
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!
@@ -33,6 +33,19 @@ else | |||
PY_RUN_CMD := docker-compose run $(DOCKER_EXEC_ARGS) --rm $(APP_NAME) poetry run | |||
endif | |||
|
|||
FLASK_CMD := $(PY_RUN_CMD) flask --env-file local.env |
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.
Something in the setup-local command?
# First make sure the table is empty | ||
db_session.query(User).delete() |
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.
Maybe call out that they're from other tests?
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.
done!
In PR #138 we renamed the route package to api, which makes it api.api. This PR renames the top level package to src to avoid naming issues and to represent the idea that the application contains other things like CLI commands / background jobs. We considered naming it app, but that would conflict with the app folder at the repo root, and it would also create ambiguity with the flask app variable.
Ticket
Resolves #90
Changes
Other changes
Context for reviewers
This PR consolidates the way we run the API server and the way we run background scripts to both use Flask's create_app factory. This removes the need to have two separate main() entrypoints, two separate ways to initialize the database, logger, etc. To do this, we leverage Flask's built-in method of creating custom CLI commands which itself is based on the click library, a popular library for building Python command line applications that's more powerful than
argparse
. This allows us to remove the script_utils file.This PR also renames the
api.route
package toapi.api
since we want to add "commands" to the user blueprint, so "route" is no longer an appropriate name that applies to everything in the blueprint. We could also consider other names likeapi.components
orapi.blueprints
, and/or we could consider renaming the top level source code folder to something likesrc
rather thanapi
.Finally, we made test_create_user_csv test things at using the test_cli_runner to be closer to the way we test routes using the test_client. This approach has more end-to-end test coverage.
Other tangential changes include making REST resources use plural names to follow REST resource naming conventions.
Testing
Running `make cmd args='--help' prints out
Running
make cmd-user-create-csv args='--help'
prints outActually running it produces a file like
(these were the users i had in my db)