feat: resolve ${env('...')} variables in cloud build yaml before parsing#155
feat: resolve ${env('...')} variables in cloud build yaml before parsing#155mozhou52 wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds environment variable substitution to the build YAML loading process. A new ChangesEnvironment Variable Resolution
Sequence DiagramsequenceDiagram
participant BuilderFactory
participant resolveEnvVariables
participant yaml
participant process.env
BuilderFactory->>BuilderFactory: read fc3-cloud-build.yaml content
BuilderFactory->>resolveEnvVariables: pass content
resolveEnvVariables->>process.env: lookup env variables
resolveEnvVariables-->>BuilderFactory: return resolved content
BuilderFactory->>yaml: parse resolved content
yaml-->>BuilderFactory: return parsed config
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/ut/commands/build_test.ts (1)
306-341: ⚡ Quick winIsolate
process.envmutations per test to avoid leakage.Line 308 onward mutates global env and cleans up inline; if a test fails before cleanup, later tests can become flaky. Snapshot/restore env in hooks for deterministic isolation.
Suggested patch
describe('BuilderFactory', () => { let mockInputs: IInputs; + let originalEnv: NodeJS.ProcessEnv; beforeEach(() => { + originalEnv = { ...process.env }; mockInputs = { cwd: '/test', @@ afterEach(() => { + process.env = originalEnv; jest.clearAllMocks(); }); @@ describe('resolveEnvVariables', () => { it('should replace env variables with single quotes', () => { process.env.MY_VAR = 'hello'; const result = resolveEnvVariables('value: ${env(\'MY_VAR\')}'); expect(result).toBe('value: hello'); - delete process.env.MY_VAR; }); @@ it('should replace env variables with double quotes', () => { process.env.MY_VAR = 'world'; const result = resolveEnvVariables('value: ${env("MY_VAR")}'); expect(result).toBe('value: world'); - delete process.env.MY_VAR; }); @@ it('should replace multiple env variables', () => { process.env.USERNAME = 'admin'; process.env.PASSWORD = 'secret'; const result = resolveEnvVariables('username: ${env(\'USERNAME\')}\npassword: ${env(\'PASSWORD\')}'); expect(result).toBe('username: admin\npassword: secret'); - delete process.env.USERNAME; - delete process.env.PASSWORD; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/ut/commands/build_test.ts` around lines 306 - 341, The tests in the resolveEnvVariables suite mutate process.env directly which can leak between tests; wrap the describe('resolveEnvVariables') block with a beforeEach that saves a shallow copy of process.env (e.g., const ORIGINAL_ENV = { ...process.env }) and an afterEach that restores it (process.env = ORIGINAL_ENV) so each test runs with an isolated env; update the tests that currently set/delete env vars to rely on this snapshot/restore and remove inline cleanup to avoid flaky failures if a test throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@__tests__/ut/commands/build_test.ts`:
- Around line 306-341: The tests in the resolveEnvVariables suite mutate
process.env directly which can leak between tests; wrap the
describe('resolveEnvVariables') block with a beforeEach that saves a shallow
copy of process.env (e.g., const ORIGINAL_ENV = { ...process.env }) and an
afterEach that restores it (process.env = ORIGINAL_ENV) so each test runs with
an isolated env; update the tests that currently set/delete env vars to rely on
this snapshot/restore and remove inline cleanup to avoid flaky failures if a
test throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8069c1c1-dc96-41a1-9345-b790e3ae3eff
📒 Files selected for processing (2)
__tests__/ut/commands/build_test.tssrc/subCommands/build/index.ts
1 participant
Summary by CodeRabbit
${env('VAR')}syntax. Variables are automatically resolved during the build process, with error handling for missing variables.