feat(cli): add application container to docker demo; make mssql opt-in - #59
Merged
Conversation
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Task 13 of the 14-task plan.
nl2sql setup --demo --dockergenerated a compose file with four databaseservices and no application, so the demo could not actually answer a query
without the user installing and wiring up
nl2sqlby hand. MSSQL also forced a~1.6 GB pull before anything came up.
What changed
DOCKER_COMPOSE_TEMPLATEgains anappservice: built from the repo root(
context: ..,dockerfile: packages/api/Dockerfile), publishes8000:8000,reads the same
.env.demothe lite path writes (env_file: ../.env.demo),mounts
../configsand../data, and waits on the three databases withcondition: service_healthy(all three declare healthchecks;manufacturing_historydoes not, and nothing depends on it).manufacturing_historyis now behindprofiles: ["mssql"]. Nothing outsidethe profile depends on it, so the default
upnever resolves it.DemoManager.setup_dockernow always writes.env.demo, instead of behind aprompt: the app service's
env_filemust exist or Compose refuses to start(and
docker compose configfails). The corresponding confirm prompt insetup_commandis gone; the next-steps panel documents the API port and the--profile mssqlopt-in.packages/api/Dockerfilenow builds from the repo instead of PyPI. Itinstalled
nl2sqlandnl2sql-apifrom PyPI, where nothing is published yet,so it could not build at all. It now copies
packages/adapter-sdk,packages/nl2sqlandpackages/apifrom the build context and installs./packages/adapter-sdk,./packages/nl2sql[postgres,mysql]and./packages/api. TheNL2SQL_EXTRASbuild arg is gone; the extras are in theinstall line.
.gitignoregainsdemo_docker/— stated explicitly rather than quietly: thedirectory
setup --demo --dockergenerates was not ignored, and it containsgenerated database passwords.
Tests
No docker-writer tests existed before this PR. Nothing under
packages/nl2sql/tests/referencedDockerWriter,write_docker, ordocker-compose.demo, sopackages/nl2sql/tests/cli/test_demo_docker_compose.pyis the first coverage for this writer. It generates the stack through the writer,
parses the compose file with
yaml.safe_load, and asserts theappservice, itsport, its
.env.demoenv file,service_healthydependencies, themssqlprofile on
manufacturing_history, and that nothing outside that profile dependson it. Written first: 5 of the 6 failed against the old template
(
KeyError: 'app',KeyError: 'profiles').Unit: 225 passed, 1 skipped, 47 deselected before; 231 passed, 1 skipped,
47 deselected after. Key-free integration: 28 passed, unchanged. Both suites run
twice under
pytest-randomly.Compose validation
Validated with a real
docker compose configrun (Docker 28.0.4) againstgenerated output; no images pulled, nothing started.
Default (
docker compose -f docker-compose.demo.yml config --services):With
--profile mssql:Both resolve cleanly.
app'sdepends_onresolves toservice_healthyon thethree database services and nothing references
manufacturing_historyoutsidethe profile.
Known gap, not addressed here
configs/datasources.demo.yamlfor the docker demo points every datasource atlocalhost:<published port>, which is correct from the host but does not resolvefrom inside the
appcontainer. The compose stack starts as specified, but aquery issued against the containerised API would not reach the databases until
that config grows a container-side host. Changing it would break the documented
host-side
nl2sql --env demo index/runworkflow, so it is left for afollow-up decision rather than changed here.