feat(generate): port local-stack stack generation to Deno#19
Conversation
wax911
left a comment
There was a problem hiding this comment.
Review notes against #4:
-
Path rewriting is incorrect for multi-service stacks.
generateSingleStack()rewrites every service usingsources[0]?.composeDir. In local-stack, each service lives in its own directory, so env-file and bind-mount paths for every service after the first can be rewritten relative to the wrong directory. -
The network name is hardcoded as
traefik-public. The issue requires the generated network block to come from config.traefik-publicis the local-stack default, not a universal stackctl assumption. -
Top-level volume metadata is lost. The local-stack Python generator preserves volume metadata such as explicit
name:overrides when emitting external volumes. This implementation emits only{ external: true }for collected names. -
The command currently discovers all stacks when
options.stacksis omitted, but it does not appear to honor configured stack names/order from.stackctl. Determinism should come from config first, discovery second. -
The generated YAML contains a generated header comment. That is fine for output readability, but ensure snapshot tests account for it and that sync comparisons do not drift against existing local-stack generated output unless the migration intentionally updates those files.
This is close in shape, but the path-rewrite bug is a correctness blocker for the actual local-stack repository.
- Fix path rewriting for multi-service stacks (per-service compose dir) - Use config stack.network instead of hardcoded "traefik-public" - Preserve top-level volume metadata from original compose files - Honor config stack.names ordering before discovery - Add deterministic output with sorted keys and stable header - Add snapshot test for generation output
wax911
left a comment
There was a problem hiding this comment.
Follow-up review after the push:
Good fixes:
- The multi-service path rewrite issue is addressed with a service-to-compose-directory map.
- The network name is now configurable through
GenerateOptions.networkinstead of being only hardcoded. - Existing top-level volume definitions are preserved and get
external: truewhen missing. - Config stack names are now accepted through
configStackNames.
Remaining issues:
-
The PR is currently not mergeable and its base SHA is behind the current config PR. Rebase it after #18 is fixed.
-
The default network still falls back to
traefik-public. That is fine as a local-stack default, but it should come from DEFAULT_CONFIG/config wiring rather than being a universal compose module default. If this is intentionally the built-in default, document it as such. -
Confirm snapshot compatibility against current local-stack generated stacks. The added generated header comment and key sorting may intentionally change output, but
syncmust not produce noisy drift during migration unless local-stack updates the committed stack artifacts in the same migration. -
Make sure the service-dir map handles duplicate service names across different compose sources deterministically. The first entry wins today; that should either be validated as an error or documented.
This is much closer. The previous correctness blocker is fixed, but mergeability and parity validation still need attention.
0438af4 to
8566b25
Compare
- Fix path rewriting for multi-service stacks (per-service compose dir) - Use config stack.network instead of hardcoded "traefik-public" - Preserve top-level volume metadata from original compose files - Honor config stack.names ordering before discovery - Add deterministic output with sorted keys and stable header - Add snapshot test for generation output
1d6cee2 to
0c01b0f
Compare
Closes #4