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

Use Flask custom CLI commands for custom scripts #138

Merged
merged 52 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
c422aa6
Rename route package to api
lorenyu Feb 9, 2023
45b108a
Make REST resource name plural
lorenyu Feb 9, 2023
54d0b33
Remove unused route_utils module
lorenyu Feb 9, 2023
f2b7cd5
Use consistent import styles
lorenyu Feb 9, 2023
56d2afc
Remove extraneous print statement
lorenyu Feb 9, 2023
6fefac1
Remove extra whitespace
lorenyu Feb 9, 2023
c9630b6
Reorganize user blueprint
lorenyu Feb 9, 2023
6d1d7df
Update apiflask
lorenyu Feb 9, 2023
bb64fa3
Add create-csv command
lorenyu Feb 9, 2023
a373ccb
Update openapi.yml
lorenyu Feb 9, 2023
941cb06
Move local.env to .env.template
lorenyu Feb 9, 2023
42b5d49
Remove unused poetry command
lorenyu Feb 9, 2023
cc9654b
Add Makefile command for Flask CLI
lorenyu Feb 9, 2023
1e365e1
Add help messages
lorenyu Feb 9, 2023
95be2d7
Add make setup-local
lorenyu Feb 9, 2023
6d65a04
Revert "Move local.env to .env.template"
lorenyu Feb 9, 2023
75ca748
Reorganize Makefile commands
lorenyu Feb 9, 2023
716128f
Add comment
lorenyu Feb 9, 2023
78da5d2
Whitespace
lorenyu Feb 9, 2023
dfe2dd3
Add cli_runner fixture
lorenyu Feb 9, 2023
11096e8
Move isolated db stuff to create user csv
lorenyu Feb 11, 2023
6e177cd
Install pytest lazy fixture
lorenyu Feb 11, 2023
6aecce5
Simplify assertion
lorenyu Feb 11, 2023
7053d91
Remove unused utils
lorenyu Feb 11, 2023
662ac67
Lint
lorenyu Feb 11, 2023
6daef3e
Add test for default filename
lorenyu Feb 11, 2023
020c4cf
Remove unused main function
lorenyu Feb 11, 2023
f1a29eb
Remove script_util
lorenyu Feb 11, 2023
1459046
Add comment to cli command
lorenyu Feb 13, 2023
badb082
Rename cli to cmd
lorenyu Feb 13, 2023
4cc0af4
Add help messages to cmd params
lorenyu Feb 13, 2023
69f3848
Remove unused env var
lorenyu Feb 15, 2023
0dc857f
Rename source directory from api to src
lorenyu Feb 16, 2023
45b9e3f
Remove rollback logic from test
lorenyu Feb 16, 2023
e55bd99
Revert "Remove rollback logic from test"
lorenyu Feb 21, 2023
5622433
Revert "Rename source directory from api to src"
lorenyu Feb 21, 2023
aeff01a
Remove rollback logic from test
lorenyu Feb 21, 2023
55c2339
Add migration to cascade on delete
lorenyu Feb 21, 2023
23a1a75
Truncate user table in create csv test
lorenyu Feb 21, 2023
ddc6313
Move empty_schema fixture to test_migrations
lorenyu Feb 21, 2023
b83213b
Rename factories_db_session fixture
lorenyu Feb 21, 2023
6fc18d1
Lint
lorenyu Feb 21, 2023
7e3cfc8
Rename preopopulate fixture
lorenyu Feb 21, 2023
1a115af
Move empty_schema fixture to correct file
lorenyu Feb 21, 2023
00be3be
Combine truncate and prepopulate fixture
lorenyu Feb 21, 2023
2c01c76
Add comments
lorenyu Feb 21, 2023
83ccce0
Rename db var to avoid name conflict
lorenyu Feb 21, 2023
cd39d03
Merge remote-tracking branch 'origin/main' into lorenyu/scripts
lorenyu Feb 21, 2023
81868e9
isort
lorenyu Feb 21, 2023
873939c
Add known first party
lorenyu Feb 21, 2023
1e2788b
Revert "Add known first party"
lorenyu Feb 21, 2023
0a837a8
Add comment
lorenyu Feb 21, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions app/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we run commands non-locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ECS could run it directly with via poetry run flask --app api.app <cmd>.

