Skip to content

fix(sdl): keep legacy builder placements independent per service - #3292

Merged
ygrishajev merged 1 commit into
mainfrom
fix/sdl-per-service-placement
Jun 10, 2026
Merged

fix(sdl): keep legacy builder placements independent per service#3292
ygrishajev merged 1 commit into
mainfrom
fix/sdl-per-service-placement

Conversation

@ygrishajev

@ygrishajev ygrishajev commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Why

The legacy SDL builder edits a placement per service, but adding a service reused the first placement and importing deduped placements by name. As a result all services ended up sharing one placement record — renaming one renamed them all.

What

  • Give each added service its own placement, and drop a service's placement on removal when nothing else references it.
  • Add a placementPerService option to importSimpleSdl (off by default, preserving the shared placement-first import) that the legacy builder entry points opt into, so a service keeps its own placement across YAML round-trips.
  • SDL output is unchanged — generation still groups placements by name.

Summary by CodeRabbit

Release Notes

  • Refactor

    • Updated deployment import and service management to assign placements more consistently across services during SDL import, template loading, and service addition/removal operations.
  • Tests

    • Added test coverage for placement behavior when adding and removing services.

The legacy SDL builder edits a placement per service, but adding a service
reused the first placement and importing deduped placements by name, so all
services ended up sharing one placement record and renaming one renamed all.

Give each added service its own placement and drop its placement on removal
when nothing else references it. Add a placementPerService option to
importSimpleSdl (off by default, preserving the shared placement-first import)
that the legacy builder entry points opt into, so a service keeps its own
placement across YAML round-trips. SDL output is unchanged since generation
still groups placements by name.
@ygrishajev
ygrishajev requested a review from a team as a code owner June 10, 2026 09:48
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 835660b2-86a5-4df6-afdf-eb764042fec3

📥 Commits

Reviewing files that changed from the base of the PR and between eadb218 and 824e239.

📒 Files selected for processing (8)
  • apps/deploy-web/src/components/new-deployment/SdlBuilder.tsx
  • apps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsx
  • apps/deploy-web/src/components/sdl/ImportSdlModal.tsx
  • apps/deploy-web/src/components/sdl/SimpleSdlBuilderForm.tsx
  • apps/deploy-web/src/hooks/useSdlServiceManager/useSdlServiceManager.spec.tsx
  • apps/deploy-web/src/hooks/useSdlServiceManager/useSdlServiceManager.ts
  • apps/deploy-web/src/utils/sdl/sdlImport.spec.ts
  • apps/deploy-web/src/utils/sdl/sdlImport.ts

📝 Walkthrough

Walkthrough

This PR refactors SDL import and service management to support one placement per service. The core utility adds a placementPerService flag controlling placement ID deduplication; the service manager hook explicitly creates and removes placements during service lifecycle; and all SDL import entry points enable this mode by default.

Changes

Placement-per-service SDL import and service lifecycle

Layer / File(s) Summary
SDL import placement mode
apps/deploy-web/src/utils/sdl/sdlImport.ts
importSimpleSdl signature adds optional { placementPerService = false } parameter. Placement ID assignment switches between per-service generation and deduplication by placementName based on the flag.
Service lifecycle with placement cleanup
apps/deploy-web/src/hooks/useSdlServiceManager/useSdlServiceManager.ts
Placement field array now exposes removePlacement. add creates a fresh placement per service instead of reusing placements[0]. remove computes and deletes orphan placements no longer referenced by any remaining service.
SDL import entry points enable per-service mode
apps/deploy-web/src/components/new-deployment/SdlBuilder.tsx, apps/deploy-web/src/components/remote-deploy/update/RemoteDeployUpdate.tsx, apps/deploy-web/src/components/sdl/ImportSdlModal.tsx, apps/deploy-web/src/components/sdl/SimpleSdlBuilderForm.tsx
All four SDL import call sites now pass { placementPerService: true } to importSimpleSdl.
Test coverage for placement behavior
apps/deploy-web/src/utils/sdl/sdlImport.spec.ts, apps/deploy-web/src/hooks/useSdlServiceManager/useSdlServiceManager.spec.tsx
New test cases verify one-to-one placement-per-service mapping under the new mode and validate orphan placement cleanup in the hook. Test helper updated to accept optional defaultPlacements.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

size: M, experienced-contributor

Suggested reviewers

  • stalniy
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sdl-per-service-placement

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.18519% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.93%. Comparing base (eadb218) to head (824e239).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...onents/remote-deploy/update/RemoteDeployUpdate.tsx 0.00% 0 Missing and 1 partial ⚠️
...s/deploy-web/src/components/sdl/ImportSdlModal.tsx 0.00% 1 Missing ⚠️
...oy-web/src/components/sdl/SimpleSdlBuilderForm.tsx 0.00% 1 Missing ⚠️
...hooks/useSdlServiceManager/useSdlServiceManager.ts 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3292      +/-   ##
==========================================
- Coverage   68.30%   66.93%   -1.38%     
==========================================
  Files        1081      993      -88     
  Lines       26443    24251    -2192     
  Branches     6351     5917     -434     
==========================================
- Hits        18063    16233    -1830     
+ Misses       7338     7007     -331     
+ Partials     1042     1011      -31     
Flag Coverage Δ *Carryforward flag
api 84.93% <ø> (ø) Carriedforward from eadb218
deploy-web 52.95% <85.18%> (+0.04%) ⬆️
log-collector ?
notifications 91.44% <ø> (ø) Carriedforward from eadb218
provider-console 81.38% <ø> (ø) Carriedforward from eadb218
provider-inventory ?
provider-proxy 86.37% <ø> (ø) Carriedforward from eadb218
tx-signer ?

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...y-web/src/components/new-deployment/SdlBuilder.tsx 75.94% <100.00%> (ø)
apps/deploy-web/src/utils/sdl/sdlImport.ts 82.88% <100.00%> (+0.97%) ⬆️
...onents/remote-deploy/update/RemoteDeployUpdate.tsx 2.00% <0.00%> (ø)
...s/deploy-web/src/components/sdl/ImportSdlModal.tsx 0.00% <0.00%> (ø)
...oy-web/src/components/sdl/SimpleSdlBuilderForm.tsx 0.00% <0.00%> (ø)
...hooks/useSdlServiceManager/useSdlServiceManager.ts 94.87% <92.30%> (+6.99%) ⬆️

... and 89 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ygrishajev
ygrishajev enabled auto-merge June 10, 2026 09:55
@ygrishajev
ygrishajev added this pull request to the merge queue Jun 10, 2026
Merged via the queue into main with commit abc2ff1 Jun 10, 2026
57 checks passed
@ygrishajev
ygrishajev deleted the fix/sdl-per-service-placement branch June 10, 2026 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants