Docker improvements #5946
Docker improvements #5946
Conversation
gc
commented
Jul 8, 2024
•
gc
commented
- Use full dockerfile for integration tests
- Add yarn build:esbuild
- Shuffle around global side-effect files
- Log files dont say the bot type now
- No longer counting sql queries
- Disabled roles task/test for now
- Add yarn watch script
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 8aa81b1 in 59 seconds
More details
- Looked at
171lines of code in5files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. dockerfile:21
- Draft comment:
TheCMDcommand useslocalhostto wait for the database port, which will not work in Docker where services are isolated. Use the service namedbinstead.
CMD ["sh", "-c", "wait-on tcp:db:5435 && \
yarn prisma db push --schema='./prisma/schema.prisma' && \
yarn prisma db push --schema='./prisma/robochimp.prisma' && \
yarn build:tsc && \
vitest run --config vitest.integration.config.mts"]
- Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_hFNBbQ9T9SCkX5ZM
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
docker-compose.yml
Outdated
| depends_on: | ||
| - db | ||
| environment: | ||
| ROBOCHIMP_DATABASE_UR: postgresql://postgres:postgres@localhost:5435/robochimp_integration_test?connection_limit=500&pool_timeout=0&schema=public |
There was a problem hiding this comment.
The environment variable name ROBOCHIMP_DATABASE_UR is misspelled. It should be ROBOCHIMP_DATABASE_URL to ensure correct usage in the application.
| ROBOCHIMP_DATABASE_UR: postgresql://postgres:postgres@localhost:5435/robochimp_integration_test?connection_limit=500&pool_timeout=0&schema=public | |
| ROBOCHIMP_DATABASE_URL: postgresql://postgres:postgres@localhost:5435/robochimp_integration_test?connection_limit=500&pool_timeout=0&schema=public |
docker-compose.yml
Outdated
| @@ -10,3 +10,19 @@ services: | |||
| POSTGRES_USER: postgres | |||
| POSTGRES_PASSWORD: postgres | |||
| PGPORT: 3001 | |||
There was a problem hiding this comment.
The PGPORT environment variable is set to 3001, but the port mapping for the database service is 5435:3001. This could lead to connection issues. The PGPORT should match the external port exposed by the container.
| PGPORT: 3001 | |
| PGPORT: 5435 |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 7f31eac in 30 seconds
More details
- Looked at
118lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_l5iXi5y4N5iF4MY8
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
dockerfile
Outdated
| ENV NODE_OPTIONS="--enable-source-maps --max_old_space_size=4096" | ||
|
|
||
| COPY --from=builder /usr/src/app/dist ./dist | ||
| COPY --from=builder /usr/src/app/src/.env ./src/.env |
There was a problem hiding this comment.
Copying the .env file into the Docker image can expose sensitive information. Consider removing this line to enhance security, especially since the .dockerignore is set to ignore .env files.
| COPY --from=builder /usr/src/app/src/.env ./src/.env | |
| # COPY --from=builder /usr/src/app/src/.env ./src/.env |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b804890 in 57 seconds
More details
- Looked at
414lines of code in14files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_NPuP4mTgfrLxRmzn
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| RUN yarn install --immutable | ||
|
|
||
|
|
||
| FROM base AS build-run |
There was a problem hiding this comment.
The NODE_ENV is set to "development" in the Dockerfile, which is typically not recommended for production builds as it can expose more verbose error messages and potentially sensitive stack traces. Consider using a production setting or managing this through environment variables passed at runtime.
| COPY --from=dependencies /usr/src/app/node_modules ./node_modules | ||
| COPY . . | ||
|
|
||
| ENV CI=true |
There was a problem hiding this comment.
The .env.test file is being copied and renamed to .env in the Dockerfile. This is generally not recommended for production environments as test configurations might include sensitive defaults or settings not suitable for production. Consider using environment-specific configuration files or managing these settings through environment variables.
dockerfile
Outdated
| ADD https://github.com/ufoscout/docker-compose-wait/releases/download/2.9.0/wait /wait | ||
| RUN chmod +x /wait | ||
|
|
||
| CMD /wait && yarn prisma db push --schema='./prisma/robochimp.prisma' && yarn prisma db push --schema='./prisma/schema.prisma' && yarn vitest run --config vitest.integration.config.mts |
There was a problem hiding this comment.
The CMD instruction in the Dockerfile is executing multiple commands which include schema migrations and tests. This is not recommended for a Dockerfile that is meant to run a production application, as it should only start the application. Consider separating these tasks into different stages or scripts that are not part of the Docker container's startup command.
| RUN apk add --no-cache dumb-init python3 g++ make git | ||
| RUN corepack enable | ||
|
|
||
| COPY yarn.lock package.json .yarnrc.yml ./ |
There was a problem hiding this comment.
The COPY command is used without specifying ownership, which might lead to files being owned by the root user. This can pose a security risk if the application is expected to run as a non-root user but has access to root-owned files. Consider using the --chown=node:node option with the COPY command to explicitly set file ownership.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6fb39ef in 21 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_oTQq7II7H1u9ldZu
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| RUN apk add --no-cache dumb-init python3 g++ make git | ||
| RUN corepack enable | ||
|
|
||
| COPY yarn.lock package.json .yarnrc.yml ./ |
There was a problem hiding this comment.
Removing the .yarn/ directory copy step could potentially break the yarn install command if the project relies on specific configurations or patches stored in this directory. Consider restoring this step if it's crucial for the build process.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6760a69 in 48 seconds
More details
- Looked at
24lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. dockerfile:22
- Draft comment:
The PR description lacks detail about the changes made and their purpose. Please provide a comprehensive list of changes and explain the intent behind them to facilitate a better review process. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_w6nxLlj3169XSp94
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Run Integration Tests | ||
| run: docker-compose up --build --abort-on-container-exit |
There was a problem hiding this comment.
The command docker-compose up --build --abort-on-container-exit might not be appropriate for integration tests as it builds and runs all services, which could include non-test related services. Consider specifying only the necessary services or adjusting the docker-compose configuration to ensure only relevant services are included during the test phase.
|
|
||
| ENV CI=true | ||
| RUN cp .env.test .env | ||
| RUN cp src/config.example.ts src/config.ts |
There was a problem hiding this comment.
Copying src/config.example.ts to src/config.ts might not be suitable for all environments. Consider using environment-specific configurations or managing this through environment variables to ensure the Docker container is configured correctly across different environments.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 0d24af7 in 36 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. dockerfile:27
- Draft comment:
Switching fromyarn run build:esbuildtoyarn run build:tsccould significantly affect build times and output. Ensure this change is intentional and tested for compatibility and performance impacts. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_D7NopXlYy8BMwg9Z
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
src/lib/workers/index.ts
Outdated
| maxThreads | ||
| filename: finishWorkerPath, | ||
| maxThreads, | ||
| workerData: { fullpath: finishWorkerPath } |
There was a problem hiding this comment.
The workerData property with fullpath seems redundant since filename already specifies the path to the worker script. If fullpath is not utilized within the worker for specific operations, consider removing it to clean up the configuration.
This comment applies to the configurations of finishWorker, killWorker, and casketWorker on lines 56, 61, and 66 respectively.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on e41f5e2 in 28 seconds
More details
- Looked at
33lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_mTG2l4HZa6m1hhTl
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
dockerfile
Outdated
| ADD https://github.com/ufoscout/docker-compose-wait/releases/download/2.9.0/wait /wait | ||
| RUN chmod +x /wait | ||
|
|
||
| CMD /wait && yarn prisma db push --schema='./prisma/robochimp.prisma' && yarn prisma db push --schema='./prisma/schema.prisma' && yarn run build:tsc && yarn vitest run --config vitest.integration.config.mts |
There was a problem hiding this comment.
Consider encapsulating the commands in the CMD instruction into a script to handle potential failures more gracefully and improve maintainability. Additionally, move the TypeScript build step (yarn run build:tsc) to the Docker image build phase to reduce container startup time and catch build issues earlier.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9b0c7bf in 30 seconds
More details
- Looked at
41lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_6ix744V2M45dCOmy
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -44,16 +44,25 @@ export type FinishWorkerReturn = Promise< | |||
|
|
|||
| const maxThreads = production ? 3 : 1; | |||
|
|
|||
| let dirName = __dirname.replace('src/lib', 'dist/lib'); | |||
There was a problem hiding this comment.
The changes in directory path resolution could potentially lead to incorrect paths if the directory structure does not match the expected 'dist/lib/workers' after the build. Please ensure that the build output directory structure aligns with these changes to avoid runtime errors.
This suggestion reverts to the original logic but adds a check to ensure the directory ends with 'dist' before appending 'lib/workers', which might be safer if the build directory structure is consistent with these expectations.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on ecc57d6 in 34 seconds
More details
- Looked at
69lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_EpnTe36VOPZBTBO5
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
dockerfile
Outdated
| RUN chmod +x /wait | ||
|
|
||
| CMD /wait && \ | ||
| psql postgres://postgres:postgres@localhost:5435/postgres -c 'CREATE EXTENSION IF NOT EXISTS intarray;' && \ |
There was a problem hiding this comment.
Running PostgreSQL commands directly in the Dockerfile CMD is not recommended as it assumes the database is always available at localhost:5435. Consider managing the database with Docker Compose or another orchestration tool to ensure proper service availability and isolation.
| psql postgres://postgres:postgres@localhost:5435/postgres -c 'CREATE EXTENSION IF NOT EXISTS intarray;' && \ | |
| # Consider managing PostgreSQL with Docker Compose or another orchestration tool |
| yarn prisma db push --schema='./prisma/schema.prisma' && \ | ||
| yarn run build:tsc && \ | ||
| yarn vitest run --config vitest.integration.config.mts && \ | ||
| exit 0 |
There was a problem hiding this comment.
Including exit 0 in the Dockerfile CMD can mask errors from previous commands. It's better to let the last command's exit status naturally determine the container's exit status.
| exit 0 | |
| # Remove the `exit 0` to allow natural exit status reporting |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 5eba8a1 in 27 seconds
More details
- Looked at
35lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_2ovOTXqt7bFzUFHn
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
docker-compose.yml
Outdated
| PGPORT: 5435 | ||
| volumes: | ||
| - postgres_data:/var/lib/postgresql/data | ||
| - .tests/integration/initdb:/docker-entrypoint-initdb.d |
There was a problem hiding this comment.
The path for the volume mapping seems to be incorrect. It should start with ./ to correctly reference the directory from the project root.
| - .tests/integration/initdb:/docker-entrypoint-initdb.d | |
| - ./tests/integration/initdb:/docker-entrypoint-initdb.d |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 18f9097 in 36 seconds
More details
- Looked at
41lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. dockerfile:13
- Draft comment:
Removingzlib-syncbefore runningyarn installmight lead to unintended consequences ifzlib-syncis still needed by the application. Please ensure that this change does not remove necessary functionality or break the application. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
2. tests/integration/grandExchange.test.ts:17
- Draft comment:
Reducing the number of users from 20 to 10 in the test might impact the test coverage and effectiveness in catching potential issues. Please ensure that this change is justified and does not compromise the test's ability to simulate realistic scenarios. - Reason this comment was not posted:
Confidence of 0% on close inspection, compared to threshold of 50%.
Workflow ID: wflow_S97TxdS3Tkh5sgyz
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| await prisma.$queryRaw`CREATE EXTENSION IF NOT EXISTS intarray;`.catch(noOp); | ||
| }); | ||
| const __prismaClient = new PrismaClient(); | ||
| __prismaClient.$queryRawUnsafe('CREATE EXTENSION IF NOT EXISTS intarray;').then(noOp).catch(noOp); |
There was a problem hiding this comment.
Adding .catch(noOp) to silently ignore potential errors during the creation of the intarray extension might hide critical issues. Consider logging or handling these errors more explicitly to ensure they do not affect the application's stability or test integrity.