Open to suggestions though. The challenge I ran into with the Makefile is that while docker-compose.yml has env_file: local.env which automatically loads the env file, running it on local host machine with PY_RUN_APPROACH=local won't automatically load the env vars.

I would love to move away from the load_local_env_vars function in our code. I think it's not very elegant to have if environment == <x> checks in the application code itself. I was actually a bit confused to see that load_local_env_vars was being called in production, and only realized upon digging into the function that it is a no-op based on the ENVIRONMENT env var.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with moving the env var loading somewhere else.

I think the issue I had here was that we can't use FLASK_CMD anywhere non-locally as the env-file is in the command itself. Would it make more sense to do some sort of if statement in the makefile to add that bit to the command if you're running locally? That way when running non-locally it doesn't get used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that makes sense. How do I detect if Makefile is running locally? I'm caught up in a bit of chicken-egg. The ENVIRONMENT=local setting is defined in local.env. Should we make that specific environment variable something that is defined outside of local.env, either with direnv or something?

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah maybe. Anyways I created a ticket to revisit env vars more thoroughly #136


##################################################
# Local Development Environment Setup
##################################################

setup-local:
# Configure poetry to use virtualenvs in the project directory
poetry config virtualenvs.in-project true

# Install dependencies
poetry install --no-root

##################################################
# API Build & Run
##################################################
Expand All @@ -46,7 +59,6 @@ start:
run-logs: start
docker-compose logs --follow --no-color $(APP_NAME)


init: build init-db

clean-volumes: ## Remove project docker volumes (which includes the DB state)
Expand All @@ -57,13 +69,6 @@ stop:

check: format-check lint test

# Set init-db as pre-requisite since there seems to be a race condition
# where the DB can't yet receive connections if it's starting from a
# clean state (e.g. after make stop, make clean-volumes, make openapi-spec)
openapi-spec: init-db ## Generate OpenAPI spec
$(PY_RUN_CMD) flask spec --format yaml --output ./openapi.yml


##################################################
# DB & migrations
##################################################
Expand Down Expand Up @@ -168,11 +173,21 @@ lint-security: # https://bandit.readthedocs.io/en/latest/index.html


##################################################
# Scripts
# CLI Commands
##################################################

create-user-csv:
$(PY_RUN_CMD) create-user-csv
cmd: ## Run Flask app CLI command (Run `make cli args="--help"` to see list of CLI commands)
$(FLASK_CMD) $(args)

cmd-user-create-csv: ## Create a CSV of the useres in the database (Run `make cli-user-create-csv args="--help"` to see the command's options)
$(FLASK_CMD) user create-csv $(args)

# Set init-db as pre-requisite since there seems to be a race condition
# where the DB can't yet receive connections if it's starting from a
# clean state (e.g. after make stop, make clean-volumes, make openapi-spec)
openapi-spec: init-db ## Generate OpenAPI spec
$(FLASK_CMD) spec --format yaml --output ./openapi.yml
Comment on lines +185 to +189
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to deal with that race condition on NJ as the Oracle DB takes 60+ seconds to startup: https://github.com/navapbc/archive-nj-dol-ui-claimant-api/blob/main/wait-for-local-oracle.sh - it just checks if a particular log message appears every 5 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting! I wonder if there's a better way to do that here as well.



##################################################
# Miscellaneous Utilities
Expand All @@ -184,10 +199,3 @@ login: start ## Start shell in running container
# Pauses for 5 seconds
sleep-5:
sleep 5

setup-local:
# Configure poetry to use virtualenvs in the project directory
poetry config virtualenvs.in-project true

# Install dependencies
poetry install --no-root
File renamed without changes.
4 changes: 2 additions & 2 deletions app/api/route/healthcheck.py → app/api/api/healthcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
from werkzeug.exceptions import ServiceUnavailable

import api.adapters.db.flask_db as flask_db
from api.route import response
from api.route.schemas import request_schema
from api.api import response
lorenyu marked this conversation as resolved.
Show resolved Hide resolved
from api.api.schemas import request_schema

logger = logging.getLogger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion app/api/route/response.py → app/api/api/response.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import dataclasses
from typing import Optional

