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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR decouples two dump warnings by adding a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review: Decouple dump warning flags for env-label fallbackThe core change is correct and well-scoped. The one-line fix in Things that look good
Minor issues (see inline comments)
None of these are blockers, but items 1 and 3 are easy fixes before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.integration.test.js (1)
490-560: Optional: extract shared build-fixture setup to reduce repetition.The duplicated temp config/build YAML setup across these tests could be moved to a small helper for easier maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.integration.test.js` around lines 490 - 560, Multiple tests (the "dump --build warns when metadata environment label falls back to build NODE_ENV", "dump --build --no-warn-sensitive keeps NODE_ENV label note", and "dump --build --no-warn-env-label suppresses NODE_ENV label note") repeat the same temp file setup using configPath, buildConfig and fs.writeFileSync; extract that repeated logic into a small helper (e.g., createBuildFixture or writeBuildBuildConfig) that takes tempDir, webpack contents and environment values and returns the configPath and buildConfig, then replace the repeated fs.writeFileSync/variable assignments in each test and use the helper before calling run and checking errorSpy. Ensure the helper is imported/defined near the top of test/cli.integration.test.js and referenced by the existing test functions (run, configPath, buildConfig, errorSpy remain unchanged in assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/cli.integration.test.js`:
- Around line 490-560: Multiple tests (the "dump --build warns when metadata
environment label falls back to build NODE_ENV", "dump --build
--no-warn-sensitive keeps NODE_ENV label note", and "dump --build
--no-warn-env-label suppresses NODE_ENV label note") repeat the same temp file
setup using configPath, buildConfig and fs.writeFileSync; extract that repeated
logic into a small helper (e.g., createBuildFixture or writeBuildBuildConfig)
that takes tempDir, webpack contents and environment values and returns the
configPath and buildConfig, then replace the repeated fs.writeFileSync/variable
assignments in each test and use the helper before calling run and checking
errorSpy. Ensure the helper is imported/defined near the top of
test/cli.integration.test.js and referenced by the existing test functions (run,
configPath, buildConfig, errorSpy remain unchanged in assertions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ba635a2-92e8-4062-a983-9926fda6d271
📒 Files selected for processing (5)
CHANGELOG.mdREADME.mddocs/cli-reference.mdsrc/cli.tstest/cli.integration.test.js
- Add test verifying --no-warn-sensitive and --no-warn-env-label work independently when used together - Clarify in help text that --no-warn-env-label only applies in build-matrix mode and is ignored for single-file dumps Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| ### Changed | ||
|
|
||
| - Decoupled dump warning controls: `--no-warn-sensitive` now only suppresses sensitive-output warnings, and new `--no-warn-env-label` suppresses build-matrix `NODE_ENV` environment-label notes. [Issue #23](https://github.com/shakacode/pack-config-diff/issues/23) |
There was a problem hiding this comment.
The behavioral change to --no-warn-sensitive is a breaking change for any existing automation that relied on the old coupling (using --no-warn-sensitive in build-matrix mode previously suppressed the NODE_ENV label note too). The current entry only describes the new semantics without warning existing users about the migration impact.
Consider calling this out more explicitly, e.g.:
| - Decoupled dump warning controls: `--no-warn-sensitive` now only suppresses sensitive-output warnings, and new `--no-warn-env-label` suppresses build-matrix `NODE_ENV` environment-label notes. [Issue #23](https://github.com/shakacode/pack-config-diff/issues/23) | |
| - **Breaking:** `--no-warn-sensitive` now only suppresses sensitive-output warnings; it no longer silences the build-matrix `NODE_ENV` environment-label note. Migrate any scripts using `--no-warn-sensitive` to suppress both by adding `--no-warn-env-label`. New `--no-warn-env-label` flag suppresses only the environment-label note independently. [Issue #23](https://github.com/shakacode/pack-config-diff/issues/23) |
| const resolvedEnvironment = resolveBuildEnvironmentLabel(build, parsed); | ||
| const envLabel = resolvedEnvironment.label; | ||
| if (resolvedEnvironment.source === "build-node-env" && parsed.warnSensitive) { | ||
| if (resolvedEnvironment.source === "build-node-env" && parsed.warnEnvLabel) { |
There was a problem hiding this comment.
This line is the core behavioral change and warrants a comment explaining the intentional decoupling, especially since the old gate used warnSensitive here. Future readers seeing this alongside the warnSensitive check at line 865 may wonder whether the different field is a bug or intentional.
| if (resolvedEnvironment.source === "build-node-env" && parsed.warnEnvLabel) { | |
| if (resolvedEnvironment.source === "build-node-env" && parsed.warnEnvLabel) { | |
| // Intentionally uses warnEnvLabel (not warnSensitive) — these warnings are independent. | |
| // --no-warn-env-label suppresses only this note; --no-warn-sensitive suppresses only | |
| // the sensitive-output warning shown when --clean is omitted. |
| @@ -504,33 +504,109 @@ describe("CLI integration", () => { | |||
| "utf8", | |||
| ); | |||
There was a problem hiding this comment.
The same configPath/buildConfig setup block (lines 491–505 here) is copy-pasted verbatim in all four new tests. Since tempDir is already shared via beforeEach, this can be pulled into its own beforeEach block (or a helper) within a nested describe grouping for these build-matrix warning tests.
Duplication isn't a correctness bug, but it adds friction when the fixture needs to change — currently you'd have to update it in four places.
| --env=<KEY=VALUE> Set env var before loading config (repeatable) | ||
| --clean Strip plugin internals and compact functions before dump (recommended for secrets safety) | ||
| --no-warn-sensitive Suppress the sensitive-output warning when running dump without --clean | ||
| --no-warn-env-label Suppress the build NODE_ENV environment-label note (only applies in build-matrix mode; ignored for single-file dump) |
There was a problem hiding this comment.
The help text documents that --no-warn-env-label is "ignored for single-file dump", but there's no test covering that case and no user-visible feedback when the flag is silently ignored. A test asserting exit code 0 with a note in stderr (or simply asserting no crash) would confirm the documented behaviour and prevent silent regressions.
Code ReviewThe decoupling approach is clean and the implementation is correct. Four observations: Breaking change needs clearer signposting (CHANGELOG.md:17) Clarifying comment missing at the decoupling point (src/cli.ts:750) Test fixture duplicated four times (test/cli.integration.test.js:491-505) No test for the documented single-file no-op (src/cli.ts:115) Overall the feature is well-scoped and the independent-flag test coverage is good. Main ask is making the breaking behaviour change explicit for upgraders. |
1 participant
Summary
--no-warn-sensitiveonly suppresses sensitive-output warnings.--no-warn-env-labelto suppress the build-matrix NODE_ENV fallback environment-label note.Closes #23.
Testing
Note
Low Risk
Low risk: changes are limited to CLI warning/flag behavior in
dump(new flag and adjusted gating), with updated docs and integration tests; main functional impact is on stderr output for build-matrix runs.Overview
Decouples
dumpwarning controls so--no-warn-sensitivesuppresses only the sensitive-output warning, and introduces--no-warn-env-labelto independently silence the build-matrix note when the environment label falls back tobuild.environment.NODE_ENV.Updates CLI help text, README/CLI reference, changelog, and expands integration tests to verify the two warnings can be enabled/suppressed independently.
Written by Cursor Bugbot for commit f0d0c0f. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
--no-warn-env-labelflag to suppress environment-label warnings in dump output.Changed
--no-warn-sensitivenow suppresses only sensitive-output warnings; environment-label notes are controlled separately.Documentation
--no-warn-env-labeldocs and examples.Tests