Skip to content

Commit

Permalink
Update Config module to current conventions
Browse files Browse the repository at this point in the history
Why these changes are being introduced:
* Use a central Config class for dynamic access to
environment variables and simplify method for configuring
loggers.

How this addresses that need:
* Create a Config class for dynamically accessing environment variables
* Deprecate load_alma_config and update AlmaClient to use Config class to dynamically
set attributes (base_url, headers, timeout)
* Update configure_logger to use 'verbose' boolean flag
* Update CLI to accept '-v/--verbose' boolean option and remove '-l/--log-level' string option
* Add/update corresponding unit tests

Side effects of this change:
* Remove LOG_LEVEL as an optional environment variable

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/IN-1065
  • Loading branch information
jonavellecuerdo committed Sep 19, 2024
1 parent da3d1d9 commit bb3bd47
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 82 deletions.
21 changes: 13 additions & 8 deletions ccslips/alma.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import requests

from ccslips.config import load_alma_config
from ccslips.config import Config

logger = logging.getLogger(__name__)

Expand All @@ -24,16 +24,21 @@ class AlmaClient:
{"total_record_count": 0} and these methods will return that object.
"""

def __init__(self) -> None:
"""Initialize AlmaClient instance."""
alma_config = load_alma_config()
self.base_url = alma_config["BASE_URL"]
self.headers = {
"Authorization": f"apikey {alma_config['API_KEY']}",
@property
def base_url(self) -> str:
return Config().ALMA_API_URL

@property
def headers(self) -> dict:
return {
"Authorization": f"apikey {Config().ALMA_API_READ_KEY}",
"Accept": "application/json",
"Content-Type": "application/json",
}
self.timeout = float(alma_config["TIMEOUT"])

@property
def timeout(self) -> float:
return float(Config().ALMA_API_TIMEOUT)

def get_paged(
self,
Expand Down
50 changes: 26 additions & 24 deletions ccslips/cli.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import datetime
import logging
import os
from time import perf_counter

import click

from ccslips.config import configure_logger, configure_sentry
from ccslips.config import Config, configure_logger, configure_sentry
from ccslips.email import Email
from ccslips.polines import generate_credit_card_slips_html, process_po_lines

logger = logging.getLogger(__name__)

CONFIG = Config()


@click.command()
@click.option(
Expand All @@ -36,48 +37,51 @@
"--date",
help=(
"Optional date of exports to process, in 'YYYY-MM-DD' format. Defaults to "
"yesterday's date if not provided."
"two (2) days before the date the application is run."
),
)
@click.option(
"-l",
"--log-level",
envvar="LOG_LEVEL",
help="Case-insensitive Python log level to use, e.g. debug or warning. Defaults to "
"INFO if not provided or found in ENV.",
"-v",
"--verbose",
is_flag=True,
help="Pass to set log level to DEBUG. Defaults to INFO.",
)
@click.pass_context
def main(
ctx: click.Context,
source_email: str,
recipient_email: list[str],
date: str | None,
log_level: str | None,
*,
verbose: bool,
) -> None:
start_time = perf_counter()
log_level = log_level or "INFO"
root_logger = logging.getLogger()
logger.info(configure_logger(root_logger, log_level))
logger.info(configure_logger(root_logger, verbose=verbose))
logger.info(configure_sentry())
logger.debug("Command called with options: %s", ctx.params)
CONFIG.check_required_env_vars()

logger.debug("Command called with options: %s", ctx.params)
logger.info("Starting credit card slips process")
date = date or (

# creation date of retrieved PO lines
created_date = date or (
datetime.datetime.now(tz=datetime.UTC) - datetime.timedelta(days=2)
).strftime("%Y-%m-%d")
credit_card_slips_data = process_po_lines(date)

credit_card_slips_data = process_po_lines(created_date)

email_content = generate_credit_card_slips_html(credit_card_slips_data)
email = Email()
env = os.environ["WORKSPACE"]
subject_prefix = f"{env.upper()} " if env != "prod" else ""
subject_prefix = f"{CONFIG.WORKSPACE.upper()} " if CONFIG.WORKSPACE != "prod" else ""
email.populate(
from_address=source_email,
to_addresses=",".join(recipient_email),
subject=f"{subject_prefix}Credit card slips {date}",
subject=f"{subject_prefix}Credit card slips {created_date}",
attachments=[
{
"content": email_content,
"filename": f"{date}_credit_card_slips.htm",
"filename": f"{created_date}_credit_card_slips.htm",
}
],
)
Expand All @@ -86,10 +90,8 @@ def main(

elapsed_time = perf_counter() - start_time
logger.info(
"Credit card slips processing complete for date %s. Email sent to recipient(s) "
"%s with SES message ID '%s'. Total time to complete process: %s",
date,
recipient_email,
response["MessageId"],
str(datetime.timedelta(seconds=elapsed_time)),
f"Credit card slips processing complete for date {created_date}. "
f"Email sent to recipient(s) {recipient_email} "
f"with SES message ID {response["MessageId"]}. "
f"Total time to complete process: {datetime.timedelta(seconds=elapsed_time)}"
)
45 changes: 29 additions & 16 deletions ccslips/config.py
Original file line number Diff line number Diff line change
@@ -1,27 +1,48 @@
import logging
import os
from typing import Any

import sentry_sdk


def configure_logger(logger: logging.Logger, log_level_string: str) -> str:
if log_level_string.upper() not in logging.getLevelNamesMapping():
message = f"'{log_level_string}' is not a valid Python logging level"
raise ValueError(message)
log_level = logging.getLevelName(log_level_string.upper())
if log_level < 20: # noqa: PLR2004
class Config:
REQUIRED_ENV_VARS = ("ALMA_API_URL", "ALMA_API_READ_KEY", "SENTRY_DSN", "WORKSPACE")

OPTIONAL_ENV_VARS = (
"ALMA_API_TIMEOUT",
"SES_RECIPIENT_EMAIL",
"SES_SEND_FROM_EMAIL",
)

def check_required_env_vars(self) -> None:
"""Method to raise exception if required env vars not set."""
missing_vars = [var for var in self.REQUIRED_ENV_VARS if not os.getenv(var)]
if missing_vars:
message = f"Missing required environment variables: {', '.join(missing_vars)}"
raise OSError(message)

def __getattr__(self, name: str) -> Any: # noqa: ANN401
"""Provide dot notation access to configurations and env vars on this class."""
if name in self.REQUIRED_ENV_VARS or name in self.OPTIONAL_ENV_VARS:
return os.getenv(name)
message = f"'{name}' not a valid configuration variable"
raise AttributeError(message)


def configure_logger(logger: logging.Logger, *, verbose: bool) -> str:
if verbose:
logging.basicConfig(
format="%(asctime)s %(levelname)s %(name)s.%(funcName)s() line %(lineno)d: "
"%(message)s"
)
logger.setLevel(log_level)
logger.setLevel(logging.DEBUG)
for handler in logging.root.handlers:
handler.addFilter(logging.Filter("ccslips"))
else:
logging.basicConfig(
format="%(asctime)s %(levelname)s %(name)s.%(funcName)s(): %(message)s"
)
logger.setLevel(log_level)
logger.setLevel(logging.INFO)
return (
f"Logger '{logger.name}' configured with level="
f"{logging.getLevelName(logger.getEffectiveLevel())}"
Expand All @@ -35,11 +56,3 @@ def configure_sentry() -> str:
sentry_sdk.init(sentry_dsn, environment=env)
return f"Sentry DSN found, exceptions will be sent to Sentry with env={env}"
return "No Sentry DSN found, exceptions will not be sent to Sentry"


def load_alma_config() -> dict[str, str]:
return {
"API_KEY": os.environ["ALMA_API_READ_KEY"],
"BASE_URL": os.environ["ALMA_API_URL"],
"TIMEOUT": os.getenv("ALMA_API_TIMEOUT", "30"),
}
10 changes: 6 additions & 4 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from moto import mock_aws

from ccslips.alma import AlmaClient
from ccslips.config import Config


# Env fixtures
Expand All @@ -21,10 +22,11 @@ def _test_environment(monkeypatch):
)
monkeypatch.setenv("SENTRY_DSN", "None")
monkeypatch.setenv("WORKSPACE", "test")
monkeypatch.setenv("AWS_ACCESS_KEY_ID", "testing")
monkeypatch.setenv("AWS_SECRET_ACCESS_KEY", "testing")
monkeypatch.setenv("AWS_SECURITY_TOKEN", "testing")
monkeypatch.setenv("AWS_SESSION_TOKEN", "testing")


@pytest.fixture
def config_instance() -> Config:
return Config()


# CLI fixture
Expand Down
2 changes: 1 addition & 1 deletion tests/test_alma.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from ccslips.alma import AlmaClient


def test_client_initializes_with_expected_values():
def test_client_initializes_with_expected_values(monkeypatch):
client = AlmaClient()
assert client.base_url == "https://example.com"
assert client.headers == {
Expand Down
5 changes: 2 additions & 3 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,15 @@ def test_cli_all_options_passed(caplog, runner):
"[email protected]",
"--date",
"2023-01-02",
"--log-level",
"debug",
"--verbose",
],
)
assert result.exit_code == 0
assert "Logger 'root' configured with level=DEBUG" in caplog.text
assert (
"Command called with options: {'source_email': '[email protected]', "
"'recipient_email': ('[email protected]', '[email protected]'), "
"'date': '2023-01-02', 'log_level': 'debug'}" in caplog.text
"'date': '2023-01-02', 'verbose': True}" in caplog.text
)
assert (
"Credit card slips processing complete for date 2023-01-02. Email sent to "
Expand Down
50 changes: 24 additions & 26 deletions tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,20 @@

import pytest

from ccslips.config import configure_logger, configure_sentry, load_alma_config
from ccslips.config import configure_logger, configure_sentry


def test_configure_logger_with_invalid_level_raises_error():
def test_configure_logger_not_verbose():
logger = logging.getLogger(__name__)
with pytest.raises(ValueError, match="'oops' is not a valid Python logging level"):
configure_logger(logger, log_level_string="oops")


def test_configure_logger_info_level_or_higher():
logger = logging.getLogger(__name__)
result = configure_logger(logger, log_level_string="info")
assert logger.getEffectiveLevel() == 20 # noqa: PLR2004
result = configure_logger(logger, verbose=False)
assert logger.getEffectiveLevel() == logging.INFO
assert result == "Logger 'tests.test_config' configured with level=INFO"


def test_configure_logger_debug_level_or_lower():
def test_configure_logger_verbose():
logger = logging.getLogger(__name__)
result = configure_logger(logger, log_level_string="DEBUG")
assert logger.getEffectiveLevel() == 10 # noqa: PLR2004
result = configure_logger(logger, verbose=True)
assert logger.getEffectiveLevel() == logging.DEBUG
assert result == "Logger 'tests.test_config' configured with level=DEBUG"


Expand All @@ -43,18 +37,22 @@ def test_configure_sentry_env_variable_is_dsn(monkeypatch):
assert result == "Sentry DSN found, exceptions will be sent to Sentry with env=test"


def test_load_alma_config_from_env():
assert load_alma_config() == {
"API_KEY": "just-for-testing",
"BASE_URL": "https://example.com",
"TIMEOUT": "10",
}
def test_config_env_var_access_success(config_instance):
assert config_instance.WORKSPACE == "test"


def test_config_env_var_access_error(config_instance):
with pytest.raises(
AttributeError, match="'DOES_NOT_EXIST' not a valid configuration variable"
):
_ = config_instance.DOES_NOT_EXIST


def test_config_check_required_env_vars_success(config_instance):
_ = config_instance.check_required_env_vars


def test_load_alma_config_from_defaults(monkeypatch):
monkeypatch.delenv("ALMA_API_TIMEOUT", raising=False)
assert load_alma_config() == {
"API_KEY": "just-for-testing",
"BASE_URL": "https://example.com",
"TIMEOUT": "30",
}
def test_config_check_required_env_vars_error(monkeypatch, config_instance):
monkeypatch.delenv("ALMA_API_URL")
with pytest.raises(OSError, match="Missing required environment variables"):
config_instance.check_required_env_vars()

0 comments on commit bb3bd47

Please sign in to comment.