Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a better optimized node image. #34

Merged
merged 2 commits into from
Jul 23, 2023

Conversation

SamTV12345
Copy link
Contributor

node latest includes a lot of bloat that is not really needed for a normal nodejs application.

@hwgilbert16
Copy link
Owner

Have you tested this locally beyond just building the image? Alpine has discrepancies in regard to the C compiler and other small changes that could potentially throw issues with some of our dependencies, along with the image not being tested as extensively as the main Node images.

@SamTV12345 SamTV12345 marked this pull request as draft July 22, 2023 20:14
@SamTV12345
Copy link
Contributor Author

Sorry. Should have been a draft. I'll continue once I have more time. It is crashing on an existing installation. Somehow without any error. Could it be prisma?

@hwgilbert16
Copy link
Owner

It is crashing on an existing installation.

With this new alpine image, or the existing one that is published?

@SamTV12345
Copy link
Contributor Author

It is crashing on an existing installation.

With this new alpine image, or the existing one that is published?

The new one. I'll have to take a deeper dive why it is crashing. Normally NodeJS should be platform independent and I didn't saw any installation in your Dockerfile. But yeah. It definitely needs some testing.

@hwgilbert16
Copy link
Owner

Seems like it was something with the environment file validation that (unsure why) only failed when using the Alpine image. Give it another shot after pulling from develop again.

@SamTV12345
Copy link
Contributor Author

I get this:

10 migrations found in prisma/migrations


No pending migrations to apply.
Configuration validation error: "HTTP_PORT" is required

> [email protected] serve:node
> npm run migrate && node dist/apps/api/main.js


> [email protected] migrate
> npx prisma migrate deploy

Prisma schema loaded from prisma/schema.prisma
Datasource "db": MySQL database "scholarsome" at "mariadb:3306"

10 migrations found in prisma/migrations


No pending migrations to apply.
Configuration validation error: "HTTP_PORT" is required

> [email protected] serve:node
> npm run migrate && node dist/apps/api/main.js


> [email protected] migrate
> npx prisma migrate deploy

Prisma schema loaded from prisma/schema.prisma
Datasource "db": MySQL database "scholarsome" at "mariadb:3306"

10 migrations found in prisma/migrations


No pending migrations to apply.
Configuration validation error: "HTTP_PORT" is required

@SamTV12345
Copy link
Contributor Author

I had to add HTTP_PORT and set the HOST variable. But now it is working without a problem. BTW you can set variables also with

web:
  env_file:
    - .env

that way you don't have to write VARIABLE=$VARIABLE and can just keep the other more complicated variables like database url.

@hwgilbert16
Copy link
Owner

you can set variables also with

Not sure why I didn't have that - could have sworn I was just passing the file.

If you add that I'll merge this, the Alpine image is certainly smaller.

@SamTV12345
Copy link
Contributor Author

Done

@SamTV12345 SamTV12345 marked this pull request as ready for review July 23, 2023 19:31
@hwgilbert16 hwgilbert16 merged commit 4e43cdb into hwgilbert16:develop Jul 23, 2023
@hwgilbert16 hwgilbert16 mentioned this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants