docs: clarify .env scope for env separation#39
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
docs/cli/troubleshooting.md:694
- The numbering in this Solutions list is now duplicated (two items labeled "4."). After adding the new recommended
flash loginstep, renumber the remaining items so the list stays sequential.
**4. Get API key:**
1. Visit https://runpod.io/console/user/settings
2. Click "API Keys"
3. Create new key or copy existing
4. Set environment variable
**4. Make persistent (bash/zsh):**
```bash
echo 'export RUNPOD_API_KEY=your-key-here' >> ~/.bashrc
💡 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.
7508b5a to
0b375c5
Compare
Update all documentation to reflect env separation:
- .env populates os.environ for CLI and local dev
- Resource env={} is the explicit way to set endpoint env vars
- flash login is the primary auth method
- Deploy-time preview shows what goes to each endpoint
- Use os.getenv() instead of os.environ[] in doc examples - Show both uv run and bare flash login invocations - Split dense paragraph into scannable bullets in CONTRIBUTING.md
runpod-Henrik
left a comment
There was a problem hiding this comment.
Nit: Datacenter and network volume files are noise in this diff
04_scaling_performance/02_datacenters/ (3 new files) and the 05_data_workflows/01_network_volumes/ changes appear in the diff but are already in main from PR #44 (merged 2026-03-17). They will be no-ops when this PR merges. No action needed.
Verdict
PASS. The .env scope clarification and flash login promotion are accurate and well-placed across the six doc files.
|
seems like this duplicates #44 ? |
311251e to
fb32778
Compare
runpod-Henrik
left a comment
There was a problem hiding this comment.
1. Prior nit resolved
The datacenter and network volume files that were noise in the previous diff are gone. The PR is now cleanly scoped to 6 doc files only.
2. Core doc changes — correct and well-placed
All six files consistently add the same two clarifications:
.envpopulatesos.environfor local CLI use — it is not carried to deployed endpoints- To pass env vars at deploy time, declare them on the resource config
The troubleshooting.md reordering (promoting flash login to solution 1 for both "API key not set" and "API key expired") is the right call given the env separation change — users should no longer be relying on .env as the primary auth path.
Issue: inconsistent env access pattern across examples
CLI-REFERENCE.md uses:
env={"KEY": os.environ["KEY"]}Every other file (commands.md, workflows.md, CONTRIBUTING.md) uses:
env={"KEY": os.getenv("KEY")}os.environ["KEY"] raises KeyError if the variable isn't set — the worker deploy fails loudly at startup. os.getenv("KEY") returns None silently — the value gets passed as None to the deployed endpoint, which may fail in a harder-to-debug way. Neither is ideal. The safest pattern for docs is:
env={"KEY": os.environ["KEY"]} # raises KeyError early — user knows immediatelyor with a guard:
env={"HF_TOKEN": os.getenv("HF_TOKEN") or ""} # explicit, no surprise NoneAt minimum, the examples should be consistent. Recommend standardising on os.environ["KEY"] — the loud failure is more user-friendly than a silently-passed None.
Nits
CONTRIBUTING.mdadds "For authentication, prefer usingflash login." as a bullet point under the env var guidance. Reads slightly awkward as a bullet — could be a separate note or a callout block matching the style used in the other files.workflows.mdstep 3 is renamed from "Configure Environment Variables" to "Authenticate" — good rename, but the body still shows the.envfile example before theflash loginexample. The recommended path (login) should come first to match the new heading intent.
Verdict
PASS WITH NITS — the documentation changes are accurate and the scope is tight. The os.environ vs os.getenv inconsistency is the only item worth fixing before merge; the rest is style.
🤖 Reviewed by Henrik's AI-Powered Bug Finder
|
Addressed Henrik's review nits:
|
- Use os.environ["KEY"] consistently across all files (loud KeyError on missing vars is better UX than silently passing None via getenv) - Convert CONTRIBUTING.md bullets to blockquote callout - Fix duplicate step numbering in troubleshooting.md
runpod-Henrik
left a comment
There was a problem hiding this comment.
Delta review — since 2026-03-19
1. Issue resolved — os.environ vs os.getenv inconsistency
The third commit (063e0d31) standardises on os.environ["KEY"] across all six files. CLI-REFERENCE.md, commands.md, workflows.md, CONTRIBUTING.md all now use the same pattern. The inconsistency flagged in the prior review is gone.
2. Nit resolved — CONTRIBUTING.md bullet awkwardness
The "For authentication, prefer using flash login." bullet has been converted to a blockquote callout, consistent with the style used in the other files.
3. Nit resolved — workflows.md step ordering
Step 3 ("Authenticate") now leads with flash login before the .env alternative. Matches the heading intent.
4. Bonus fix — duplicate step numbering in troubleshooting.md
Not flagged in prior reviews but the commit also fixes a numbering gap in troubleshooting.md (steps 1–5 now sequential, and both "API key not set" and "API key expired" sections now correctly promote flash login to position 1). Clean.
Verdict
PASS — all prior findings addressed, no new issues. The docs are now consistent on os.environ["KEY"], callout style, step ordering, and flash login primacy.
🤖 Reviewed by Henrik's AI-Powered Bug Finder
Summary
.envfiles populateos.environfor CLI and local dev only, not deployed endpointsflash loginas primary auth and explicitenv={}for deploy-time varsdeanq/ae-1549-explicit-env-vars)Files Changed
CONTRIBUTING.md.envis for local CLI use onlyCLI-REFERENCE.md.envsection scopedocs/cli/commands.mdenv={}guidancedocs/cli/getting-started.mdflash login, clarify.envscopedocs/cli/troubleshooting.mdflash loginas primary fix, clarify.envscopedocs/cli/workflows.mdflash loginprimary,.envalternativeTest plan