feat(compose): accept a comma-separated service list in --service#13705
Open
czubocha wants to merge 1 commit into
Open
feat(compose): accept a comma-separated service list in --service#13705czubocha wants to merge 1 commit into
czubocha wants to merge 1 commit into
Conversation
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughCompose adds comma-separated ChangesCompose subset execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComposeCLI
participant runCompose
participant Compose
participant StateResolver
ComposeCLI->>runCompose: parse --service=a,b
runCompose->>Compose: executeSubsetComponents
Compose->>StateResolver: read dependency closure
StateResolver-->>Compose: return service state
Compose->>Compose: execute selected services in dependency order
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sf-core/src/lib/runners/compose/index.js (1)
322-357: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
=== undefinedfor resolved outputs
if (!stateValue)treats an empty-string CloudFormation output as missing, so this can throw the new "has no output" / "no deployed state" errors for a value that did resolve.🤖 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 `@packages/sf-core/src/lib/runners/compose/index.js` around lines 322 - 357, Update the stateValue presence check in the parameter-resolution flow to use an explicit undefined comparison instead of truthiness. Preserve empty-string and other valid resolved output values, while retaining the existing missing-output and missing-service error handling for genuinely undefined results.
🧹 Nitpick comments (1)
packages/sf-core/tests/integration/compose-dev/compose-dev.test.js (1)
38-43: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider retrying on all non-200 responses to prevent flakiness.
While a cold local-dev proxy might return 5xx errors before the shim is fully wired, newly deployed API Gateway endpoints can also briefly return
403 Forbiddenor404 Not Foundwhile routes are propagating. Retrying on any non-200 status will make the test more resilient against these transient API Gateway states.♻️ Proposed refactor
- // A cold local-dev proxy can briefly answer 5xx before the shim is wired. - if (response.status >= 500) { + // A cold local-dev proxy can briefly answer 5xx before the shim is wired, + // and API Gateway can briefly answer 403/404 during route propagation. + if (!response.ok) { lastErr = new Error(`endpoint returned ${response.status}`) } else { return response🤖 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 `@packages/sf-core/tests/integration/compose-dev/compose-dev.test.js` around lines 38 - 43, Update the response handling in the compose-dev retry loop to retry on every non-200 HTTP status, not only statuses 500 and above. Preserve returning the response immediately when its status is 200, while recording an error containing the returned status for all other responses.
🤖 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.
Inline comments:
In `@packages/sf-core/src/lib/runners/compose/compose.js`:
- Around line 246-259: Update the compose service-selection flow around
serviceNames and executeComponentsGraph to reject truthy options.service values
that parse to no service names, including whitespace or comma-only values,
before any command execution. Ensure interactive or single-service commands
cannot fall through to whole-graph execution, and remove options.service before
invoking executeComponentsGraph so malformed raw values are not passed
downstream.
In `@packages/sf-core/tests/integration/compose-dev/compose-dev.test.js`:
- Around line 81-87: Update the beforeAll environment setup at
packages/sf-core/tests/integration/compose-dev/compose-dev.test.js#L81-L87 to
mutate process.env directly, conditionally set SERVERLESS_LICENSE_KEY only when
its dev value exists, and delete SERVERLESS_ACCESS_KEY instead of assigning
undefined. Update afterAll at
packages/sf-core/tests/integration/compose-dev/compose-dev.test.js#L126 to clear
current environment keys and restore originalEnv with Object.assign rather than
reassigning process.env.
---
Outside diff comments:
In `@packages/sf-core/src/lib/runners/compose/index.js`:
- Around line 322-357: Update the stateValue presence check in the
parameter-resolution flow to use an explicit undefined comparison instead of
truthiness. Preserve empty-string and other valid resolved output values, while
retaining the existing missing-output and missing-service error handling for
genuinely undefined results.
---
Nitpick comments:
In `@packages/sf-core/tests/integration/compose-dev/compose-dev.test.js`:
- Around line 38-43: Update the response handling in the compose-dev retry loop
to retry on every non-200 HTTP status, not only statuses 500 and above. Preserve
returning the response immediately when its status is 200, while recording an
error containing the returned status for all other responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6a015a5-8126-4e6e-940d-5d3d790a5113
📒 Files selected for processing (22)
docs/sf/guides/compose.mdpackages/sf-core/package.jsonpackages/sf-core/src/lib/runners/compose/compose.jspackages/sf-core/src/lib/runners/compose/help.jspackages/sf-core/src/lib/runners/compose/index.jspackages/sf-core/src/lib/runners/compose/state.jspackages/sf-core/tests/integration/compose-dev/compose-dev.test.jspackages/sf-core/tests/integration/compose-dev/fixture/api/handler.jspackages/sf-core/tests/integration/compose-dev/fixture/api/serverless.ymlpackages/sf-core/tests/integration/compose-dev/fixture/localdep/serverless.ymlpackages/sf-core/tests/integration/compose-dev/fixture/serverless-compose.ymlpackages/sf-core/tests/integration/compose-dev/fixture/shared/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/compose-service-subset.test.jspackages/sf-core/tests/integration/compose-service-subset/fixture/api/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/db/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/middle/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/serverless-compose.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/worker/serverless.ymlpackages/sf-core/tests/unit/lib/runners/compose/execute-subset.test.jspackages/sf-core/tests/unit/lib/runners/compose/service-option.test.jspackages/sf-core/tests/unit/lib/runners/compose/state.test.jspackages/sf-core/tests/unit/lib/runners/compose/teaching-errors.test.js
4678a5d to
8d6eb0a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In
`@packages/sf-core/tests/integration/compose-service-subset/compose-service-subset.test.js`:
- Around line 145-160: Hoist the freshStage value from the deploy test to the
surrounding test scope so it is available during teardown. Update afterAll to
delete the API stack associated with freshStage in addition to the existing
stage stack, ensuring cleanup runs even when the expected failure assertion
regresses.
- Line 18: Update both suites in
packages/sf-core/tests/integration/compose-service-subset/compose-service-subset.test.js:
at lines 18 and 190, snapshot process.env by value using a spread copy; at lines
40-49 and 202-211, mutate environment keys in place, delete the access key as
needed, and restore the original values with Object.assign instead of replacing
process.env.
- Around line 28-34: Update stackExists to return false only for the expected
CloudFormation ValidationError indicating the stack does not exist, and rethrow
all other DescribeStacks failures. In both teardown sites at lines 52-69 and
212-229, apply the same missing-stack-only filtering so delete and waiter errors
propagate instead of being suppressed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d823cb3-ce97-46d4-b6a8-236bf2f9b327
📒 Files selected for processing (22)
docs/sf/guides/compose.mdpackages/sf-core/package.jsonpackages/sf-core/src/lib/runners/compose/compose.jspackages/sf-core/src/lib/runners/compose/help.jspackages/sf-core/src/lib/runners/compose/index.jspackages/sf-core/src/lib/runners/compose/state.jspackages/sf-core/tests/integration/compose-dev/compose-dev.test.jspackages/sf-core/tests/integration/compose-dev/fixture/api/handler.jspackages/sf-core/tests/integration/compose-dev/fixture/api/serverless.ymlpackages/sf-core/tests/integration/compose-dev/fixture/localdep/serverless.ymlpackages/sf-core/tests/integration/compose-dev/fixture/serverless-compose.ymlpackages/sf-core/tests/integration/compose-dev/fixture/shared/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/compose-service-subset.test.jspackages/sf-core/tests/integration/compose-service-subset/fixture/api/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/db/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/middle/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/serverless-compose.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/worker/serverless.ymlpackages/sf-core/tests/unit/lib/runners/compose/execute-subset.test.jspackages/sf-core/tests/unit/lib/runners/compose/service-option.test.jspackages/sf-core/tests/unit/lib/runners/compose/state.test.jspackages/sf-core/tests/unit/lib/runners/compose/teaching-errors.test.js
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/sf-core/tests/integration/compose-dev/fixture/api/handler.js
- packages/sf-core/tests/unit/lib/runners/compose/service-option.test.js
- packages/sf-core/tests/integration/compose-service-subset/fixture/api/serverless.yml
- packages/sf-core/tests/integration/compose-dev/fixture/api/serverless.yml
- packages/sf-core/tests/integration/compose-service-subset/fixture/serverless-compose.yml
- packages/sf-core/tests/integration/compose-dev/fixture/serverless-compose.yml
- packages/sf-core/src/lib/runners/compose/state.js
- packages/sf-core/src/lib/runners/compose/help.js
- packages/sf-core/tests/unit/lib/runners/compose/state.test.js
- docs/sf/guides/compose.md
- packages/sf-core/tests/unit/lib/runners/compose/execute-subset.test.js
- packages/sf-core/src/lib/runners/compose/compose.js
- packages/sf-core/src/lib/runners/compose/index.js
8d6eb0a to
f1cb5a8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sf-core/tests/integration/compose-service-subset/fixture/db/serverless.yml (1)
7-8: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUnencrypted SNS topic (test fixture only).
Checkov flags this topic as unencrypted (CKV_AWS_26). Low real-world risk since this is a disposable integration-test fixture, but adding
KmsMasterKeyIdwould silence the finding if it matters for policy compliance.🤖 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 `@packages/sf-core/tests/integration/compose-service-subset/fixture/db/serverless.yml` around lines 7 - 8, Update the AWS::SNS::Topic resource in the serverless fixture to include a KmsMasterKeyId encryption configuration, using the project’s established KMS key reference if available, so Checkov rule CKV_AWS_26 is satisfied while preserving the fixture’s behavior.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@packages/sf-core/src/lib/runners/compose/index.js`:
- Around line 352-356: Update the refresh command in the ServerlessError message
within the compose runner to append the existing stageFlag, matching the
stage-specific deploy hint. Preserve the current depService info command and all
other error text.
In `@packages/sf-core/tests/integration/compose-dev/compose-dev.test.js`:
- Around line 69-95: Move the originalCwd initialization in beforeAll above the
shim existence check so it is set even when the check throws. Keep afterAll’s
process.chdir(originalCwd) behavior unchanged and preserve the existing shim
validation.
---
Nitpick comments:
In
`@packages/sf-core/tests/integration/compose-service-subset/fixture/db/serverless.yml`:
- Around line 7-8: Update the AWS::SNS::Topic resource in the serverless fixture
to include a KmsMasterKeyId encryption configuration, using the project’s
established KMS key reference if available, so Checkov rule CKV_AWS_26 is
satisfied while preserving the fixture’s behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 064db6f7-9b81-4152-a7d6-c7c360a6c0bb
📒 Files selected for processing (22)
docs/sf/guides/compose.mdpackages/sf-core/package.jsonpackages/sf-core/src/lib/runners/compose/compose.jspackages/sf-core/src/lib/runners/compose/help.jspackages/sf-core/src/lib/runners/compose/index.jspackages/sf-core/src/lib/runners/compose/state.jspackages/sf-core/tests/integration/compose-dev/compose-dev.test.jspackages/sf-core/tests/integration/compose-dev/fixture/api/handler.jspackages/sf-core/tests/integration/compose-dev/fixture/api/serverless.ymlpackages/sf-core/tests/integration/compose-dev/fixture/localdep/serverless.ymlpackages/sf-core/tests/integration/compose-dev/fixture/serverless-compose.ymlpackages/sf-core/tests/integration/compose-dev/fixture/shared/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/compose-service-subset.test.jspackages/sf-core/tests/integration/compose-service-subset/fixture/api/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/db/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/middle/serverless.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/serverless-compose.ymlpackages/sf-core/tests/integration/compose-service-subset/fixture/worker/serverless.ymlpackages/sf-core/tests/unit/lib/runners/compose/execute-subset.test.jspackages/sf-core/tests/unit/lib/runners/compose/service-option.test.jspackages/sf-core/tests/unit/lib/runners/compose/state.test.jspackages/sf-core/tests/unit/lib/runners/compose/teaching-errors.test.js
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/sf-core/src/lib/runners/compose/state.js
- packages/sf-core/tests/integration/compose-dev/fixture/api/handler.js
- docs/sf/guides/compose.md
- packages/sf-core/src/lib/runners/compose/help.js
- packages/sf-core/tests/integration/compose-service-subset/fixture/api/serverless.yml
- packages/sf-core/tests/integration/compose-service-subset/fixture/serverless-compose.yml
- packages/sf-core/tests/unit/lib/runners/compose/service-option.test.js
- packages/sf-core/tests/integration/compose-dev/fixture/serverless-compose.yml
- packages/sf-core/package.json
- packages/sf-core/tests/unit/lib/runners/compose/state.test.js
- packages/sf-core/tests/unit/lib/runners/compose/teaching-errors.test.js
- packages/sf-core/src/lib/runners/compose/compose.js
- packages/sf-core/tests/unit/lib/runners/compose/execute-subset.test.js
`--service` now accepts a comma-separated list, so a project-wide command
runs on an exact subset of services:
serverless deploy --service=service-a,service-d --stage my-feature
- Runs on exactly the named services, ordered among themselves by `dependsOn`.
- Services not named are left untouched — neither deployed nor removed. Their
outputs remain available for `${param:xxx}` resolution, so a subset can
reference services already deployed elsewhere.
- Supported for deploy, remove, info, print, and package.
- Adds clearer guidance for single-service-only commands (e.g. `dev`) and
unresolved parameters.
Docs updated in guides/compose.md. Covered by unit and live integration tests.
f1cb5a8 to
2e17519
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--servicenow accepts a comma-separated list, so a project-wide command runs on an exact subset of services:dependsOn.${param:xxx}resolution, so a subset can reference services already deployed elsewhere.deploy,remove,info,print, andpackage.dev) and unresolved parameters.Docs
Updated
docs/sf/guides/compose.mdwith a "Running a command on a subset of services" section.Tests
execute-subset,service-option,teaching-errors,state.compose-service-subset(exact-set semantics, dependency ordering, first-run exit code),compose-dev(single-service dev with graph/hoisted params).Summary by CodeRabbit
--servicevalues (applies todeploy,remove,info,print, andpackage).packagecommand and document the global--serviceoption.--service, and missing param/output resolution, including clearer stage guidance.