From 4d014640821240e2621e69720f7c8e638e902dd5 Mon Sep 17 00:00:00 2001 From: jonavellecuerdo Date: Wed, 18 Sep 2024 10:43:47 -0400 Subject: [PATCH] Update Config module to current conventions 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 --- ccslips/alma.py | 21 ++++++++++++------- ccslips/cli.py | 50 +++++++++++++++++++++++--------------------- ccslips/config.py | 45 +++++++++++++++++++++++++-------------- tests/conftest.py | 10 +++++---- tests/test_alma.py | 2 +- tests/test_cli.py | 5 ++--- tests/test_config.py | 50 +++++++++++++++++++++----------------------- 7 files changed, 101 insertions(+), 82 deletions(-) diff --git a/ccslips/alma.py b/ccslips/alma.py index baea9c4..b35094f 100644 --- a/ccslips/alma.py +++ b/ccslips/alma.py @@ -5,7 +5,7 @@ import requests -from ccslips.config import load_alma_config +from ccslips.config import Config logger = logging.getLogger(__name__) @@ -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, diff --git a/ccslips/cli.py b/ccslips/cli.py index 0bcd23e..5537882 100644 --- a/ccslips/cli.py +++ b/ccslips/cli.py @@ -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( @@ -36,15 +37,14 @@ "--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( @@ -52,32 +52,36 @@ def main( 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", } ], ) @@ -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)}" ) diff --git a/ccslips/config.py b/ccslips/config.py index 963b679..afe51a6 100644 --- a/ccslips/config.py +++ b/ccslips/config.py @@ -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())}" @@ -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"), - } diff --git a/tests/conftest.py b/tests/conftest.py index ab9eb38..eed2900 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,7 @@ from moto import mock_aws from ccslips.alma import AlmaClient +from ccslips.config import Config # Env fixtures @@ -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 diff --git a/tests/test_alma.py b/tests/test_alma.py index 246cfaa..e1612e9 100644 --- a/tests/test_alma.py +++ b/tests/test_alma.py @@ -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 == { diff --git a/tests/test_cli.py b/tests/test_cli.py index a9d781d..2787f1d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -30,8 +30,7 @@ def test_cli_all_options_passed(caplog, runner): "recipient2@example.com", "--date", "2023-01-02", - "--log-level", - "debug", + "--verbose", ], ) assert result.exit_code == 0 @@ -39,7 +38,7 @@ def test_cli_all_options_passed(caplog, runner): assert ( "Command called with options: {'source_email': 'from@example.com', " "'recipient_email': ('recipient1@example.com', 'recipient2@example.com'), " - "'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 " diff --git a/tests/test_config.py b/tests/test_config.py index 5139f76..af51f0b 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -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" @@ -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()