Fixed 1Password and the op.env file to properly handle shared secrets #361

Merged

johnsturgeon merged 6 commits from fix-1password-secrets-to-move-common-ones-to-shared-358 into main 2026-08-12 20:34:13 +02:00

Conversation 1 Commits 6 Files changed 11 +96 -191

johnsturgeon commented 2026-08-12 15:27:39 +02:00

Owner

Copy link

Also updated the structure of the secrets in 1Password to match the variable names
Also cleaned up unused variables
Also fixed linting errors in the config file.

This PR also resolves #340 (which is why local_dev files have been removed)

Also updated the structure of the secrets in 1Password to match the variable names Also cleaned up unused variables Also fixed linting errors in the config file. This PR also resolves #340 (which is why local_dev files have been removed)

johnsturgeon added 1 commit 2026-08-12 15:27:39 +02:00

Fixed 1Password and the op.env file to properly handle shared secrets ... ff70af988c

Also updated the structure of the secrets in 1Password to match the variable names
Also cleaned up unused variables
Also fixed linting errors in the config file.

forgejo-actions commented 2026-08-12 15:28:30 +02:00

Copy link

This is a clean, low-risk cleanup PR. I verified the removed config fields (KESTRA_*, LISTMONK_*, SEQ_*, LOG_LEVEL, USE_SQL, MONGO_URI) have no remaining references anywhere in the codebase, the renamed scripts exist, and docs/sample.env covers every field Config now requires. Here's the review.

---

Summary

This PR renames the local_dev environment to development across docs/scripts/compose, restructures config/op.env to pull shared secrets from a shared 1Password vault, and tightens Config.get_config() to fail fast on missing env vars. The changes are consistent and I found no blocking issues; a couple of behavioral shifts are worth a second look.

Worth fixing

• MAIL_SSL_TLS / MAIL_STARTTLS are now hardcoded and shared across all environments. In config/op.env they moved out of the per-env op://tgfp/${APP_ENV}/email/... secrets to plain non-secret values (MAIL_SSL_TLS=False, MAIL_STARTTLS=True). Dev and prod now get identical TLS behavior. That's reasonable for port-587/STARTTLS, but please confirm production's mail server doesn't rely on MAIL_SSL_TLS=True — this is a silent behavior change for prod, not just a rename.

• All config values are now mandatory — app/config/config.py:51-75 switched from os.getenv(...) (returns None) to os.environ[...] (raises KeyError). This is a good fail-fast improvement, but it now hard-crashes startup if SENTRY_DSN, UMAMI_TRACKING_ID, or API_BASE_URL are absent — integrations that were previously optional-by-omission. Since docs/sample.env lists all of them this is fine for fresh setups, but anyone with a pre-existing partial .env will need to add the missing keys. Worth a note in the PR description / migration notes.

Nits

• app/config/config.py:8 — env_bool is a genuine bug fix worth calling out: the old MAIL_STARTTLS=bool(os.getenv("MAIL_STARTTLS")) evaluated bool("False") → True, so STARTTLS was effectively always on regardless of the configured value. The new parser corrects this. No change requested — just flagging it as an intended semantic change reviewers/ops should be aware of.

• docs/sample.env sets DISCORD_REDIRECT_URI="https://tgfp.us/auth/callback" — a production URL in a file meant to bootstrap local dev. The prior http://127.0.0.1:8000/callback was more appropriate for a sample. Minor.

• No test covers env_bool, and there are no config-loading tests. A tiny parametrized test ("True"/"1"/"yes"/"false"/"" → expected bool) would lock in the fix above cheaply. Not required for this PR.

Nice cleanup overall — the dead-variable removal and the os.environ tightening both improve the config surface.

