Testing: overriding env with test env #321
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughEnvironment handling was changed: OpenAI/Langfuse variables removed from configs, a testing env template and loader were added, settings and DB engine load env-files dynamically, private API routes enabled for development/testing, CI and test scripts updated to handle testing env, and .env ignore broadened. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Proc as Process Start
participant App as backend/app/main.py
participant Env as load_environment()
participant Cfg as get_settings()
participant DB as get_engine()
participant API as FastAPI App
Proc->>App: import main
App->>Env: load_environment()
note right of Env #DDEBF7: Read ENVIRONMENT (default: development)\nLoad .env or .env.test
Env-->>App: Env vars loaded
App->>Cfg: get_settings()
note right of Cfg #F5F7DC: Resolve _env_file by ENVIRONMENT\nInstantiate Settings
Cfg-->>App: settings
App->>DB: get_engine(settings.DATABASE_URL, settings.ENVIRONMENT)
note right of DB #FDE9E9: Configure pool sizes, pre_ping, recycle
DB-->>App: engine
App->>API: Initialize FastAPI, routers, middleware
API-->>App: App ready
sequenceDiagram
autonumber
participant CI as CI/Test Runner
participant Sh as backend/scripts/tests-start.sh
participant Py as pytest/conftest.py
participant DBM as Alembic
participant App as App Import
CI->>Sh: Run tests-start.sh
Sh->>Sh: export ENVIRONMENT=testing
Sh->>DBM: uv run alembic upgrade head
alt Migration fails
DBM-->>Sh: error
Sh-->>CI: exit 1
else Migration ok
DBM-->>Sh: success
Sh->>Py: invoke tests
Py->>Py: set ENVIRONMENT=testing early
Py->>App: import application code
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
backend/test_env.py (1)
1-18: Remove unusedbackend/test_env.pydevelopment artifact
backend/test_env.pyisn’t referenced or imported anywhere else in the codebase and contains a hard-coded, absolute path to a local.env.testfile. To keep the repository clean and avoid confusion:• File to address:
- backend/test_env.py
• Issue:- Standalone utility not integrated into the application
- Hard-coded, machine-specific
env_filepath
• Recommended actions:- Remove this file if it’s no longer needed
- Or relocate it outside version control and add to
.gitignoreif you wish to keep it locally
🧹 Nitpick comments (4)
backend/app/tests/api/routes/test_org.py (2)
19-19: Move import to top of file.The
import osshould be placed with other imports at the top of the file to follow PEP 8 conventions.+import os import pytest from fastapi.testclient import TestClient from sqlmodel import Session from app.core.config import settings from app.models import Organization from app.main import app from app.crud.organization import get_organization_by_id from app.tests.utils.test_data import create_test_organization client = TestClient(app) -import os
22-27: Rename test function for clarity.The test function name
test_secret_key_env()is misleading as it's testing thePOSTGRES_DBenvironment variable, not a secret key. Consider renaming totest_postgres_db_env()ortest_database_environment().-def test_secret_key_env(): +def test_postgres_db_env(): # Get the value of the POSTGRES_DB environment variable postgres_db = os.getenv("POSTGRES_DB") # Verify it's using the test database assert postgres_db == "ai_platform_test"The test logic itself is good - it ensures the correct test database is being used during testing.
backend/app/main.py (1)
1-1: Remove unused import.The
osimport is not used anywhere in the file and should be removed to clean up the imports.-import osbackend/app/core/db.py (1)
7-23: Review pool sizing logic for different environments.The dynamic engine creation is well-implemented, but the pool sizing logic may need adjustment. Currently, "development" gets the largest pool (20/30), but typically testing environments need more connections due to parallel test execution.
Consider adjusting the pool sizing logic:
def get_engine(): """Get database engine with current settings.""" # Import settings dynamically to get the current instance from app.core.config import settings - # Configure connection pool settings - # For testing, we need more connections since tests run in parallel - pool_size = 20 if settings.ENVIRONMENT == "development" else 5 - max_overflow = 30 if settings.ENVIRONMENT == "development" else 10 + # Configure connection pool settings based on environment needs + if settings.ENVIRONMENT == "testing": + # Testing needs more connections for parallel test execution + pool_size, max_overflow = 20, 30 + elif settings.ENVIRONMENT == "development": + # Development typically needs moderate connections + pool_size, max_overflow = 10, 20 + else: + # Production should be conservative with connections + pool_size, max_overflow = 5, 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.env.example(0 hunks).github/workflows/benchmark.yml(0 hunks).github/workflows/continuous_integration.yml(2 hunks).gitignore(1 hunks)backend/app/api/main.py(1 hunks)backend/app/core/config.py(5 hunks)backend/app/core/db.py(1 hunks)backend/app/load_env.py(1 hunks)backend/app/main.py(1 hunks)backend/app/tests/api/routes/test_onboarding.py(1 hunks)backend/app/tests/api/routes/test_org.py(1 hunks)backend/app/tests/conftest.py(3 hunks)backend/test_env.py(1 hunks)docker-compose.yml(0 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/benchmark.yml
- .env.example
- docker-compose.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/core/db.py (3)
backend/app/core/config.py (1)
SQLALCHEMY_DATABASE_URI(76-84)backend/app/initial_data.py (1)
init(11-13)backend/app/api/deps.py (1)
get_db(33-35)
🪛 Ruff (0.12.2)
backend/app/tests/api/routes/test_org.py
19-19: Module level import not at top of file
(E402)
backend/app/core/config.py
4-4: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
backend/app/main.py
1-1: os imported but unused
Remove unused import: os
(F401)
🔇 Additional comments (10)
.gitignore (1)
9-9: LGTM! Properly supports multiple environment files.The change from
.envto.env*correctly ensures all environment files are ignored by Git, supporting the new environment isolation strategy with files like.env.testand.env.example.backend/app/tests/api/routes/test_onboarding.py (1)
6-6: LGTM! Clean import removal.Good cleanup removing unused imports while retaining the necessary
Sessionimport for the test functions..github/workflows/continuous_integration.yml (2)
18-18: LGTM! Proper test database isolation.Changing
POSTGRES_DBtoai_platform_testcorrectly isolates the CI test environment from production database settings.
37-39: LGTM! Supports new environment file strategy.The updated environment file preparation correctly copies
.env.test.exampleto both.envand.env.test, supporting the new dynamic environment loading mechanism described in the PR objectives.backend/app/main.py (2)
12-16: Environment loading implementation looks correct.The addition of
load_environment()call early in the application startup ensures environment variables are loaded before other initialization steps. This aligns well with the dynamic environment loading strategy.
22-22: No “local” environment found—“development” is used consistently
We confirmed via code inspection and searches that:
- There are no references to a “local” environment.
APP_ENVdefaults to “development” in both load_env.py and config.py.settings.ENVIRONMENTlikewise defaults to “development” (and is overridden only where APP_ENV is “testing”).Therefore, changing the Sentry check to
settings.ENVIRONMENT != "development"aligns with the project’s environment naming and will initialize Sentry in testing, staging, and production as intended.backend/app/tests/conftest.py (3)
18-25: Settings singleton reset implementation is correct.The
pytest_configure()function properly sets the testing environment and resets the settings singleton to ensure tests run with the correct configuration. The approach of forcing a reload of settings after setting the environment variable is sound.
29-31: Dynamic engine usage is appropriate for tests.Switching from static
engineimport to dynamicget_engine()call ensures the database fixture uses the correct engine configuration for the testing environment.
52-54: Consistent use of dynamic engine in seed fixture.The seed_baseline fixture correctly uses
get_engine()to maintain consistency with the test database configuration approach.backend/app/core/db.py (1)
26-27: Backward compatibility approach is sound.Creating the global
enginevariable by callingget_engine()maintains backward compatibility while enabling the new dynamic configuration.
backend/app/api/main.py
Outdated
| api_router.include_router(utils.router) | ||
|
|
||
| if settings.ENVIRONMENT == "local": | ||
| if settings.ENVIRONMENT == "development": |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify environment naming consistency across codebase.
The change from "local" to "development" aligns with the updated environment naming conventions. Ensure all references to environment values are updated consistently.
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining references to "local" environment that should be "development"
# Expected: No hardcoded "local" environment references in environment checks
echo "Searching for environment references..."
rg -A 2 -B 2 'ENVIRONMENT.*==.*"local"'
rg -A 2 -B 2 'ENVIRONMENT.*==.*"dev"'
rg -A 2 -B 2 'ENV.*==.*"local"'Length of output: 204
🏁 Script executed:
#!/bin/bash
# Verify all ENVIRONMENT references and default settings
echo "Listing all occurrences of 'ENVIRONMENT' in the repository:"
rg -n 'ENVIRONMENT'
echo "Searching for default assignment of ENVIRONMENT in settings:"
rg -n 'ENVIRONMENT *=' -C 2
echo "Checking for hardcoded 'development' literal usages:"
rg -n '"development"'
echo "Checking for hardcoded 'production' literal usages:"
rg -n '"production"'Length of output: 3792
Update deployment documentation to “development”
The code consistently uses “development” (no remaining “local” checks), but the deployment guide still defaults to local. Please adjust:
• In deployment.md (around lines 116–119), change the default environment from local to development.
• Verify that your instructions (export commands, examples) align with the new naming convention.
🤖 Prompt for AI Agents
In backend/app/api/main.py at line 40, the environment check uses "development"
instead of "local". Update deployment.md around lines 116 to 119 to change the
default environment from "local" to "development" and ensure all export commands
and examples in the documentation reflect this new naming convention
consistently.
backend/app/core/config.py
Outdated
| class Settings(BaseSettings): | ||
| env: str = "development" # Default value, will be overridden in __init__ | ||
|
|
||
| def __init__(self, **kwargs): | ||
| # Determine env_file based on current environment at instantiation time | ||
| env = os.getenv("APP_ENV", "development") | ||
| # Use absolute path to ensure the file is found correctly | ||
| # config.py is in backend/app/core/, so we need to go up 3 levels to reach project root | ||
| base_dir = os.path.dirname( | ||
| os.path.dirname(os.path.dirname(os.path.dirname(__file__))) | ||
| ) | ||
| env_file = ( | ||
| os.path.join(base_dir, ".env.test") | ||
| if env == "testing" | ||
| else os.path.join(base_dir, ".env") | ||
| ) | ||
|
|
||
| # Load the environment file manually to ensure it's loaded correctly | ||
| if os.path.exists(env_file): | ||
| from dotenv import load_dotenv | ||
|
|
||
| load_dotenv(env_file, override=True) | ||
|
|
||
| # Pass env as a keyword argument to override the default | ||
| kwargs["env"] = env | ||
| super().__init__(**kwargs) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve path handling and optimize imports
Several issues with the current implementation:
- The nested
os.path.dirnamecalls are fragile and hard to maintain - Importing
dotenvinside__init__is inefficient - The default value for
envfield is misleading since it's always overridden
+from pathlib import Path
+from dotenv import load_dotenv
class Settings(BaseSettings):
- env: str = "development" # Default value, will be overridden in __init__
+ env: str # Will be set in __init__
def __init__(self, **kwargs):
# Determine env_file based on current environment at instantiation time
env = os.getenv("APP_ENV", "development")
- # Use absolute path to ensure the file is found correctly
- # config.py is in backend/app/core/, so we need to go up 3 levels to reach project root
- base_dir = os.path.dirname(
- os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
- )
- env_file = (
- os.path.join(base_dir, ".env.test")
- if env == "testing"
- else os.path.join(base_dir, ".env")
- )
+ # Use pathlib for more robust path handling
+ # config.py is in backend/app/core/
+ config_path = Path(__file__)
+ base_dir = config_path.parents[3] # Navigate up to project root
+ env_file = base_dir / (".env.test" if env == "testing" else ".env")
# Load the environment file manually to ensure it's loaded correctly
- if os.path.exists(env_file):
- from dotenv import load_dotenv
-
- load_dotenv(env_file, override=True)
+ if env_file.exists():
+ load_dotenv(env_file, override=True)
+ elif env != "development":
+ # Warn if env file is missing in non-development environments
+ warnings.warn(f"Environment file {env_file} not found for {env} environment")
# Pass env as a keyword argument to override the default
kwargs["env"] = env
super().__init__(**kwargs)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/core/config.py around lines 18 to 44, improve path handling by
replacing the nested os.path.dirname calls with a more robust method like using
pathlib's Path to navigate to the project root. Move the import of
dotenv.load_dotenv to the top of the file to avoid repeated imports inside
__init__. Remove the default value for the env field since it is always
overridden in __init__, or adjust the logic to set it properly without
overriding. These changes will make the code cleaner, more efficient, and easier
to maintain.
| def load_environment(): | ||
| env = os.getenv("APP_ENV", "development") | ||
|
|
||
| # Use the same path as config.py expects (one level above backend/) | ||
| env_file = "../.env" | ||
| if env == "testing": | ||
| env_file = "../.env.test" | ||
|
|
||
| load_dotenv(env_file) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve path robustness.
The function lacks error handling and uses relative paths that could be fragile depending on the working directory.
Consider these improvements:
+import os
+from pathlib import Path
from dotenv import load_dotenv
def load_environment():
env = os.getenv("APP_ENV", "development")
- # Use the same path as config.py expects (one level above backend/)
- env_file = "../.env"
+ # Use absolute path relative to this file's location
+ backend_dir = Path(__file__).parent.parent
+ env_file = backend_dir.parent / ".env"
if env == "testing":
- env_file = "../.env.test"
+ env_file = backend_dir.parent / ".env.test"
- load_dotenv(env_file)
+ try:
+ if env_file.exists():
+ load_dotenv(env_file)
+ else:
+ print(f"Warning: Environment file {env_file} not found")
+ except Exception as e:
+ print(f"Error loading environment file {env_file}: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def load_environment(): | |
| env = os.getenv("APP_ENV", "development") | |
| # Use the same path as config.py expects (one level above backend/) | |
| env_file = "../.env" | |
| if env == "testing": | |
| env_file = "../.env.test" | |
| load_dotenv(env_file) | |
| import os | |
| from pathlib import Path | |
| from dotenv import load_dotenv | |
| def load_environment(): | |
| env = os.getenv("APP_ENV", "development") | |
| # Use absolute path relative to this file's location | |
| backend_dir = Path(__file__).parent.parent | |
| env_file = backend_dir.parent / ".env" | |
| if env == "testing": | |
| env_file = backend_dir.parent / ".env.test" | |
| try: | |
| if env_file.exists(): | |
| load_dotenv(env_file) | |
| else: | |
| print(f"Warning: Environment file {env_file} not found") | |
| except Exception as e: | |
| print(f"Error loading environment file {env_file}: {e}") |
🤖 Prompt for AI Agents
In backend/app/load_env.py around lines 5 to 13, the load_environment function
lacks error handling and uses a fragile relative path for the env file. Modify
the function to construct an absolute path to the env file based on the current
file's directory to improve robustness. Add try-except blocks around the
load_dotenv call to catch and log any exceptions, ensuring the function handles
errors gracefully and provides useful feedback if loading the environment file
fails.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.env.test.example (1)
10-10: Unnecessary quoting tripsdotenv-linterwarnings
The two values below contain no spaces; the surrounding quotes aren’t needed and trigger lint warnings.-EMAIL_TEST_USER="test@example.com" +EMAIL_TEST_USER=test@example.com -AWS_S3_BUCKET_PREFIX="bucket-prefix-name" +AWS_S3_BUCKET_PREFIX=bucket-prefix-namePurely cosmetic, but it removes noise from CI lint jobs.
Also applies to: 25-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.test.example(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 8-8: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 9-9: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
[warning] 10-10: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[warning] 15-15: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 16-16: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 18-18: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
[warning] 24-24: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
[warning] 25-25: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
| @@ -0,0 +1,25 @@ | |||
| ENVIRONMENT=testing | |||
There was a problem hiding this comment.
ENVIRONMENT likely ignored – backend code expects APP_ENV
According to the PR description and conftest.py, the configuration loader branches on APP_ENV="testing".
Because this file sets ENVIRONMENT=testing, the loader will never see the flag and will fall back to the default (usually “production”) settings, defeating the purpose of the test-isolation work.
-ENVIRONMENT=testing
+APP_ENV=testingIf you still need ENVIRONMENT for other tooling, keep both lines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ENVIRONMENT=testing | |
| APP_ENV=testing |
🤖 Prompt for AI Agents
In the .env.test.example file at line 1, replace the variable name ENVIRONMENT
with APP_ENV to match the backend code's expected environment variable for
configuration loading. If ENVIRONMENT is still needed for other tools, keep both
lines by adding APP_ENV=testing alongside ENVIRONMENT=testing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
backend/app/core/config.py (4)
67-71: Duplicate source of truth for environment (ENVIRONMENTvsenv)Maintaining both
ENVIRONMENTandenvinvites drift. Either removeENVIRONMENTor sync it toenv.Two options:
- Remove the field if not needed:
- ENVIRONMENT: str = "development" # This is different from the env field
- Or keep it synchronized:
kwargs["env"] = env super().__init__(**kwargs) + # Keep legacy ENVIRONMENT synchronized + self.ENVIRONMENT = envPlease verify downstream usages before removal.
125-143: Singleton is broken; reset() cannot refresh the exportedsettings
__new__returns aSettingsinstance, sosettings = SettingsSingleton()becomes a concrete Settings object. CallingSettingsSingleton.reset()won’t affect the already-exportedsettings, and the proxy methods (__getattr__) never run. This prevents reliable reloading in tests and at runtime.Refactor to a real proxy (or use a cached getter). Proxy approach:
-class SettingsSingleton: - _instance: Settings | None = None - - def __new__(cls): - if cls._instance is None: - cls._instance = Settings() - return cls._instance - - def __getattr__(self, name): - return getattr(self._instance, name) - - @classmethod - def reset(cls): - """Reset the singleton instance to force recreation.""" - cls._instance = None - -# Create settings instance with lazy loading -settings = SettingsSingleton() # type: ignore +class SettingsSingleton: + _instance: Settings | None = None + + def _get(self) -> Settings: + if SettingsSingleton._instance is None: + SettingsSingleton._instance = Settings() + return SettingsSingleton._instance + + def __getattr__(self, name): + return getattr(self._get(), name) + + def __setattr__(self, name, value): + if name == "_instance": + object.__setattr__(self, name, value) + else: + setattr(self._get(), name, value) + + @classmethod + def reset(cls): + """Reset the singleton instance to force recreation.""" + cls._instance = None + +# Stable proxy instance +settings = SettingsSingleton()Alternatively, replace the class with a cached getter:
from functools import lru_cache @lru_cache def get_settings() -> Settings: return Settings()Then use
get_settings()everywhere andget_settings.cache_clear()in tests.
17-19: Type-safe env; validate allowed values; removetype: ignore
envis a plainstr, whileENVclaims aLiteral. Add validation and align the type to eliminate# type: ignore.- Since
__init__always setsenv, the field doesn’t need a default.-class Settings(BaseSettings): - env: str = "development" # Default value, will be overridden in __init__ +class Settings(BaseSettings): + env: Literal["testing", "development", "staging", "production"] @@ def __init__(self, **kwargs): # Determine env_file based on current environment at instantiation time env = os.getenv("ENVIRONMENT", "development") + valid_envs = {"testing", "development", "staging", "production"} + if env not in valid_envs: + raise ValueError(f"Invalid ENVIRONMENT: {env}. Must be one of {valid_envs}") @@ - # Pass env as a keyword argument to override the default + # Pass env as a keyword argument to override the field kwargs["env"] = env super().__init__(**kwargs) @@ - def ENV(self) -> Literal["testing", "development", "staging", "production"]: - return self.env # type: ignore + def ENV(self) -> Literal["testing", "development", "staging", "production"]: + return self.envAlso applies to: 54-57, 41-42
20-39: Centralize and harden env-file loading; use pathlib; avoid importing in init
- Replace fragile nested
os.path.dirnamewithpathlib.- Import
load_dotenvat module scope.- Optionally warn if env file missing (non-dev) and avoid overriding OS env in prod.
+from pathlib import Path +from dotenv import load_dotenv @@ - # Use absolute path to ensure the file is found correctly - # config.py is in backend/app/core/, so we need to go up 3 levels to reach project root - base_dir = os.path.dirname( - os.path.dirname(os.path.dirname(os.path.dirname(__file__))) - ) - env_file = ( - os.path.join(base_dir, ".env.test") - if env == "testing" - else os.path.join(base_dir, ".env") - ) + # Resolve project root (backend/app/core/config.py → project/) + base_dir = Path(__file__).resolve().parents[3] + env_file = base_dir / (".env.test" if env == "testing" else ".env") @@ - # Load the environment file manually to ensure it's loaded correctly - if os.path.exists(env_file): - from dotenv import load_dotenv - - load_dotenv(env_file, override=True) + # Load the environment file if present + if env_file.exists(): + load_dotenv(env_file, override=(env in {"testing", "development"})) + elif env != "development": + warnings.warn(f"Environment file {env_file} not found for {env} environment")Note: if the PR’s
load_environment()is the single point of truth, consider removing file-loading from here entirely to avoid double-loading and ordering issues.
🧹 Nitpick comments (3)
backend/app/core/config.py (3)
101-101: Prefer pathlib forLOG_DIRMinor readability/portability improvement.
- LOG_DIR: str = os.path.join(os.path.dirname(os.path.dirname(__file__)), "logs") + LOG_DIR: str = str(Path(__file__).resolve().parents[1] / "logs")
24-27: Comment mismatch: says “go up 3 levels” but code goes up 4The code resolves to project root (correct), but the comment is misleading.
- # config.py is in backend/app/core/, so we need to go up 3 levels to reach project root + # config.py is in backend/app/core/; go up 4 levels to reach project root
35-39: Don’t override system env in non-dev/test environmentsCurrently
override=Truecan replace process env with.envvalues. Safer default: only override for development/testing, or make it configurable.See suggested diff in the earlier path-handling comment using:
override=(env in {"testing", "development"}).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/core/config.py(5 hunks)
🔇 Additional comments (2)
backend/app/core/config.py (2)
99-100: LGTM: S3 bucket now derives from typedENVUsing
ENVinAWS_S3_BUCKETis consistent with the new environment model.
109-113: LGTM: sensible warning vs. error behavior for secretsWarning in development/testing and error otherwise is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.env.test.example (1)
1-1: Use APP_ENV=testing (keep ENVIRONMENT if needed). Test loader likely ignores ENVIRONMENT.Backend config appears to branch on APP_ENV="testing". Keep both to de-risk, but ensure APP_ENV is present.
-ENVIRONMENT=testing +APP_ENV=testing +ENVIRONMENT=testingAlso ensure tests (e.g., conftest.py or CI) export APP_ENV=testing before loading settings.
🧹 Nitpick comments (1)
.env.test.example (1)
3-25: Satisfy dotenv-linter: remove unnecessary quotes and optionally order keys within sections.Quotes are unnecessary for EMAIL_TEST_USER and AWS_S3_BUCKET_PREFIX; removing them clears the QuoteCharacter warnings. Optionally, order keys alphabetically within each section to silence UnorderedKey warnings and keep the file tidy. Keep quotes for PROJECT_NAME because it contains a space.
PROJECT_NAME="AI Platform" STACK_NAME=ai-platform #Backend -SECRET_KEY=changethis -FIRST_SUPERUSER=superuser@example.com -FIRST_SUPERUSER_PASSWORD=changethis -EMAIL_TEST_USER="test@example.com" +EMAIL_TEST_USER=test@example.com +FIRST_SUPERUSER=superuser@example.com +FIRST_SUPERUSER_PASSWORD=changethis +SECRET_KEY=changethis # Postgres - -POSTGRES_SERVER=localhost -POSTGRES_PORT=5432 -POSTGRES_DB=ai_platform_test -POSTGRES_USER=postgres -POSTGRES_PASSWORD=postgres +POSTGRES_DB=ai_platform_test +POSTGRES_PASSWORD=postgres +POSTGRES_PORT=5432 +POSTGRES_SERVER=localhost +POSTGRES_USER=postgres # AWS - -AWS_ACCESS_KEY_ID=this_is_a_test_key -AWS_SECRET_ACCESS_KEY=this_is_a_test_key -AWS_DEFAULT_REGION=ap-south-1 -AWS_S3_BUCKET_PREFIX="bucket-prefix-name" +AWS_ACCESS_KEY_ID=this_is_a_test_key +AWS_DEFAULT_REGION=ap-south-1 +AWS_S3_BUCKET_PREFIX=bucket-prefix-name +AWS_SECRET_ACCESS_KEY=this_is_a_test_key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.test.example(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 8-8: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 9-9: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
[warning] 10-10: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[warning] 15-15: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 16-16: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 18-18: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
[warning] 24-24: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 25-25: [QuoteCharacter] The value has quote characters (', ")
[warning] 25-25: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
| POSTGRES_SERVER=localhost | ||
| POSTGRES_PORT=5432 | ||
| POSTGRES_DB=ai_platform_test | ||
| POSTGRES_USER=postgres | ||
| POSTGRES_PASSWORD=postgres |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Docker Compose gotcha: localhost won’t reach the DB container.
If tests run via docker compose, POSTGRES_SERVER should be the service name (commonly "postgres"), not localhost. Otherwise the app container can’t reach the DB and tests will fail/flap.
Consider documenting an alternative in the example:
POSTGRES_SERVER=localhost
+# If running via Docker Compose, prefer the service name:
+# POSTGRES_SERVER=postgresRun this to verify Compose service naming and usage across the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for compose files and postgres service names..."
fd -HI 'docker-compose*.yml' -t f || true
fd -HI 'docker-compose*.yaml' -t f || true
fd -HI 'compose*.yml' -t f || true
fd -HI 'compose*.yaml' -t f || true
echo "Look for a postgres service definition..."
rg -n -A3 -i 'services:|^\s*postgres:|container_name:\s*postgres' || true
echo "Check where POSTGRES_SERVER is consumed..."
rg -n 'POSTGRES_SERVER' || true
echo "Check for DATABASE_URL usage that may bypass POSTGRES_*..."
rg -n 'DATABASE_URL' || trueLength of output: 1779
Update Docker Compose host to db
For tests running under Docker Compose, the PostgreSQL host must match the service name (db), not localhost, or the app container won’t be able to reach the database. Consider updating the example in .env.test.example:
POSTGRES_SERVER=localhost
+# If running tests via Docker Compose, use the `db` service:
+# POSTGRES_SERVER=db
POSTGRES_PORT=5432
POSTGRES_DB=ai_platform_test
POSTGRES_USER=postgres
POSTGRES_PASSWORD=postgres- docker-compose.yml defines the database service as
db(lines 1–4) - Environment overrides in docker-compose.yml already set
POSTGRES_SERVER=db(lines 69 and 105)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| POSTGRES_SERVER=localhost | |
| POSTGRES_PORT=5432 | |
| POSTGRES_DB=ai_platform_test | |
| POSTGRES_USER=postgres | |
| POSTGRES_PASSWORD=postgres | |
| POSTGRES_SERVER=localhost | |
| # If running tests via Docker Compose, use the `db` service: | |
| # POSTGRES_SERVER=db | |
| POSTGRES_PORT=5432 | |
| POSTGRES_DB=ai_platform_test | |
| POSTGRES_USER=postgres | |
| POSTGRES_PASSWORD=postgres |
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 15-15: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 16-16: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 18-18: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
🤖 Prompt for AI Agents
In the .env.test.example file between lines 14 and 18, the POSTGRES_SERVER is
set to localhost, which will prevent the app container from connecting to the
database when running under Docker Compose. Update the POSTGRES_SERVER value
from localhost to db to match the service name defined in docker-compose.yml,
ensuring proper connectivity during tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.env.test.example (2)
1-1: APP_ENV vs ENVIRONMENT mismatch may bypass test loaderYour example sets ENVIRONMENT=testing, but prior reviews and PR notes indicate backend code branches on APP_ENV="testing". If the code reads APP_ENV, tests won’t isolate correctly.
Proposed fix (keep both if other tools still rely on ENVIRONMENT):
-ENVIRONMENT=testing +APP_ENV=testing +# If other tooling requires this, keep it as well: +# ENVIRONMENT=testingRun this to confirm which env var the code actually reads:
#!/bin/bash set -euo pipefail echo "Searching references to APP_ENV and ENVIRONMENT…" rg -n --hidden --no-ignore 'APP_ENV|ENVIRONMENT' echo echo "Look for loader and settings usage…" rg -n --hidden --no-ignore 'load_environment|Settings\(|class Settings|settings\s*=|settings\.__init__'
14-18: Postgres host should be service name under Docker Compose; also align key orderIn containers, localhost won’t reach the DB service. Use the Compose service name (db). Also re-order to satisfy dotenv-linter’s alphabetical preference.
-POSTGRES_SERVER=localhost -POSTGRES_PORT=5432 -POSTGRES_DB=ai_platform_test -POSTGRES_USER=postgres -POSTGRES_PASSWORD=postgres +# If running tests via Docker Compose, prefer the service name: +# POSTGRES_SERVER=db +# Otherwise for local, uncomment: +# POSTGRES_SERVER=localhost +POSTGRES_DB=ai_platform_test +POSTGRES_PASSWORD=postgres +POSTGRES_PORT=5432 +POSTGRES_SERVER=db +POSTGRES_USER=postgres
🧹 Nitpick comments (4)
.env.test.example (2)
10-10: Remove unnecessary quotes to satisfy dotenv lintersQuotes are not needed here and are flagged by dotenv-linter.
-EMAIL_TEST_USER="test@example.com" +EMAIL_TEST_USER=test@example.com
27-30: AWS block: order keys and drop quotes on the bucket prefixAlphabetize to quiet dotenv-linter and remove quotes around the prefix.
-AWS_ACCESS_KEY_ID=this_is_a_test_key -AWS_SECRET_ACCESS_KEY=this_is_a_test_key -AWS_DEFAULT_REGION=ap-south-1 -AWS_S3_BUCKET_PREFIX="bucket-prefix-name" +AWS_ACCESS_KEY_ID=this_is_a_test_key +AWS_DEFAULT_REGION=ap-south-1 +AWS_S3_BUCKET_PREFIX=bucket-prefix-name +AWS_SECRET_ACCESS_KEY=this_is_a_test_keybackend/app/tests/api/routes/documents/test_route_document_permanent_remove.py (1)
61-66: Handle us-east-1 bucket creation quirk to avoid InvalidLocationConstraintPassing CreateBucketConfiguration for us-east-1 causes errors with real S3 (moto mirrors behavior). Make creation conditional, so tests won’t break if AWS_DEFAULT_REGION changes.
-aws.client.create_bucket( - Bucket=settings.AWS_S3_BUCKET, - CreateBucketConfiguration={ - "LocationConstraint": settings.AWS_DEFAULT_REGION, - }, -) +region = settings.AWS_DEFAULT_REGION +args = {"Bucket": settings.AWS_S3_BUCKET} +if region and region != "us-east-1": + args["CreateBucketConfiguration"] = {"LocationConstraint": region} +aws.client.create_bucket(**args)Also consider extracting this into a small helper/fixture (e.g., ensure_test_bucket()) to remove duplication across tests.
backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
73-78: Make bucket creation region-safe and DRY across testsMirror the us-east-1 handling to prevent future breakage if region switches, and consider a shared helper/fixture to avoid repetition.
-aws.client.create_bucket( - Bucket=settings.AWS_S3_BUCKET, - CreateBucketConfiguration={ - "LocationConstraint": settings.AWS_DEFAULT_REGION, - }, -) +region = settings.AWS_DEFAULT_REGION +args = {"Bucket": settings.AWS_S3_BUCKET} +if region and region != "us-east-1": + args["CreateBucketConfiguration"] = {"LocationConstraint": region} +aws.client.create_bucket(**args)Optionally add a fixture in conftest.py:
@pytest.fixture def ensure_test_bucket(): from app.core.config import settings from app.core.cloud import AmazonCloudStorageClient def _ensure(): aws = AmazonCloudStorageClient() region = settings.AWS_DEFAULT_REGION args = {"Bucket": settings.AWS_S3_BUCKET} if region and region != "us-east-1": args["CreateBucketConfiguration"] = {"LocationConstraint": region} aws.client.create_bucket(**args) return aws return _ensureThen call aws = ensure_test_bucket() in tests.
Also applies to: 95-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.test.example(1 hunks)backend/app/api/main.py(1 hunks)backend/app/tests/api/routes/documents/test_route_document_permanent_remove.py(1 hunks)backend/app/tests/api/routes/documents/test_route_document_upload.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/main.py
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.test.example
[warning] 8-8: [UnorderedKey] The FIRST_SUPERUSER key should go before the SECRET_KEY key
[warning] 9-9: [UnorderedKey] The FIRST_SUPERUSER_PASSWORD key should go before the SECRET_KEY key
[warning] 10-10: [QuoteCharacter] The value has quote characters (', ")
[warning] 10-10: [UnorderedKey] The EMAIL_TEST_USER key should go before the FIRST_SUPERUSER key
[warning] 15-15: [UnorderedKey] The POSTGRES_PORT key should go before the POSTGRES_SERVER key
[warning] 16-16: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
[warning] 18-18: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
[warning] 29-29: [UnorderedKey] The AWS_DEFAULT_REGION key should go before the AWS_SECRET_ACCESS_KEY key
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
[warning] 30-30: [UnorderedKey] The AWS_S3_BUCKET_PREFIX key should go before the AWS_SECRET_ACCESS_KEY key
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
AkhileshNegi
force-pushed
the
enhancement/testing-env
branch
from
b3580f4 to
d204f1a
Compare
backend/app/core/config.py
Outdated
| elif isinstance(v, list | str): | ||
| return v | ||
| raise ValueError(v) | ||
| class Settings(BaseSettings): |
There was a problem hiding this comment.
this is too complex, custom __init__ logic and path manipulation would be hard to maintain
we could do something like this:
import os
from functools import lru_cache
from typing import Literal
from pydantic import computed_field
from pydantic_settings import BaseSettings, SettingsConfigDict
class Settings(BaseSettings):
# Environment-specific configuration
ENVIRONMENT: Literal["development", "testing", "staging", "production"] = "development"
# Core settings
API_V1_STR: str = "/api/v1"
SECRET_KEY: str = "changethis"
PROJECT_NAME: str = "AI Platform"
# Database
POSTGRES_SERVER: str = "localhost"
POSTGRES_PORT: int = 5432
POSTGRES_USER: str = "postgres"
POSTGRES_PASSWORD: str = ""
POSTGRES_DB: str = "ai_platform"
@computed_field
@property
def SQLALCHEMY_DATABASE_URI(self) -> str:
return f"postgresql://{self.POSTGRES_USER}:{self.POSTGRES_PASSWORD}@{self.POSTGRES_SERVER}:{self.POSTGRES_PORT}/{self.POSTGRES_DB}"
@lru_cache()
def get_settings() -> Settings:
"""Get settings with appropriate env file based on ENVIRONMENT."""
environment = os.getenv("ENVIRONMENT", "development")
# Determine env file
env_files = {
"testing": ".env.test",
"staging": ".env.staging",
"production": ".env.prod",
}
env_file = env_files.get(environment, ".env")
# Only use env_file if it exists
if not os.path.exists(env_file):
env_file = None
return Settings(_env_file=env_file)
# Export settings instance
settings = get_settings()
backend/app/core/config.py
Outdated
|
|
||
|
|
||
| settings = Settings() # type: ignore | ||
| class SettingsSingleton: |
There was a problem hiding this comment.
this singleton can be removed, if the suggested approach is used
the @lru_cache() on get_settings() will ensure single instance per environment
| from app.tests.utils.utils import get_superuser_token_headers, get_api_key_by_email | ||
| from app.seed_data.seed_data import seed_database | ||
|
|
||
|
|
There was a problem hiding this comment.
assuming, the suggested option in app.core.config is used:
# Set environment before importing settings
def pytest_configure():
os.environ["ENVIRONMENT"] = "testing"
# Now import after setting environment
from app.core.config import settings
from app.core.db import engine
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/core/config.py (1)
19-24: TypeError risk: invalid isinstance with union typesisinstance(v, list | str) raises TypeError on Python 3.10/3.11. Use a tuple of types.
Apply this diff:
- elif isinstance(v, list | str): + elif isinstance(v, (list, str)):
🧹 Nitpick comments (5)
backend/app/tests/conftest.py (1)
2-6: Initialize test ENV without clobbering existing valueSetting ENVIRONMENT early is correct. Prefer setdefault to avoid overriding a value already provided by CI or the runner.
Apply this diff:
-# Set environment before importing ANYTHING else -os.environ["ENVIRONMENT"] = "testing" +# Set environment before importing ANYTHING else +os.environ.setdefault("ENVIRONMENT", "testing")Note: PR summary mentions invoking load_environment automatically before tests run, but this file does not call it. If the intended flow is to rely solely on pydantic Settings(_env_file=...) via get_settings(), this is fine. If you want the dotenv loader’s warnings/fallback behavior, call load_environment() here before importing settings/engine. Please confirm the intended approach.
backend/scripts/tests-start.sh (1)
5-7: Don’t overwrite ENVIRONMENT if already setAllow runners/CI to override the environment without being clobbered by the script.
Apply this diff:
-# Set environment for testing -export ENVIRONMENT=testing +# Set environment for testing (allow override if already set) +: "${ENVIRONMENT:=testing}" +export ENVIRONMENTbackend/app/core/config.py (3)
4-4: Remove unused import AnnotatedAnnotated isn’t used in this module.
Apply this diff:
-from typing import Annotated, Any, Literal +from typing import Any, Literal
102-103: Prefer @lru_cache without parenthesesStyle nit: with no arguments, parentheses are unnecessary (ruff UP011).
Apply this diff:
-@lru_cache() +@lru_cache
107-113: Resolve env file robustly and guard missing filesRelying on "../.env" relative strings is brittle. Resolve from this file’s directory and fall back to None if missing so BaseSettings won’t try to load a non-existent file.
Apply this diff:
- # Determine env file - env_files = {"testing": "../.env.test", "development": "../.env"} - env_file = env_files.get(environment, "../.env") - - # Create Settings instance with the appropriate env file - return Settings(_env_file=env_file) + # Determine env file (resolve relative to backend/app) + base_dir = os.path.dirname(os.path.dirname(__file__)) # backend/app + env_files = { + "testing": os.path.join(base_dir, ".env.test"), + "development": os.path.join(base_dir, ".env"), + } + env_file = env_files.get(environment, os.path.join(base_dir, ".env")) + # Only use env_file if it exists; otherwise skip to environment/defaults + if not os.path.exists(env_file): + env_file = None + return Settings(_env_file=env_file)Optional: If you plan dedicated files for staging/production later, extend env_files accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/app/core/config.py(4 hunks)backend/app/tests/conftest.py(1 hunks)backend/scripts/tests-start.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/scripts/tests-start.sh (6)
backend/app/tests_pre_start.py (2)
init(22-29)main(32-35)backend/app/tests/scripts/test_test_pre_start.py (1)
test_init_success(5-23)backend/app/tests/scripts/test_backend_pre_start.py (1)
test_init_success(5-23)backend/app/alembic/versions/99f4fc325617_add_organization_project_setup.py (1)
upgrade(20-72)backend/app/alembic/versions/0f205e3779ee_add_api_key_table.py (1)
upgrade(20-39)backend/app/alembic/versions/4aa1f48c6321_add_inconistency_fixes.py (1)
upgrade(20-49)
backend/app/tests/conftest.py (3)
backend/app/tests_pre_start.py (1)
init(22-29)backend/app/tests/scripts/test_backend_pre_start.py (1)
test_init_success(5-23)backend/app/tests/scripts/test_test_pre_start.py (1)
test_init_success(5-23)
🪛 Ruff (0.12.2)
backend/app/core/config.py
102-102: Unnecessary parentheses to functools.lru_cache
Remove unnecessary parentheses
(UP011)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (1)
backend/app/tests/conftest.py (1)
12-12: Import order LGTMAcknowledging imports after ENV setup. This avoids premature instantiation of settings/engine with the wrong environment.
If app.main calls load_environment() at import time, verify it doesn’t reinitialize settings in a way that diverges from the settings/engine already imported here. The current order (ENV set before settings/engine imports) should keep them consistent.
| # Run pending migrations for test database | ||
| uv run alembic upgrade head | ||
| if [ $? -ne 0 ]; then | ||
| echo 'Error: Test database migrations failed' | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Redundant exit-code check under set -e
With set -e, the script already exits on a non-zero status. The manual $? check is unnecessary.
Apply this diff:
-# Run pending migrations for test database
-uv run alembic upgrade head
-if [ $? -ne 0 ]; then
- echo 'Error: Test database migrations failed'
- exit 1
-fi
+# Run pending migrations for test database
+uv run alembic upgrade headAlso verify that the uv CLI is available in all environments running this script, or fall back to invoking Alembic directly:
🏁 Script executed:
#!/usr/bin/env bash
# Verify uv availability and suggest a fallback to plain alembic if missing
if ! command -v uv >/dev/null 2>&1; then
echo "Warning: 'uv' not found. Consider using 'alembic upgrade head' instead of 'uv run alembic upgrade head'."
fiLength of output: 252
Remove redundant exit-code check and ensure Alembic runs whether or not uv is installed
With set -e in place, the manual if [ $? -ne 0 ]…exit 1 block is unnecessary. Additionally, since uv isn’t guaranteed to be available in all environments, fall back to invoking Alembic directly:
• File: backend/scripts/tests-start.sh (around lines 10–16)
-# Run pending migrations for test database
-uv run alembic upgrade head
-if [ $? -ne 0 ]; then
- echo 'Error: Test database migrations failed'
- exit 1
-fi
+# Run pending migrations for test database
+if command -v uv >/dev/null 2>&1; then
+ uv run alembic upgrade head
+else
+ alembic upgrade head
+fiThis preserves the “fail fast” behavior of set -e and guarantees migrations run even if uv isn’t installed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run pending migrations for test database | |
| uv run alembic upgrade head | |
| if [ $? -ne 0 ]; then | |
| echo 'Error: Test database migrations failed' | |
| exit 1 | |
| fi | |
| # Run pending migrations for test database | |
| if command -v uv >/dev/null 2>&1; then | |
| uv run alembic upgrade head | |
| else | |
| alembic upgrade head | |
| fi |
🤖 Prompt for AI Agents
In backend/scripts/tests-start.sh around lines 10 to 16, remove the redundant
manual exit-code check (the if [ $? -ne 0 ] ... exit 1 block) because set -e
already enforces fail-fast, and replace the hardcoded `uv run alembic upgrade
head` invocation with a fallback that tries `uv run alembic upgrade head` if
`uv` is available, otherwise runs `alembic upgrade head` directly so migrations
run even when `uv` is not installed; keep existing set -e and ensure the script
exits non-zero on failure.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/core/config.py (1)
79-88: Secrets validation misses empty values; treat empty as insecure (raise outside dev/test).Currently you only guard against the sentinel "changethis". Empty secrets (e.g.,
POSTGRES_PASSWORD="") should also warn in dev/test and raise in staging/prod.Apply:
def _check_default_secret(self, var_name: str, value: str | None) -> None: - if value == "changethis": + # Consider both placeholder and empty as insecure + if not value or value == "changethis": message = ( f'The value of {var_name} is "changethis", ' "for security, please change it, at least for deployments." ) - if self.ENVIRONMENT in ["development", "testing"]: + if self.ENVIRONMENT in ["development", "testing"]: warnings.warn(message, stacklevel=1) else: raise ValueError(message)Optional: bump
stacklevelto 2 so the warning points to the caller.
♻️ Duplicate comments (1)
backend/app/core/config.py (1)
4-4: Previous “unused Any” comment is no longer valid — keepAny.
Anyis used byparse_cors(v: Any). No action needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/app/core/config.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (2)
backend/app/core/config.py (2)
28-31: LGTM: env file moved out ofmodel_config.Deferring env file selection to a factory makes test-time overrides and per-env bootstrapping cleaner.
37-39: ENVIRONMENT guards verified — no legacy values foundWe searched the codebase and confirmed:
- No references remain to the legacy
APP_ENVvariable or a"local"environment setting.- In
backend/app/main.py, Sentry is initialized only whensettings.ENVIRONMENT != "development".- In
backend/app/core/db.py, the connection-pool settings correctly usesettings.ENVIRONMENT == "development"for increased pool sizes.- In
backend/app/api/main.py, the private router is only included whensettings.ENVIRONMENTis one of["development", "testing"].- The
.envfile selection in bothbackend/app/load_env.pyandget_settings()covers"testing"and"development", falling back to the default.envfile for"staging"and"production".All downstream guards now align with the expanded environment literals. No further changes required.
| def get_settings() -> Settings: | ||
| """Get settings with appropriate env file based on ENVIRONMENT.""" | ||
| environment = os.getenv("ENVIRONMENT", "development") | ||
|
|
||
| # Determine env file | ||
| env_files = {"testing": "../.env.test", "development": "../.env"} | ||
| env_file = env_files.get(environment, "../.env") | ||
|
|
||
| # Create Settings instance with the appropriate env file | ||
| return Settings(_env_file=env_file) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Relative _env_file paths will mis-resolve when CWD varies (pytest, Docker, IDE) — switch to absolute Path, add existence check, and cache.
"../.env*" is evaluated relative to the current working directory, not this file. Under repo-root pytest or container entrypoints, it will point outside the repository. Also, constructing settings repeatedly is unnecessary. Use an absolute path from __file__, guard for missing files, and cache the instance. Provide a reload hook for tests instead of calling settings.__init__().
Apply within this hunk:
-def get_settings() -> Settings:
- """Get settings with appropriate env file based on ENVIRONMENT."""
- environment = os.getenv("ENVIRONMENT", "development")
-
- # Determine env file
- env_files = {"testing": "../.env.test", "development": "../.env"}
- env_file = env_files.get(environment, "../.env")
-
- # Create Settings instance with the appropriate env file
- return Settings(_env_file=env_file)
+@lru_cache
+def get_settings() -> Settings:
+ """Get settings with appropriate env file based on ENVIRONMENT."""
+ environment = os.getenv("ENVIRONMENT", "development")
+
+ # Resolve project root from this file: backend/app/core/config.py -> repo root
+ base_dir = Path(__file__).resolve().parents[3]
+
+ # Map env → filename; staging/prod typically rely on real env vars, so no file
+ mapping = {
+ "testing": base_dir / ".env.test",
+ "development": base_dir / ".env",
+ }
+ env_file_path = mapping.get(environment)
+ if env_file_path is not None and not env_file_path.exists():
+ # Fall back to OS env only if the file isn't present
+ env_file_path = None
+
+ return Settings(_env_file=env_file_path)Add these imports at the top of the file:
from functools import lru_cache
from pathlib import PathRecommended (outside this hunk): provide an explicit reload hook for tests and fixtures that currently do settings.__init__():
def reload_settings() -> Settings:
"""Clear cached settings and rebuild after dotenv changes in tests."""
get_settings.cache_clear()
return get_settings()And replace any settings.__init__() calls in test bootstrapping with reload_settings() to avoid losing _env_file context.
I can push a follow-up patch updating conftest.py to call reload_settings() after load_environment().
To confirm the risks and cleanup opportunities:
Also applies to: 113-114
🏁 Script executed:
#!/bin/bash
# Where is settings reinitialized?
rg -nP -C2 --type=py 'settings\.__init__\s*\('
# Do we accidentally rely on CWD for dotenv files elsewhere?
rg -nP -C2 --type=py '\.\./\.env(\.test)?'
# Validate the actual presence of example/test dotenv at repo root
fd -H --max-depth 2 '.env*' -x ls -al {} || trueLength of output: 2080
Absolute path resolution and caching for .env loading in config.py and load_env.py
To avoid mis-resolved relative paths when the CWD changes (pytest, Docker, IDE), refactor both modules to compute the project root from __file__, use Path, guard for missing files, and cache the settings:
• backend/app/core/config.py
• Wrap get_settings() with @lru_cache
• Compute base_dir = Path(__file__).resolve().parents[3]
• Map environment → base_dir/".env" or ".env.test"
• Check env_file_path.exists(), else set to None
• Return Settings(_env_file=env_file_path)
-from functools import lru_cache
+from functools import lru_cache
from os import getenv
-from pathlib import Path
+from pathlib import Path
@lru_cache
def get_settings() -> Settings:
"""Get settings with appropriate env file based on ENVIRONMENT."""
environment = getenv("ENVIRONMENT", "development")
# Resolve project root from this file
- base_dir = Path(__file__).resolve().parents[3]
+ base_dir = Path(__file__).resolve().parents[3]
# Map env → filename; staging/prod rely on real env vars
mapping = {
"testing": base_dir / ".env.test",
"development": base_dir / ".env",
}
env_file_path = mapping.get(environment)
if env_file_path is not None and not env_file_path.exists():
# Fall back to OS env only if file isn't present
env_file_path = None
return Settings(_env_file=env_file_path)• backend/app/load_env.py
• Add from pathlib import Path; compute base_dir = Path(__file__).resolve().parents[2]
• Replace hard-coded "../.env"/"../.env.test" with absolute paths
• Only call load_dotenv(env_file_path) if the file exists
-from dotenv import load_dotenv
-# Use the same path as config.py expects (one level above backend/)
-env_file = "../.env"
-if env == "testing":
- env_file = "../.env.test"
-load_dotenv(env_file)
+from dotenv import load_dotenv
+from pathlib import Path
+# Resolve project root (load_env.py → backend/app → backend → repo root)
+base_dir = Path(__file__).resolve().parents[2]
+env_files = {
+ "testing": base_dir / ".env.test",
+ "development": base_dir / ".env",
+}
+env_file_path = env_files.get(env, base_dir / ".env")
+if env_file_path.exists():
+ load_dotenv(str(env_file_path))
+else:
+ # Skip file-based loading; rely on OS env vars
+ passOptional (for tests): add a reload hook instead of calling settings.__init__():
def reload_settings() -> Settings:
"""Clear cached settings and rebuild after dotenv changes in tests."""
get_settings.cache_clear()
return get_settings()Note: no occurrences of settings.__init__() were found in the codebase, so test modifications aren’t required at this time.
🤖 Prompt for AI Agents
In backend/app/core/config.py around lines 101-110, wrap get_settings() with
functools.lru_cache and compute an absolute project root using base_dir =
Path(__file__).resolve().parents[3]; map the ENVIRONMENT value to env file paths
base_dir/".env" or base_dir/".env.test", check env_file_path.exists() and set
env_file to None if missing, then return Settings(_env_file=env_file_path or
None). Also update backend/app/load_env.py to import Path, compute base_dir =
Path(__file__).resolve().parents[2], replace relative "../.env" and
"../.env.test" with absolute base_dir paths, and only call
load_dotenv(env_file_path) if the file exists; optionally add a
reload_settings() helper that clears the get_settings cache and returns
get_settings().
* getting rid of unused variables * getting rid of unused variables * loading variables * loading engine * creating more pools * adding example * updating gitignore * cleanup variables * cleanups * cleanups * skipping warning in development and testing * skipping for testing and development * updating default keys * updating default keys * WIP towards using get_setting * updating script * removing LRU cache * renaming variables --------- Co-authored-by: Akhilesh Negi <akhileshnegi@Akhileshs-MacBook-Pro.local>
2 participants
Summary
Target issue is #204
Explain the motivation for making this change. What existing problem does the pull request solve?
WIP
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
This PR introduces the creation of a .env.test file for testing configurations, along with a utility function that loads the test environment variables from this file if it exists. It also adds an .env.test.example file as a reference for developers to easily create their own .env.test files. These changes ensure that testing can be done in an isolated environment without affecting the production settings.
Changes:
Created .env.test File:
Added .env.test.example File:
Utility Function to Load Test Environment Variables:
Created the load_environment function that:
Loads the environment variables from .env.test if the file exists.
If the .env.test file is missing, it prints a warning and uses the default environment settings.
After overriding the .env with .env.test, we call settings.init() to reinitialize the settings object, ensuring that it picks up the new environment variables defined in the test configuration
Updated conftest.py:
Summary by CodeRabbit
New Features
Performance/Reliability
CI
Chores