typescript compose builder #375
Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Coverage Report for ./apps/cli
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
endersonmaia
force-pushed
the
feature/ts-compose-builder
branch
2 times, most recently
from
bd75d24 to
68d9896
Compare
endersonmaia
force-pushed
the
feature/ts-compose-builder
branch
2 times, most recently
from
90656d3 to
062e3bd
Compare
There was a problem hiding this comment.
Pull request overview
This pull request successfully refactors the Docker Compose configuration for the CLI app by migrating from static YAML files to a programmatic TypeScript-based approach using a builder pattern. This significantly improves maintainability and flexibility.
Key Changes:
- Replaced all static Docker Compose YAML files with TypeScript modules that export service and proxy configuration objects
- Implemented a
ComposeBuilderclass with fluent API for constructing Compose configurations programmatically - Consolidated environment variable management from
.envfiles into TypeScript constants - Added the
yamlnpm package for YAML generation and removed the unusedcopyfilesdependency
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
apps/cli/src/types/compose.ts |
New comprehensive TypeScript type definitions for Docker Compose specification |
apps/cli/src/compose/builder.ts |
New builder class implementing fluent API for creating Compose configurations |
apps/cli/src/compose/anvil.ts |
Anvil service and proxy configuration in TypeScript |
apps/cli/src/compose/bundler.ts |
Bundler service and proxy configuration in TypeScript |
apps/cli/src/compose/database.ts |
Database service configuration in TypeScript |
apps/cli/src/compose/common.ts |
Shared constants for environment variables and health checks |
apps/cli/src/compose/explorer.ts |
Explorer, Explorer API, and Squid Processor service configurations |
apps/cli/src/compose/passkey.ts |
Passkey server service and proxy configuration |
apps/cli/src/compose/paymaster.ts |
Paymaster service and proxy configuration |
apps/cli/src/compose/proxy.ts |
Traefik proxy service configuration |
apps/cli/src/compose/rollupsNode.ts |
Rollups Node service and proxy configuration |
apps/cli/src/exec/rollups.ts |
Updated to use ComposeBuilder instead of static YAML files |
apps/cli/tests/unit/compose.test.ts |
Comprehensive test suite for ComposeBuilder functionality |
apps/cli/package.json |
Added yaml dependency, removed copyfiles, updated build scripts |
pnpm-lock.yaml |
Updated dependency lock file reflecting package changes |
Various docker-compose-*.yaml files |
Deleted static YAML files replaced by TypeScript modules |
apps/cli/src/compose/default.env |
Deleted environment file, moved to TypeScript |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // pull images first | ||
| const pullArgs = ["--policy", "missing"]; | ||
| await execa("docker", [...composeArgs, "pull", ...pullArgs], { | ||
| env, | ||
| stdio: "inherit", | ||
| }); | ||
| // const pullArgs = ["--policy", "missing"]; | ||
| // await execa("docker", [...composeArgs, "pull", ...pullArgs], { | ||
| // env, | ||
| ////FIXME: stdio and input won't work together | ||
| // stdio: "inherit", | ||
| // input: composeFile.build() | ||
| // }); |
There was a problem hiding this comment.
The commented-out code for pulling Docker images should either be removed or properly implemented. Leaving it commented with a FIXME indicates unfinished work. Either remove this commented block entirely or create a proper TODO comment explaining the issue and when it should be addressed.
endersonmaia
force-pushed
the
feature/ts-compose-builder
branch
from
e968264 to
c94d9e2
Compare
endersonmaia
force-pushed
the
feature/ts-compose-builder
branch
from
c94d9e2 to
e7cda47
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 12 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| imageTag: "1.3.3", | ||
| listenPort: port, |
There was a problem hiding this comment.
The hardcoded image tag "1.3.3" for the explorer service differs from the pattern used for other services. Consider using a constant or configuration value instead of hardcoded version strings to make version management more maintainable.
| (this.composeFile.services!.anvil.command as Array<string>).push( | ||
| `--block-time=${options.blockTime.toString()}`, | ||
| ); |
There was a problem hiding this comment.
The block time option is being appended to the command array each time withAnvil is called, which will result in duplicate --block-time arguments if the method is called multiple times. Consider either clearing the command first or checking if the option already exists before appending.
| (this.composeFile.services!.anvil.command as Array<string>).push( | |
| `--block-time=${options.blockTime.toString()}`, | |
| ); | |
| const anvilService = this.composeFile.services!.anvil; | |
| let command = anvilService.command as Array<string> | undefined; | |
| if (!Array.isArray(command)) { | |
| command = []; | |
| } | |
| const blockTimeArg = `--block-time=${options.blockTime.toString()}`; | |
| if (!command.some((arg) => arg.startsWith("--block-time"))) { | |
| command.push(blockTimeArg); | |
| } | |
| anvilService.command = command; |
| this.composeFile.services!.anvil = { | ||
| ...ANVIL_SVC, | ||
| ...this.composeFile.services!.anvil, | ||
| }; |
There was a problem hiding this comment.
The pattern of spreading the default service and then spreading the existing service creates a merge conflict. If a property exists in both, the second spread will win. This could lead to unexpected behavior where the default configuration overrides user customizations. Consider a more explicit merge strategy that preserves user customizations.
| imageTag: "1.0.0", | ||
| }); |
There was a problem hiding this comment.
The hardcoded image tag "1.0.0" for the explorer-api service differs from the pattern used for other services. Consider using a constant or configuration value instead of hardcoded version strings to make version management more maintainable.
| }); | ||
|
|
||
| // cpu and memory limits, mostly for testing and debuggingpurposes | ||
| // cpu and memory limits, mostly for testing and debugging purposes |
There was a problem hiding this comment.
Typo in comment: "debuggingpurposes" should be "debugging purposes" (missing space between words).
| import { Healthcheck } from "../types/compose"; | ||
|
|
||
| export const DB_ENV: Record<string, string | number | boolean | null> = { | ||
| DB_PORT: 5432, | ||
| DB_HOST: "database", | ||
| DB_PASS: "password", | ||
| }; | ||
|
|
||
| export const DEFAULT_HEALTHCHECK: Healthcheck = { | ||
| start_period: "10s", | ||
| start_interval: "200ms", | ||
| interval: "10s", | ||
| timeout: "1s", | ||
| retries: 5, | ||
| }; | ||
|
|
||
| export const DEFAULT_ENV: Record<string, string | number | boolean | null> = { | ||
| CARTESI_LOG_LEVEL: "${CARTESI_LOG_LEVEL:-info}", | ||
| // features | ||
| CARTESI_FEATURE_INPUT_READER_ENABLED: | ||
| "${CARTESI_FEATURE_INPUT_READER_ENABLED:-true}", | ||
| CARTESI_FEATURE_CLAIM_SUBMISSION_ENABLED: | ||
| "${CARTESI_FEATURE_CLAIM_SUBMISSION_ENABLED:-true}", | ||
| CARTESI_FEATURE_MACHINE_HASH_CHECK_ENABLED: | ||
| "${CARTESI_FEATURE_MACHINE_HASH_CHECK_ENABLED:-true}", | ||
| CARTESI_SNAPSHOTS_DIR: "/var/lib/cartesi-rollups-node/snapshots", | ||
| // rollups | ||
| CARTESI_EVM_READER_RETRY_POLICY_MAX_RETRIES: | ||
| "${CARTESI_EVM_READER_RETRY_POLICY_MAX_RETRIES:-3}", | ||
| CARTESI_EVM_READER_RETRY_POLICY_MAX_DELAY: | ||
| "${CARTESI_EVM_READER_RETRY_POLICY_MAX_DELAY:-3}", | ||
| CARTESI_ADVANCER_POLLING_INTERVAL: | ||
| "${CARTESI_ADVANCER_POLLING_INTERVAL:-3}", | ||
| CARTESI_VALIDATOR_POLLING_INTERVAL: | ||
| "${CARTESI_VALIDATOR_POLLING_INTERVAL:-3}", | ||
| CARTESI_CLAIMER_POLLING_INTERVAL: "${CARTESI_CLAIMER_POLLING_INTERVAL:-3}", | ||
| CARTESI_MAX_STARTUP_TIME: "${CARTESI_MAX_STARTUP_TIME:-15}", | ||
| // blockchain | ||
| CARTESI_BLOCKCHAIN_ID: "${CARTESI_BLOCKCHAIN_ID:-13370}", | ||
| CARTESI_BLOCKCHAIN_HTTP_ENDPOINT: | ||
| "${CARTESI_BLOCKCHAIN_HTTP_ENDPOINT:-http://anvil:8545}", | ||
| CARTESI_BLOCKCHAIN_WS_ENDPOINT: | ||
| "${CARTESI_BLOCKCHAIN_WS_ENDPOINT:-ws://anvil:8545}", | ||
| CARTESI_BLOCKCHAIN_DEFAULT_BLOCK: | ||
| "${CARTESI_BLOCKCHAIN_DEFAULT_BLOCK:-latest}", | ||
| // contracts | ||
| CARTESI_CONTRACTS_APPLICATION_FACTORY_ADDRESS: | ||
| "${CARTESI_CONTRACTS_APPLICATION_FACTORY_ADDRESS:-0xc7006f70875BaDe89032001262A846D3Ee160051}", | ||
| CARTESI_CONTRACTS_AUTHORITY_FACTORY_ADDRESS: | ||
| "${CARTESI_CONTRACTS_AUTHORITY_FACTORY_ADDRESS:-0xC7003566dD09Aa0fC0Ce201aC2769aFAe3BF0051}", | ||
| CARTESI_CONTRACTS_INPUT_BOX_ADDRESS: | ||
| "${CARTESI_CONTRACTS_INPUT_BOX_ADDRESS:-0xc70074BDD26d8cF983Ca6A5b89b8db52D5850051}", | ||
| CARTESI_CONTRACTS_SELF_HOSTED_APPLICATION_FACTORY_ADDRESS: | ||
| "${CARTESI_CONTRACTS_SELF_HOSTED_APPLICATION_FACTORY_ADDRESS:-0xc700285Ab555eeB5201BC00CFD4b2CC8DED90051}", | ||
| // auth | ||
| CARTESI_AUTH_MNEMONIC: | ||
| "${CARTESI_AUTH_MNEMONIC:-test test test test test test test test test test test junk}", | ||
| // postgres | ||
| CARTESI_DATABASE_CONNECTION: `postgres://postgres:${DB_ENV.DB_PASS}@${DB_ENV.DB_HOST}:${DB_ENV.DB_PORT}/rollupsdb?sslmode=disable`, | ||
| }; |
There was a problem hiding this comment.
Missing JSDoc comments for the exported constants. Consider adding documentation describing what these constants represent and when they should be used.
| * Build an image string from ServiceOptions | ||
| * @param options - ServiceOptions containing imageName and/or imageTag | ||
| * @param currentImage - Current image string to use as fallback | ||
| * @returns Complete image string in format "imageName:imageTag" |
There was a problem hiding this comment.
The JSDoc comment for this method is outdated. It references parameters and behavior (imageName, currentImage, return value) that don't match the actual implementation. The method should document that it modifies the service's image tag in place and returns 'this' for chaining.
| * Build an image string from ServiceOptions | |
| * @param options - ServiceOptions containing imageName and/or imageTag | |
| * @param currentImage - Current image string to use as fallback | |
| * @returns Complete image string in format "imageName:imageTag" | |
| * Set or update the image tag for a service in the compose file. | |
| * Uses the existing image name and tag as defaults, and applies any | |
| * tag override specified in {@link ServiceOptions}. | |
| * | |
| * @param service - Name of the service whose image tag will be updated. | |
| * @param options - ServiceOptions containing an optional imageTag override. | |
| * @returns The current builder instance for method chaining. |
This pull request refactors the Docker Compose configuration for the CLI app by removing all static YAML Compose files and replacing them with TypeScript modules that define service and proxy configurations programmatically. Additionally, it consolidates environment variable management into TypeScript, updates dependencies, and simplifies the build process.
The most important changes are:
Compose Configuration Refactor:
Removed all static Docker Compose YAML files (
docker-compose-*.yamlanddefault.env) and replaced them with TypeScript modules (anvil.ts,bundler.ts,database.ts,common.ts) that export service and proxy configuration objects. This enables dynamic and maintainable Compose generation. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]Moved all environment variable definitions and default values from
.envfiles into the newcommon.tsTypeScript module for centralized configuration management. [1] [2]Dependency and Build Script Updates:
yamlnpm package as a new dependency to support YAML generation in TypeScript; removed the unusedcopyfilesdependency and related build script since static YAML files are no longer copied. [1] [2] [3]These changes significantly improve the maintainability and flexibility of the CLI's Compose configuration by making it programmatic and easier to extend or modify in the future.