refactor: separate .env from deployed endpoint env vars#257
refactor: separate .env from deployed endpoint env vars#257
Conversation
5b765f6 to
be4269a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/runpod_flash/cli/utils/env_preview.py:1
- The
_SECRET_PATTERNregex matches any key containingTOKENas a substring, soTOKENIZER_PATHgets masked even though it's not a secret. This is a false positive — environment variables likeTOKENIZER_PATH,TOKEN_COUNT, orBUCKET_NAME(no match, but illustrative) could be misleadingly masked. The test at line 52-55 intest_env_preview.pyconfirms this behavior as intended, but the docstring says "TOKEN in TOKENIZER should still match" which suggests this was a deliberate choice. However, this will surprise users who have non-secret vars with these substrings. Consider using word-boundary matching (e.g.,r"\b(KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL)\b"orr"(^|_)(KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL)($|_)") to avoid masking keys likeTOKENIZER_PATHorTOKENIZER_CONFIG.
"""Deploy-time env preview: show what env vars go to each endpoint."""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
bab2b46 to
6782404
Compare
There was a problem hiding this comment.
Review
Clean, well-motivated change. The core fix (env defaults to None instead of implicitly loading .env) removes a real footgun. Good test coverage in test_env_separation.py. One confirmed bug and two nits below.
Bug: preview/deploy parity broken — makes_remote_calls default mismatch
_do_deploy calls _check_makes_remote_calls() which defaults to True in all failure cases:
# serverless.py:583
makes_remote_calls = resource_config.get("makes_remote_calls", True)
# also: "Manifest not found, assuming makes_remote_calls=True"
# also: "Resource not in manifest, assuming makes_remote_calls=True"collect_env_for_preview uses the opposite default:
# env_preview.py
config.get("makes_remote_calls", False) # ← should be TrueOn first deploy (no prior build/manifest), the preview shows no RUNPOD_API_KEY for QB endpoints but deploy injects it. The preview is wrong exactly when it matters most — the first time a user runs flash deploy.
Fix: config.get("makes_remote_calls", True) in env_preview.py to match deploy behaviour.
Nit: env={} explicit empty dict won't appear in manifest
manifest.py uses:
if hasattr(resource_config, "env") and resource_config.env:An explicit env={} is falsy so it's silently skipped. Probably the right behaviour (nothing to store), but test_env_separation.py::test_serverless_resource_env_explicit_empty_dict_preserved only checks the resource object — not the manifest path. Worth a test or a comment to make the intent explicit.
Nit: _MASK_VISIBLE_CHARS = 6 exposes key type prefix
RUNPOD_API_KEY values start with rp_ — 6 visible chars reveals the full type prefix (rp_xxx). Not a security issue since the prefix is public, but worth being intentional about if multiple key formats are ever supported.
Positives
- Removing
env_dict.pop("RUNPOD_API_KEY", None)inmanifest.pyis correct — user-explicit keys should be preserved, not silently stripped except Exception: logger.debug(...)aroundrender_env_previewindeploy.pyis the right call — preview failure must not block deploytest_env_separation.pycovers the_configure_existing_templateNonecase, which was the trickiest correctness question- Companion flash-examples PR (#39) is the right process for a breaking change
Verdict: PASS with fix — the makes_remote_calls default is a one-line fix but should land before merge to avoid shipping a misleading preview.
🤖 Reviewed by Henrik's AI-Powered Bug Finder
Change ServerlessResource.env default from get_env_vars() (reads .env)
to None. Delete EnvironmentVars class and get_env_vars(). Template
creation now uses self.env or {} instead of falling back to .env file.
.env still populates os.environ via load_dotenv() in __init__.py for
CLI and get_api_key() usage. This change only affects what gets sent
to deployed endpoints.
Replace TestServerlessResourceEnvLoading tests that imported deleted get_env_vars/EnvironmentVars with tests that verify env defaults to None and preserves explicit dicts.
With env separation, resource.env only contains user-declared vars. If a user explicitly sets RUNPOD_API_KEY in their resource env, it should be preserved. Runtime injection via _inject_runtime_template_vars() handles the automatic case.
New module renders a Rich table per resource showing all env vars that will be sent to deployed endpoints. User-declared vars shown directly; flash-injected vars (RUNPOD_API_KEY, FLASH_MODULE_PATH) labeled as 'injected by flash'. Secret values masked based on key pattern matching (KEY, TOKEN, SECRET, PASSWORD, CREDENTIAL). Wired into flash deploy: renders before provisioning so users see exactly what goes to each endpoint.
Clarify that .env populates os.environ for CLI/local dev only.
Resource env vars must be explicit via env={} on resource config.
RUNPOD_API_KEY injected automatically for makes_remote_calls=True.
- Fix preview/deploy mismatch: LB endpoints no longer show RUNPOD_API_KEY injection in preview (matches _do_deploy behavior) - Wrap render_env_preview in try-except so preview failures don't abort deployment - Fix stale comment referencing .env file in serverless_cpu.py - Correct drift detection doc: env is conditionally included in hash, not always excluded - Fix LB architecture doc: LB endpoints get FLASH_MODULE_PATH, not RUNPOD_API_KEY - Update config_hash docstring to reflect exclude_none behavior - Add tests for render_env_preview, LB API key exclusion, and _configure_existing_template with env=None
- Compact env preview into single table with resource column to reduce terminal clutter - Show "user" source label for user-declared vars instead of empty string - Remove local filesystem paths from plan docs - Fix design doc: env=None omits the key, not stores empty dict - Update render tests for new table format
6782404 to
d88584e
Compare
Summary
.envfile contents to deployed endpointsServerlessResource.envdefaults toNoneinstead of reading.envviaget_env_vars()EnvironmentVarsclass andenvironment.pyRUNPOD_API_KEYfrom explicit resource env in manifestMotivation
.envfiles were implicitly dumped into every deployed endpoint, causing:PORT,PORT_HEALTH) overwritten on template updatesself.envChanges
bd92886environment.py, removeget_env_vars(), changeenvdefault toNone50ffc7ed6545f7RUNPOD_API_KEYfrom explicit resource env in manifest4d24808flash deploy5b765f6How it works now
.envstill populatesos.environviaload_dotenv()for CLI andget_api_key()usageenv={"HF_TOKEN": os.environ["HF_TOKEN"]}RUNPOD_API_KEY,FLASH_MODULE_PATH) intotemplate.envautomaticallyflash deployrenders a preview table showing all env vars per resource before provisioningBreaking change
ServerlessResource.envno longer defaults to.envfile contents. Users relying on implicit carryover must add explicitenv={}to their resource configs.Companion PR
Test plan
EnvironmentVarsorget_env_vars()flash deploywith explicitenv={}on resourceflash deploywith noenv(verify preview shows only flash-injected vars)