Repository navigation
feat(mongodb-runner): locate the DSC directory with --slsDir - #913
Conversation
929dc53 to
d6c8410
Compare
d6c8410 to
ddd4bed
Compare
ddd4bed to
7193bcb
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Relative bundle paths can break teardown from another working directory, leaving SLS containers running after cluster metadata is deleted.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds automatic SLS bundle discovery to mongodb-runner’s disaggregated-storage support through --slsDir.
Changes:
- Resolves and validates bundles from install roots or atlas directories.
- Adds CLI directory inference and mutual exclusion with
--slsCompose. - Adds resolver tests, fixtures, public exports, and usage documentation.
| File | Description |
|---|---|
packages/mongodb-runner/test/fixtures/sls/not-a-build/.gitkeep |
Preserves an invalid-build fixture. |
packages/mongodb-runner/test/fixtures/sls/bundle-incomplete/buildscripts/modules/atlas/sls-multicell-docker-compose.yml |
Supplies an incomplete-bundle compose fixture. |
packages/mongodb-runner/test/fixtures/sls/bundle-incomplete/buildscripts/modules/atlas/manifest.json |
Supplies the incomplete bundle’s manifest. |
packages/mongodb-runner/test/fixtures/sls/bundle-complete/buildscripts/modules/atlas/slsbackup.proto |
Supplies a required bundle placeholder. |
packages/mongodb-runner/test/fixtures/sls/bundle-complete/buildscripts/modules/atlas/sls-multicell-docker-compose.yml |
Supplies a complete-bundle compose fixture. |
packages/mongodb-runner/test/fixtures/sls/bundle-complete/buildscripts/modules/atlas/manifest.json |
Supplies a pinned image-tag fixture. |
packages/mongodb-runner/test/fixtures/sls/bundle-complete/buildscripts/modules/atlas/flags-state.json |
Completes the valid bundle fixture. |
packages/mongodb-runner/src/sls.ts |
Implements bundle discovery and validation. |
packages/mongodb-runner/src/sls.spec.ts |
Tests bundle resolution and missing files. |
packages/mongodb-runner/src/index.ts |
Exports the resolver and bundle types. |
packages/mongodb-runner/src/cli.ts |
Adds --slsDir handling. |
packages/mongodb-runner/README.md |
Introduces directory-based usage. |
packages/mongodb-runner/docs/disaggregated-storage.md |
Updates prerequisites and examples. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f189737 to
0895f47
Compare
0895f47 to
f15d2a7
Compare
f15d2a7 to
10d18c4
Compare
10d18c4 to
0fcfb17
Compare
0fcfb17 to
fabb78b
Compare
fabb78b to
9f5b97e
Compare
def6dd1 to
de0b5bb
Compare
de0b5bb to
f1e96be
Compare
| throw new Error( | ||
| '--slsDir was given without a value and --binDir is not set, so there is nowhere to look for the SLS dir', | ||
| ); | ||
| })()); |
There was a problem hiding this comment.
What about checking for undefined or empty, and default to the regular flow if so?
Also, we could move this part to a helper function to make it easier to read the flow. Something like startWithSlsDir.
There was a problem hiding this comment.
This had a feature where if it was an empty string, it'd resolve it relative the value in --binDir, but I removed that. It's kind of fiddly. If it's not present at all, yes, it uses the regular flow and requires --slsCompose for DSC. But per the PR description, I was wondering if we might want to get rid of the --slsCompose arg entirely.
If we merge this PR, we have two different ways to start SLS, but the --slsCompose way could fail if you pass a compose file that doesn't have the other required files in the same dir. So just using --slsDir as the only mechanism for starting SLS would make it clearer how to get a DSC cluster running.
There was a problem hiding this comment.
I'll ask someone from the team to have an opinion on this, I don't have much context to know the impact of the removal.
There was a problem hiding this comment.
I think dropping --slsCompose in favour of --slsDir makes sense, but it's a breaking change. In this case I believe it's fine because I think you folks are the only ones using mongodb-runner with SLS right now, but in the future whenever we have other parties using it we should reduce the amount of breaking changes.
There was a problem hiding this comment.
I removed --slsCompose
4e8b9ee to
7b6b656
Compare
| } | ||
| if (!argv.slsDir) { | ||
| throw new Error( | ||
| '--slsDir requires a path to a disaggregated-storage-capable MongoDB build', |
There was a problem hiding this comment.
Probably we want to be more precise what this means?
There was a problem hiding this comment.
I made this clearer
7b6b656 to
fabdc6c
Compare
fabdc6c to
473118d
Compare


Description
This PR makes it easier to start SLS clusters. It adds a new
--slsDirCLI parameter which takes a path. The runner verifies that the path contains all of the files needed to start an SLS cluster, which are:sls-multicell-docker-compose.ymlslsbackup.protoflags-state.jsonThe latter two files aren't used by the runner, but they're referenced from the
sls-multicell-docker-compose.ymlfile, so attempting to start an SLS stack without them present will fail.It also looks for the
manifest.jsonfile in this directory and will pull the SLS image tag from it by default. This can still be overridden with an explicit--slsTagargument.Open Questions
--slsComposearg entirely in favor of--slsDir. Do we want to do that?Checklist