from api.api.schemas import response_schema
from api.db.models.base import Base
from api.route.schemas import response_schema


@dataclasses.dataclass
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from apiflask import fields

from api.route.schemas import request_schema
from api.api.schemas import request_schema


class ValidationErrorSchema(request_schema.OrderedSchema):
Expand Down
10 changes: 10 additions & 0 deletions app/api/api/users/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from api.api.users.user_blueprint import user_blueprint

# import user_commands module to register the CLI commands on the user_blueprint
import api.api.users.user_commands # noqa: F401 E402 isort:skip

# import user_commands module to register the API routes on the user_blueprint
import api.api.users.user_routes # noqa: F401 E402 isort:skip


__all__ = ["user_blueprint"]
3 changes: 3 additions & 0 deletions app/api/api/users/user_blueprint.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from apiflask import APIBlueprint

user_blueprint = APIBlueprint("user", __name__, tag="User", cli_group="user")
34 changes: 34 additions & 0 deletions app/api/api/users/user_commands.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import logging
import os.path as path
from typing import Optional

import click

import api.adapters.db as db
import api.adapters.db.flask_db as flask_db
import api.services.users as user_service
from api.api.users.user_blueprint import user_blueprint
from api.util.datetime_util import utcnow

logger = logging.getLogger(__name__)

user_blueprint.cli.help = "User commands"


@user_blueprint.cli.command("create-csv", help="Create a CSV of all users and their roles")
@flask_db.with_db_session
@click.option(
"--dir",
default=".",
help="Directory to save output file in. Can be an S3 path (e.g. 's3://bucketname/folder/') Defaults to current directory ('.').",
)
@click.option(
"--filename",
default=None,
help="Filename to save output file as. Defaults to '[timestamp]-user-roles.csv.",
)
def create_csv(db_session: db.Session, dir: str, filename: Optional[str]) -> None:
if filename is None:
filename = utcnow().strftime("%Y-%m-%d-%H-%M-%S") + "-user-roles.csv"
filepath = path.join(dir, filename)
user_service.create_user_csv(db_session, filepath)
44 changes: 12 additions & 32 deletions app/api/route/user_route.py → app/api/api/users/user_routes.py
Original file line number Diff line number Diff line change
@@ -1,44 +1,34 @@
import logging
from typing import Any

from apiflask import APIBlueprint

import api.adapters.db as db
import api.adapters.db.flask_db as flask_db
import api.api.response as response
import api.api.users.user_schemas as user_schemas
import api.services.users as user_service
import api.services.users as users
from api.api.users.user_blueprint import user_blueprint
from api.auth.api_key_auth import api_key_auth
from api.db.models.user_models import User
from api.route import response
from api.route.schemas import user_schemas
from api.services import users

logger = logging.getLogger(__name__)


user_blueprint = APIBlueprint("user", __name__, tag="User")


@user_blueprint.post("/v1/user")
@user_blueprint.post("/v1/users")
@user_blueprint.input(user_schemas.UserSchema)
@user_blueprint.output(user_schemas.UserSchema, status_code=201)
@user_blueprint.auth_required(api_key_auth)
@flask_db.with_db_session
def user_post(db_session: db.Session, user_params: users.CreateUserParams) -> dict:
"""
POST /v1/user
POST /v1/users
"""
user = user_service.create_user(db_session, user_params)

logger.info(
"Successfully inserted user",
extra=get_user_log_params(user),
)
print("Successfully inserted user", get_user_log_params(user))

logger.info("Successfully inserted user", extra=get_user_log_params(user))
return response.ApiResponse(message="Success", data=user).asdict()


@user_blueprint.patch("/v1/user/<uuid:user_id>")
@user_blueprint.patch("/v1/users/<uuid:user_id>")
# Allow partial updates. partial=true means requests that are missing
# required fields will not be rejected.
# https://marshmallow.readthedocs.io/en/stable/quickstart.html#partial-loading
Expand All @@ -50,29 +40,19 @@ def user_patch(
db_session: db.Session, user_id: str, patch_user_params: users.PatchUserParams
) -> dict:
user = user_service.patch_user(db_session, user_id, patch_user_params)

