Skip to content

refactor: centralize deployment settings in top-level config.py - #345

Merged
as535364 merged 4 commits into
mainfrom
refactor/centralize-config
Jul 20, 2026
Merged

refactor: centralize deployment settings in top-level config.py#345
as535364 merged 4 commits into
mainfrom
refactor/centralize-config

Conversation

@as535364

@as535364 as535364 commented Jul 20, 2026

Copy link
Copy Markdown
Member

Closes #343.

把散在各模組的環境變數讀取集中到頂層 config.py,用 pydantic-settings 的 Settings 在 import 時一次載入並驗證,之後程式一律讀 config.settings。搬遷的有 MINIO_*(原 mongo/config.py,已刪)、MONGO_HOSTJWT_*TESTINGREDIS_*SUBMISSION_TMP_DIRSMTP_*。原本 setup_smtp() 裡「SMTP_SERVER 有設但 SMTP_NOREPLY 未設就擋啟動」的規則搬進 Settings 的 model_validator,函式本身移除;啟動時那兩條 info log 改由 create_app 依 settings 判斷後照舊輸出。

行為凍結,除了兩個刻意的變更:

  1. SUBMISSION_TMP_DIR 預設值改用 tempfile.mkdtemp()。舊寫法 TemporaryDirectory(...).name 沒有把物件留住,GC 一回收目錄就被刪掉。順手修了 noj-submisisons 這個 typo。
  2. FLASK_DEBUG 改名 DEBUG。外部部署沒有任何地方設這個變數,唯一消費者是 mongo/utils.py 給 MinIO client 的 secure=

另外三件事需要說明。typed settings 會讓無效的值(像 DEBUG=garbageREDIS_PORT='')在啟動時就直接失敗;原本的程式碼會默默吃下這種值,等真正用到才爆,或根本不會爆。變數沒設仍然不擋啟動,只有設了無效值才會擋。issue 的測試清單漏了 tests/test_copycat.py:它在 runtime 改 TESTING env,讀取點搬到 settings 後 delenv 變成 no-op,背景執行緒不會啟動,測試會 hang 死,所以一併遷移。migrations/__init__.pyMONGO_HOST 讀取刻意不動,它不在 issue 的搬遷清單內,migration script 也是獨立執行的。

測試方面新增 tests/test_config.py(21 個案例:各欄位預設值、SMTP cross-field 驗證、TESTING 寬鬆解析、SUBMISSION_TMP_DIR 覆寫優先),原本 runtime 改 os.environ 的測試都改成 monkeypatch.setattr(settings, ...)。全套件 509 passed、yapf diff 乾淨。

新依賴:pydantic-settings 2.14.2。

Load and validate deployment settings once at startup via a
pydantic-settings `Settings` eager singleton (`settings = Settings()`).
Replace scattered os.environ reads across mongo/, model/, and app.py
with `config.settings`, and delete mongo/config.py plus the ad-hoc
setup_smtp() startup check (its SMTP cross-field rule moves into a
model_validator).

Two deliberate behavior changes:
- SUBMISSION_TMP_DIR default now uses tempfile.mkdtemp(suffix=
  'noj-submissions'), keeping a persistent directory (the old
  TemporaryDirectory(...).name kept no reference and was GC-deleted)
  and fixing the 'noj-submisisons' typo.
- FLASK_DEBUG renamed to DEBUG (bool, default False); its only consumer
  is MinioClient's secure=not DEBUG.

Tests that mutated os.environ for these vars now patch the singleton.
@as535364
as535364 requested review from Bogay and Copilot and removed request for Copilot July 20, 2026 10:47

Copilot AI left a comment

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.

Pull request overview

This PR centralizes previously scattered environment-variable reads into a single top-level config.py using pydantic-settings (Settings + import-time singleton settings), and updates runtime modules/tests to consistently read configuration from config.settings.

Changes:

  • Introduce config.Settings (pydantic-settings) and a module-level settings = Settings() singleton, with cross-field SMTP validation and typed parsing.
  • Migrate config consumers (Mongo engine/utils/user/submission, SMTP utils, app startup logging) to read from settings, and remove mongo/config.py.
  • Update tests to avoid runtime os.environ mutation by patching settings / constructing Settings() in isolation; add tests/test_config.py.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
config.py New centralized typed settings model and import-time singleton.
app.py Remove setup_smtp(); emit startup SMTP logs based on settings.
mongo/config.py Removed legacy env-var module for MinIO/FLASK_DEBUG.
mongo/engine.py Read MONGO_HOST from settings.
mongo/utils.py Switch TESTING/Redis/MinIO config reads to settings.
mongo/user.py Switch JWT config reads to settings (keeping JWT_EXP as days).
mongo/submission.py Use settings.SUBMISSION_TMP_DIR for submission tmp path.
model/utils/smtp.py Read SMTP configuration via settings.
tests/conftest.py Patch settings (MinIO creds, DEBUG, TESTING) instead of env/module globals.
tests/test_config.py New unit tests for defaults, parsing, SMTP cross-field validation, tmp dir behavior.
tests/test_copycat.py Patch settings.TESTING instead of deleting TESTING env var.
tests/test_mongo_utils.py Patch settings.REDIS_PORT instead of env var.
tests/test_smtp.py Patch settings.SMTP_SERVER instead of env var.
tests/unittest/submission/test_target_sandbox.py Use autouse fixture to isolate DB/config and set settings.TESTING.
pyproject.toml Add pydantic-settings dependency.
poetry.lock Lock updated to include pydantic-settings and transitive deps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread config.py Outdated
SMTP_NOREPLY: Optional[str] = None
SMTP_NOREPLY_PASSWORD: Optional[str] = None

SUBMISSION_TMP_DIR: str = None # type: ignore[assignment]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

已改用 Field(default_factory=...),原本的 before-validator 和 type: ignore 一併移除,行為不變(env 有設時 factory 不會執行):7488795

as535364 added 2 commits July 20, 2026 18:57
Addresses Copilot review: the str-annotated field had a None default with
a type: ignore. default_factory keeps the same behavior (mkdtemp only when
the env var is unset) without the type mismatch.
mkdtemp(suffix=...) appends the suffix, so endswith is the precise
assertion; a substring check would also pass on prefixed paths.

@as535364 as535364 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

LGTM

Comment thread config.py Outdated
The old-TemporaryDirectory rationale only made sense at migration time;
the behavior change is documented in the PR description.
@as535364
as535364 requested a review from Bogay July 20, 2026 12:57

@Bogay Bogay left a comment

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.

👍

@as535364
as535364 added this pull request to the merge queue Jul 20, 2026
Merged via the queue into main with commit a5934ee Jul 20, 2026
1 check passed
@as535364
as535364 deleted the refactor/centralize-config branch July 20, 2026 12:59
as535364 added a commit that referenced this pull request Jul 20, 2026
…ting (ADR-0005)

RUNNER_REGISTRATION_TOKEN moves from a live env accessor in dispatch/config.py
to a field on the centralized Settings (top-level config.py, #345), loaded and
validated once at startup. Unset/empty still means registration disabled (fail
closed); rotating the shared secret now takes a restart, per-runner revocation
stays immediate.

dispatch/config.py is renamed to dispatch/params.py and now holds only the
spec §13 protocol parameters — the dispatch module no longer reads settings
of its own.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: centralize deployment settings in top-level config.py (pydantic-settings)

3 participants