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

Refactor to avoid unnecessary multi-stage build #109

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

tianon
Copy link
Collaborator

@tianon tianon commented Jan 4, 2024

This takes advantage of apk add --virtual to install packages temporarily so they can be cleanly/easily removed after the build is complete.

It also cleans up / avoids extra files being created by Yarn / Node.js during build.

This takes advantage of `apk add --virtual` to install packages temporarily so they can be cleanly/easily removed after the build is complete.

It also cleans up / avoids extra files being created by Yarn / Node.js during build.
@tianon
Copy link
Collaborator Author

tianon commented Jan 4, 2024

I'd be happy to also remove alpine3.17 and add alpine3.19, but didn't want to get ahead of myself 😄

@rtritto
Copy link
Member

rtritto commented Jan 4, 2024

I got again the error [FATAL tini (2)] exec /docker-entrypoint.sh failed: Permission denied caused by removing RUN chmod +x /docker-entrypoint.sh.

@tianon
Copy link
Collaborator Author

tianon commented Jan 4, 2024

How are you building? If you git clone, then git will set the +x bit (and docker build will pass it in correctly).

@tianon
Copy link
Collaborator Author

tianon commented Jan 4, 2024

For example, if you build test via docker build --pull 'https://github.com/mongo-express/mongo-express-docker.git#refs/pull/109/merge:1.0/20-alpine3.18', then Docker will do the git clone and have the correct executable bit on the file.

(Doing chmod +x inside the Dockerfile duplicates the file in a second layer, just to change the metadata 😅)

@rtritto
Copy link
Member

rtritto commented Jan 4, 2024

How are you building? If you git clone, then git will set the +x bit (and docker build will pass it in correctly).

I used git remote add <NAME> https://github.com/tianon/mongo-express-docker.git and then i did the checkout on this branch.


Maybe it's safe to leave the chmod +x.

@BlackthornYugen
Copy link
Member

I got again the error [FATAL tini (2)] exec /docker-entrypoint.sh failed: Permission denied caused by removing RUN chmod +x /docker-entrypoint.sh.

I was never getting Permission denied when I built with docker build --tag mongo-express 1.0/20-alpine3.18. I have it running locally and I'm almost done testing now.

@rtritto
Copy link
Member

rtritto commented Jan 4, 2024

I'm using Podman instead of Docker.
I did git clone https://github.com/tianon/mongo-express-docker.git, podman build -t=<TAG> . and then podman run ... but I got the same error.

Can the error be related to a git setting?

@BlackthornYugen
Copy link
Member

BlackthornYugen commented Jan 4, 2024

I'm using Podman instead of Docker. I did git clone https://github.com/tianon/mongo-express-docker.git, podman build -t=<TAG> . and then podman run ... but I got the same error.

Can the error be related to a git setting?

Since the git clone happens inside the container I don't see how it could be a git setting, I tested using podman on my centos box and it seemed to work:

image

Edit -- Oh, sorry, I misspoke, the copy is on the host machine. Maybe it is a git setting? But I don't think there is a risk of this impacting others if it's passing the test on the official image repo right?

@rtritto
Copy link
Member

rtritto commented Jan 4, 2024

Or maybe is related to the OS (I'm using Windows 11 + WSL)

@BlackthornYugen
Copy link
Member

Or maybe is related to the OS (I'm using Windows 11 + WSL)

I think WSL should work as long as you are not trying to build from an NTFS path. I think WSL reads all files as 777 (rwxrwxrwx) and I'm not sure what git would do in that case. What do you see when you do an ls -l? If you are using an NTFS path, can you try a path that isn't?

@rtritto
Copy link
Member

rtritto commented Jan 4, 2024

I think WSL should work as long as you are not trying to build from an NTFS path. I think WSL reads all files as 777 (rwxrwxrwx) and I'm not sure what git would do in that case. What do you see when you do an ls -l? If you are using an NTFS path, can you try a path that isn't?

I have files (Dockerfile, git cloned repos etc) on Windows and then use a Podman client (it communicates with Podman installed in a WSL distro (podman-machine-default)) to use Podman commands in command line.
Exactly what do I need to do to check?

containers/podman#15299 it's the same problem?

Edit: another issue containers/podman#18093

@BlackthornYugen
Copy link
Member

Exactly what do I need to do to check?

If you cloned from windows then I'm pretty sure the permissions are not going to work. Can you try to clone from the command line in your WSL home folder? Permissions should work correctly there.

You should be able to see what I mean if you do ls -l on /mnt/c/ vs $HOME -- everything in /mnt/c is a translated filesystem and all files will show 777. At least that's the way it worked the last time I had tried WSL, it was probably over a year ago. I haven't had access to a Windows device in a while.

@BlackthornYugen
Copy link
Member

@rtritto do you think it's okay to merge this as-is? Or would you prefer that we keep the chmod +x?

@BlackthornYugen
Copy link
Member

I'd be happy to also remove alpine3.17 and add alpine3.19, but didn't want to get ahead of myself 😄

We should probably keep PR's small if possible, I can make a new one for that after this gets merged. I am curious why alpine3.17 isn't working though... it should still be supported right?

@rtritto
Copy link
Member

rtritto commented Jan 4, 2024

If you cloned from windows then I'm pretty sure the permissions are not going to work. Can you try to clone from the command line in your WSL home folder? Permissions should work correctly there.

You should be able to see what I mean if you do ls -l on /mnt/c/ vs $HOME -- everything in /mnt/c is a translated filesystem and all files will show 777. At least that's the way it worked the last time I had tried WSL, it was probably over a year ago. I haven't had access to a Windows device in a while.

Inside the WSL distro, I did git clone + podman build + podman run and it works.


@rtritto do you think it's okay to merge this as-is? Or would you prefer that we keep the chmod +x?

Yes, this PR is LGTM. The chmod +x issue seems related to Windows + Podman.
Btw during my test I will add chmod +x.


Can these warnings during build be ignored?

...
+ apk del --no-network .me-fetch-deps
WARNING: opening from cache https://dl-cdn.alpinelinux.org/alpine/v3.18/main: No such file or directory
WARNING: opening from cache https://dl-cdn.alpinelinux.org/alpine/v3.18/community: No such file or directory
...

@BlackthornYugen
Copy link
Member

Can these warnings during build be ignored?

I think so. It looks like it's just a warning that the cache for these repos cannot be found, but that's expected since we always install with --no-cache.

@BlackthornYugen BlackthornYugen merged commit ef54f9d into mongo-express:master Jan 4, 2024
@tianon tianon deleted the multistageless branch January 4, 2024 17:11
@tianon
Copy link
Collaborator Author

tianon commented Jan 4, 2024

I'd be happy to also remove alpine3.17 and add alpine3.19, but didn't want to get ahead of myself 😄

We should probably keep PR's small if possible, I can make a new one for that after this gets merged. I am curious why alpine3.17 isn't working though... it should still be supported right?

That was dropped by node in nodejs/docker-node#2007 (because supporting too many entries in the matrix is a bit much, and there's not a lot of strong reasons to keep older versions of Alpine since the level of maintenance they receive declines steeply with time 😅).

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.

3 participants