logger.info(
"Successfully patched user",
extra=get_user_log_params(user),
)

logger.info("Successfully patched user", extra=get_user_log_params(user))
return response.ApiResponse(message="Success", data=user).asdict()


@user_blueprint.get("/v1/user/<uuid:user_id>")
@user_blueprint.get("/v1/users/<uuid:user_id>")
@user_blueprint.output(user_schemas.UserSchema)
@user_blueprint.auth_required(api_key_auth)
@flask_db.with_db_session
def user_get(db_session: db.Session, user_id: str) -> dict:
user = user_service.get_user(db_session, user_id)

logger.info(
"Successfully fetched user",
extra=get_user_log_params(user),
)

logger.info("Successfully fetched user", extra=get_user_log_params(user))
return response.ApiResponse(message="Success", data=user).asdict()


def get_user_log_params(user: User) -> dict[str, Any]:
return {"user_id": user.id}
return {"user.id": user.id}
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
from apiflask import fields
from marshmallow import fields as marshmallow_fields

from api.api.schemas import request_schema
from api.db.models import user_models
from api.route.schemas import request_schema

##############
# Role Models
##############


class RoleSchema(request_schema.OrderedSchema):
Expand All @@ -20,11 +16,6 @@ class RoleSchema(request_schema.OrderedSchema):
# will always be a nested fields of the API user


##############
# User Models
##############


class UserSchema(request_schema.OrderedSchema):
id = fields.UUID(dump_only=True)
first_name = fields.String(metadata={"description": "The user's first name"}, required=True)
Expand Down
6 changes: 3 additions & 3 deletions app/api/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@
import api.adapters.db.flask_db as flask_db
import api.logging
import api.logging.flask_logger as flask_logger
from api.api.healthcheck import healthcheck_blueprint
from api.api.schemas import response_schema
from api.api.users import user_blueprint
from api.auth.api_key_auth import User, get_app_security_scheme
from api.route.healthcheck import healthcheck_blueprint
from api.route.schemas import response_schema
from api.route.user_route import user_blueprint

logger = logging.getLogger(__name__)

Expand Down
30 changes: 30 additions & 0 deletions app/api/db/migrations/versions/2023_02_21_cascade_on_delete.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
"""cascade on delete

Revision ID: 9fe657340f70
Revises: 4ff1160282d1
Create Date: 2023-02-21 18:16:56.052679

"""
from alembic import op

# revision identifiers, used by Alembic.
revision = "9fe657340f70"
down_revision = "4ff1160282d1"
branch_labels = None
depends_on = None


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_constraint("role_user_id_user_fkey", "role", type_="foreignkey")
op.create_foreign_key(
op.f("role_user_id_user_fkey"), "role", "user", ["user_id"], ["id"], ondelete="CASCADE"
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_constraint(op.f("role_user_id_user_fkey"), "role", type_="foreignkey")
op.create_foreign_key("role_user_id_user_fkey", "role", "user", ["user_id"], ["id"])
# ### end Alembic commands ###
6 changes: 4 additions & 2 deletions app/api/db/models/user_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ class User(Base, IdMixin, TimestampMixin):
date_of_birth: date = Column(Date, nullable=False)
is_active: bool = Column(Boolean, nullable=False)

roles: list["Role"] = relationship("Role", back_populates="user", order_by="Role.type")
roles: list["Role"] = relationship(
"Role", back_populates="user", cascade="all, delete", order_by="Role.type"
)


class Role(Base, TimestampMixin):
__tablename__ = "role"
user_id: Mapped[UUID] = Column(
postgresql.UUID(as_uuid=True), ForeignKey("user.id"), primary_key=True
postgresql.UUID(as_uuid=True), ForeignKey("user.id", ondelete="CASCADE"), primary_key=True
)

# Set native_enum=False to use store enum values as VARCHAR/TEXT
Expand Down
21 changes: 0 additions & 21 deletions app/api/route/route_utils.py

This file was deleted.

Empty file removed app/api/scripts/__init__.py
Empty file.
Empty file removed app/api/scripts/util/__init__.py
Empty file.
41 changes: 0 additions & 41 deletions app/api/scripts/util/script_util.py

This file was deleted.

Loading