Skip to content

Refactor: Force all configurations to be fetched from settings.py #179

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.

Sign up for GitHub

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

Open
KelvinLinBU wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor: Force all configurations to be fetched from settings.py #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 2 additions & 8 deletions process_report/invoices/pi_specific_invoice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import os
import sys
from dataclasses import dataclass
import subprocess
import tempfile
Expand All @@ -10,10 +9,10 @@

import process_report.invoices.invoice as invoice
import process_report.util as util
from process_report.settings import invoice_settings


TEMPLATE_DIR_PATH = "process_report/templates"
CHROME_BIN_PATH = os.environ.get("CHROME_BIN_PATH", "/usr/bin/chromium")


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -134,17 +133,12 @@ def _create_html_invoice(temp_fd):
temp_fd.flush()

def _create_pdf_invoice(temp_fd_name):
if not os.path.exists(CHROME_BIN_PATH):
sys.exit(
f"Chrome binary does not exist at {CHROME_BIN_PATH}. Make sure the env var CHROME_BIN_PATH is set correctly and that Google Chrome is installed"
)

invoice_pdf_path = (
f"{self.name}/{pi_instituition}_{pi}_{self.invoice_month}.pdf"
)
subprocess.run(
[
CHROME_BIN_PATH,
invoice_settings.chrome_bin_path,
"--headless",
"--no-sandbox",
f"--print-to-pdf={invoice_pdf_path}",
Expand Down
14 changes: 0 additions & 14 deletions process_report/process_report.py
View file
Open in desktop

@QuanMPhm QuanMPhm Jun 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should add CHROME_BIN_PATH in REQUIRED_ENV_VARS

@larsks larsks Apr 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's no reason to perform validation in two different places. If we're using pydantic_settings to manage environment variables, then rather than using required_env_files here, just mark chrome_bin_path as required in process_report.settings.Settings. Currently it is optional:

    chrome_bin_path: str | None = None

To make it required:

    chrome_bin_path: str

This will cause pydantic to throw a validation error when instantiating Settings:

pydantic_core._pydantic_core.ValidationError: 1 validation error for Settings
chrome_bin_path
  Field required [type=missing, input_value={}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.10/v/missing

Similarly, you can remove the check for KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET here by adding a validator to Settings:

from pydantic import model_validator
from pydantic_settings import BaseSettings

class Settings(BaseSettings):
    ...

    
    @model_validator(mode="after")
    def check_keycloak_auth(self):
        if not self.coldfront_api_filepath and not (
            self.keycloak_client_id and self.keycloak_client_secret
        ):
            raise ValueError(
                "You must either set coldfront_api_filepath or provide keycloak credentials in "
                "KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET"
            )

You probably want to catch the exception from pydantic to produce a more friendly error message that does not include a full Python traceback.

@QuanMPhm QuanMPhm Apr 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the very detailed feedback. I don't know why I didn't thought of getting rid of that hacky function in the first place. Your feedback makes it look obvious

Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import sys
import logging
import os

import pandas
import pyarrow
Expand Down Expand Up @@ -43,19 +41,7 @@
logging.basicConfig(level=logging.INFO)


def validate_required_env_vars(required_env_vars):
for required_env_var in required_env_vars:
if required_env_var not in os.environ:
sys.exit(f"Required environment variable {required_env_var} is not set")


def main():
"""Remove non-billable PIs and projects"""
required_env_vars = []
if not invoice_settings.coldfront_api_filepath:
required_env_vars.extend(["KEYCLOAK_CLIENT_ID", "KEYCLOAK_CLIENT_SECRET"])
validate_required_env_vars(required_env_vars)

invoice_month = invoice_settings.invoice_month

merged_dataframe = merge_csv(loader.get_csv_invoice_filepath_list())
Expand Down
27 changes: 25 additions & 2 deletions process_report/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from dateutil.relativedelta import relativedelta
from pydantic_settings import BaseSettings
from pydantic import model_validator, ValidationError


class Settings(BaseSettings):
Expand All @@ -18,6 +19,8 @@ class Settings(BaseSettings):
fetch_from_s3: bool = True
upload_to_s3: bool = False

chrome_bin_path: str = "/usr/bin/chromium"

# S3 Files
pi_remote_filepath: str = "PIs/PI.csv"
alias_remote_filepath: str = "PIs/alias.csv"
Expand All @@ -36,5 +39,25 @@ class Settings(BaseSettings):
bu_subsidy_amount: Decimal | None = None
lenovo_charge_info: dict[str, Decimal] | None = None


invoice_settings = Settings()
@model_validator(mode="after")
def check_keycloak_auth(self):
if not self.coldfront_api_filepath and not (
self.keycloak_client_id and self.keycloak_client_secret
):
raise ValueError(
"You must either set coldfront_api_filepath or provide keycloak credentials in "
"KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET"
)

return self


try:
invoice_settings = Settings()
except ValidationError as e:
for error in e.errors():
if error["type"] == "missing":
print(f"Missing required environment variable: {error['loc'][0]}")
else:
print(f"Error in environment variable {error['loc']}: {error['msg']}")
raise
2 changes: 2 additions & 0 deletions process_report/tests/integration/pytest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[pytest_env]
COLDFRONT_API_FILEPATH = "/foo/coldfront_api.json"
3 changes: 1 addition & 2 deletions process_report/tests/unit/invoices/test_pi_specific_invoice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import pandas

from process_report.tests import util as test_utils
from process_report.invoices.pi_specific_invoice import CHROME_BIN_PATH


class TestPISpecificInvoice(TestCase):
Expand Down Expand Up @@ -155,7 +154,7 @@ def test_export_pi(self, mock_subprocess_run, mock_path_exists, mock_filter_cols
for i, pi_pdf_path in enumerate([pi_pdf_1, pi_pdf_2]):
chrome_arglist, _ = mock_subprocess_run.call_args_list[i]
answer_arglist = [
CHROME_BIN_PATH,
"/usr/bin/chromium", # Default config value in settings.py
"--headless",
"--no-sandbox",
f"--print-to-pdf={pi_pdf_path}",
Expand Down
2 changes: 2 additions & 0 deletions process_report/tests/unit/pytest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[pytest_env]
COLDFRONT_API_FILEPATH = "/foo/coldfront_api.json"
19 changes: 1 addition & 18 deletions process_report/tests/unit/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from unittest import TestCase, mock
from unittest import TestCase
import tempfile
import pandas
import os
Expand Down Expand Up @@ -138,20 +138,3 @@ def test_get_nonbillable_projects_loads_yaml_into_expected_dataframe(self):
)

assert nonbillable_projects.equals(expected_projects)


class TestValidateRequiredEnvVars(TestCase):
@mock.patch.dict(
"os.environ", {"KEYCLOAK_CLIENT_ID": "test", "KEYCLOAK_CLIENT_SECRET": "test"}
)
def test_env_vars_valid(self):
process_report.validate_required_env_vars(
["KEYCLOAK_CLIENT_ID", "KEYCLOAK_CLIENT_SECRET"]
)

@mock.patch.dict("os.environ", {"KEYCLOAK_CLIENT_ID": "test"})
def test_env_vars_missing(self):
with pytest.raises(SystemExit):
process_report.validate_required_env_vars(
["KEYCLOAK_CLIENT_ID", "KEYCLOAK_CLIENT_SECRET"]
)
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ Jinja2
validators
python-dateutil
pydantic-settings
pytest-env
Loading