<!-- claude-code-review --> This is a clean, low-risk cleanup PR. I verified the removed config fields (`KESTRA_*`, `LISTMONK_*`, `SEQ_*`, `LOG_LEVEL`, `USE_SQL`, `MONGO_URI`) have no remaining references anywhere in the codebase, the renamed scripts exist, and `docs/sample.env` covers every field `Config` now requires. Here's the review. --- ## Summary This PR renames the `local_dev` environment to `development` across docs/scripts/compose, restructures `config/op.env` to pull shared secrets from a `shared` 1Password vault, and tightens `Config.get_config()` to fail fast on missing env vars. The changes are consistent and I found no blocking issues; a couple of behavioral shifts are worth a second look. ## Worth fixing - **`MAIL_SSL_TLS` / `MAIL_STARTTLS` are now hardcoded and shared across all environments.** In `config/op.env` they moved out of the per-env `op://tgfp/${APP_ENV}/email/...` secrets to plain non-secret values (`MAIL_SSL_TLS=False`, `MAIL_STARTTLS=True`). Dev and prod now get identical TLS behavior. That's reasonable for port-587/STARTTLS, but please confirm production's mail server doesn't rely on `MAIL_SSL_TLS=True` — this is a silent behavior change for prod, not just a rename. - **All config values are now mandatory** — `app/config/config.py:51-75` switched from `os.getenv(...)` (returns `None`) to `os.environ[...]` (raises `KeyError`). This is a good fail-fast improvement, but it now hard-crashes startup if `SENTRY_DSN`, `UMAMI_TRACKING_ID`, or `API_BASE_URL` are absent — integrations that were previously optional-by-omission. Since `docs/sample.env` lists all of them this is fine for fresh setups, but anyone with a pre-existing partial `.env` will need to add the missing keys. Worth a note in the PR description / migration notes. ## Nits - `app/config/config.py:8` — `env_bool` is a genuine bug fix worth calling out: the old `MAIL_STARTTLS=bool(os.getenv("MAIL_STARTTLS"))` evaluated `bool("False")` → `True`, so STARTTLS was effectively always on regardless of the configured value. The new parser corrects this. No change requested — just flagging it as an intended semantic change reviewers/ops should be aware of. - `docs/sample.env` sets `DISCORD_REDIRECT_URI="https://tgfp.us/auth/callback"` — a production URL in a file meant to bootstrap local dev. The prior `http://127.0.0.1:8000/callback` was more appropriate for a sample. Minor. - No test covers `env_bool`, and there are no config-loading tests. A tiny parametrized test (`"True"/"1"/"yes"/"false"/""` → expected bool) would lock in the fix above cheaply. Not required for this PR. Nice cleanup overall — the dead-variable removal and the `os.environ` tightening both improve the config surface.

johnsturgeon added 1 commit 2026-08-12 17:28:53 +02:00

Updated the config to use a wrapper for setting the 'bool' values 9c0b590430

johnsturgeon added 1 commit 2026-08-12 18:03:23 +02:00

Finished migration from local_dev to development fa60294ed7

johnsturgeon added 1 commit 2026-08-12 19:20:13 +02:00

went through readme claude.md and other places to clean up all references to local_dev ... 43dffb856b

Also cleaned up the sample.env which was quite out of date

johnsturgeon added 1 commit 2026-08-12 20:20:54 +02:00

Addressed claude comments on review 7b107d2d5c

johnsturgeon added 1 commit 2026-08-12 20:29:21 +02:00

Addressed a few more claude nits 56d6e7468e

johnsturgeon merged commit d8e1af65cb into main 2026-08-12 20:34:13 +02:00

johnsturgeon referenced this pull request from a commit 2026-08-12 20:34:13 +02:00

Fixed 1Password and the op.env file to properly handle shared secrets (#361)

johnsturgeon deleted branch fix-1password-secrets-to-move-common-ones-to-shared-358 2026-08-12 20:34:28 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

Kestra

  

bug

Something isn't working

  

enhancement

New feature or request

  

someday

Someday / Maybe

  

subtask

  

☁️ api

  

🎛️ infrastructure

  

🐞 sentry

  

📆 2025 Season

  

📝 pages

allpicks

Related to the All Picks page

  

📝 pages

picks

  

📝 pages

standings

  

🚀 performance

No labels

Kestra

bug

enhancement

someday

subtask

☁️ api

🎛️ infrastructure

🐞 sentry

📆 2025 Season

📝 pages

allpicks

📝 pages

picks

📝 pages

standings

🚀 performance

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

johnsturgeon/tgfp-web!361

No description provided.