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

fix: docker build of Excalidraw app #7430

Merged
merged 3 commits into from
May 7, 2024

Conversation

agebhar1
Copy link
Contributor

Fixes #7403.

Copy link

vercel bot commented Dec 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview May 7, 2024 11:59am
excalidraw ✅ Ready (Inspect) Visit Preview May 7, 2024 11:59am
excalidraw-package-example ✅ Ready (Inspect) Visit Preview May 7, 2024 11:59am
excalidraw-package-example-with-nextjs ✅ Ready (Inspect) Visit Preview May 7, 2024 11:59am

Copy link

vercel bot commented Dec 12, 2023

@agebhar1 is attempting to deploy a commit to the Excalidraw Team on Vercel.

A member of the Team first needs to authorize it.

@ad1992
Copy link
Member

ad1992 commented Feb 12, 2024

@agebhar1 can you confirm if the updated docker build in this PR works for you ?

@agebhar1
Copy link
Contributor Author

As far as I can remember yes @ad1992, but let me check again today.

@agebhar1
Copy link
Contributor Author

There was a small issue left but now the container is build @ad1992. Also updated to latest version of Nginx.

@agebhar1
Copy link
Contributor Author

Any chance to get this PR merged @ad1992?

Copy link

@aolyang aolyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fix works fine and no bad side effect.

image

image

@AlphaCraft9658
Copy link

This should be merged.

@ad1992
Copy link
Member

ad1992 commented May 7, 2024

since its been long time, @agebhar1 @AlphaCraft9658 can one of you test the build and confirm if this PR is good to go?

@AlphaCraft9658
Copy link

since its been long time, @agebhar1 @AlphaCraft9658 can one of you test the build and confirm if this PR is good to go?

How should I test it? Clone and build, or with GitHub Cl?

@ad1992
Copy link
Member

ad1992 commented May 7, 2024

since its been long time, @agebhar1 @AlphaCraft9658 can one of you test the build and confirm if this PR is good to go?

How should I test it? Clone and build, or with GitHub Cl?

Yes, Running the docker build command locally and testing it out.

@AlphaCraft9658
Copy link

since its been long time, @agebhar1 @AlphaCraft9658 can one of you test the build and confirm if this PR is good to go?

How should I test it? Clone and build, or with GitHub Cl?

Yes, Running the docker build command locally and testing it out.

I'll do that on my homelab in a second.

@AlphaCraft9658
Copy link

since its been long time, @agebhar1 @AlphaCraft9658 can one of you test the build and confirm if this PR is good to go?

How should I test it? Clone and build, or with GitHub Cl?

Yes, Running the docker build command locally and testing it out.

Cross-env is missing. We should add it to the dev dependencies.

@aolyang
Copy link

aolyang commented May 7, 2024

I have done test for locally, see #7430 (review)

and https://excalidraw.aolyang.me/

I have used for a time, just try it

image

@AlphaCraft9658
Copy link

AlphaCraft9658 commented May 7, 2024

I have done test for locally, see #7430 (review)

and https://excalidraw.aolyang.me/

I have used for a time, just try it

image

It doesn't work for me and it says that cross-env is not found.

EDIT: I did clone his repository, but his changes are not present.

EDIT 2: Oh, wrong branch.

@AlphaCraft9658
Copy link

since its been long time, @agebhar1 @AlphaCraft9658 can one of you test the build and confirm if this PR is good to go?

I tested it. The build succeeds.

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aolyang @AlphaCraft9658 @agebhar1 , merging 🚀

@AlphaCraft9658
Copy link

Tests are failing.

@AlphaCraft9658
Copy link

Tests are failing.

@ad1992 ?

@AlphaCraft9658
Copy link

Any ETA on when the next release with this fix will be pushed? It should automatically build and push to docker hub, if everything works now, including the publish workflow.

@ad1992
Copy link
Member

ad1992 commented May 7, 2024

Any ETA on when the next release with this fix will be pushed? It should automatically build and push to docker hub, if everything works now, including the publish workflow.

I will wait for some time to push to release so at max by tomorrow it should be deployed.

@agebhar1 agebhar1 deleted the feature/gh7403-fix-docker-build branch May 7, 2024 16:58
@ad1992
Copy link
Member

ad1992 commented May 8, 2024

The docker build is published 🚀

@jauderho
Copy link

jauderho commented May 8, 2024

I was eager to test this out and can confirm that the Docker image does build. But then I noticed that the build image node:20 is not an Alpine base image.

node:20-alpine should probably be used as the build image instead of node:20

I can confirm that node:20-alpine builds.

@agebhar1
Copy link
Contributor Author

agebhar1 commented May 9, 2024

The builder images was not changed with this PR, it's still node:18.

@jauderho
Copy link

jauderho commented May 9, 2024

My point still stands that the build image ideally should be the same Linux OS as the final image. This just means that the issue has been around longer than the most recent changes.

@agebhar1
Copy link
Contributor Author

It's almost an good idea to use the same image IMHO. The build result are only static files which are served by Nginx. Maybe I did not see the issue to serve these files from a different Linux OS.

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.

[ Bug ] Docker image on Dockerhub not updated for 4 months
5 participants