feat(constance): support explicit CONSTANCE_* env var overrides for NLP settings DEV-1898#6933
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 Walkthrough📣 SummaryMake Google NLP (ASR/MT) Constance settings configurable via environment variables to allow deployment-time configuration without using the Django admin. 📖 DescriptionThis change updates four Constance-backed settings so they can be overridden at startup using environment variables, centralizing ASR/MT Google configuration in the environment and removing the need for post-deployment manual configuration in Django admin. 👷 Description for instance maintainersFour Constance settings in kobo/settings/base.py now read defaults from environment variables:
These values remain Constance-configurable in the Django admin, but when the corresponding CONSTANCE_* environment variable is set at startup it becomes the effective default, simplifying automated deployments and container-based setups. 💭 Notes
👀 Preview steps
WalkthroughFour Constance configuration entries in kobo/settings/base.py were changed to derive their default values from environment variables via Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| @@ -362,14 +362,14 @@ | |||
| ), | |||
| ), | |||
| 'ASR_MT_GOOGLE_TRANSLATION_LOCATION': ( | |||
| 'us-central1', | |||
| env.str('CONSTANCE_ASR_MT_GOOGLE_TRANSLATION_LOCATION', 'us-central1'), | |||
| ( | |||
| 'Google Cloud location to use for large translation tasks. It' | |||
| ' cannot be `global`, and Google only allows certain locations.' | |||
| ), | |||
| ), | |||
| 'ASR_MT_GOOGLE_CREDENTIALS': ( | |||
| '', | |||
| env.str('CONSTANCE_ASR_MT_GOOGLE_CREDENTIALS', ''), | |||
There was a problem hiding this comment.
Naming convention differs from existing constance-backed env vars
Other constance settings that read from env vars use either a project-specific prefix (KOBO_SUPPORT_EMAIL, KOBO_SUPPORT_URL) or the bare setting name (USAGE_LIMIT_ENFORCEMENT). The four new vars introduce a third convention: CONSTANCE_<SETTING_NAME>. This is arguably the clearest pattern, but it's inconsistent with the rest of the file and could confuse operators reading the code side-by-side. A brief code comment explaining why the CONSTANCE_ prefix is used here (and whether it should become the standard going forward) would help future contributors.
| 'ASR_MT_GOOGLE_PROJECT_ID': ( | |
| # Env var prefixed with CONSTANCE_ to signal it seeds the constance default. | |
| env.str('CONSTANCE_ASR_MT_GOOGLE_PROJECT_ID', 'kobo-asr-mt'), | |
| 'ID of the Google Cloud project used to access ASR/MT APIs', | |
| ), |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
@rajpatel24, the bot is right, it could be the standard in the future.
We could start to deprecat other not prefixed env vars and use the prefix everywhere.
Let's add a comment here. We can tackle this after migrating to Constance 4.x
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kobo/settings/base.py (1)
352-372:⚠️ Potential issue | 🟠 Major
CONSTANCE_*env vars here act as initialization defaults, not runtime overridesThe commit message claims "explicit CONSTANCE_* env var overrides," but the implementation only sets these as
CONSTANCE_CONFIGdefaults at startup. With the customDatabaseBackend(line 807), theget()method returns database-stored values if they exist, falling back toNonerather than theCONSTANCE_CONFIGdefaults. This means on instances with pre-existing database entries for these keys, environment variables will be silently ignored—they only take effect during initial setup when the database is empty.Clarify whether the requirement is:
- Env vars should set one-time initialization defaults (current behavior), or
- Env vars should override database values at runtime (not currently implemented)
If runtime override is needed, the custom backend's
get()method must be updated to check environment variables before querying the database.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 185862f6-babc-48b4-814e-cdc30d99a1d1
📒 Files selected for processing (1)
kobo/settings/base.py
| @@ -362,14 +362,14 @@ | |||
| ), | |||
| ), | |||
| 'ASR_MT_GOOGLE_TRANSLATION_LOCATION': ( | |||
| 'us-central1', | |||
| env.str('CONSTANCE_ASR_MT_GOOGLE_TRANSLATION_LOCATION', 'us-central1'), | |||
| ( | |||
| 'Google Cloud location to use for large translation tasks. It' | |||
| ' cannot be `global`, and Google only allows certain locations.' | |||
| ), | |||
| ), | |||
| 'ASR_MT_GOOGLE_CREDENTIALS': ( | |||
| '', | |||
| env.str('CONSTANCE_ASR_MT_GOOGLE_CREDENTIALS', ''), | |||
There was a problem hiding this comment.
@rajpatel24, the bot is right, it could be the standard in the future.
We could start to deprecat other not prefixed env vars and use the prefix everywhere.
Let's add a comment here. We can tackle this after migrating to Constance 4.x
… Constance settings DEV-2017 (#6965) ### 📖 Description We previously introduced `CONSTANCE_`-prefixed environment variable fallbacks (using `env.str()`) for the Google NLP settings (`ASR_MT_GOOGLE_*`) within the `CONSTANCE_CONFIG` dictionary in `base.py` ([#6933](#6933)). The Sysadmin team has since identified that injecting these configurations especially complex JSON credentials via environment variables leads to escaping issues in Kubernetes deployments. To improve stability and security, the team is transitioning to securely mounted files (e.g., Kubernetes Secrets) and relying on native Google library mechanisms (such as the `GOOGLE_APPLICATION_CREDENTIALS` environment variable) for NLP authentication and configuration. This PR removes the custom environment variable fallback logic from the codebase. The following four settings in base.py have been reverted to their original values to support this pattern: * `CONSTANCE_ASR_MT_GOOGLE_PROJECT_ID` →`'kobo-asr-mt'` * `CONSTANCE_ASR_MT_GOOGLE_STORAGE_BUCKET_PREFIX` →`'kobo-asr-mt-tmp'` * `CONSTANCE_ASR_MT_GOOGLE_TRANSLATION_LOCATION` →`'us-central1'` * `CONSTANCE_ASR_MT_GOOGLE_CREDENTIALS` → `""`
📖 Description
This PR allows the Google NLP (ASR/MT) settings to be configured directly via environment variables, eliminating the need for manual setup in the Django admin panel during deployments.
The following four settings have been explicitly updated in
base.pyto support this pattern:CONSTANCE_ASR_MT_GOOGLE_PROJECT_IDCONSTANCE_ASR_MT_GOOGLE_STORAGE_BUCKET_PREFIXCONSTANCE_ASR_MT_GOOGLE_TRANSLATION_LOCATIONCONSTANCE_ASR_MT_GOOGLE_CREDENTIALS