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

Speed up Docker build and fix entrypoint permissions #894

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

PseudoResonance
Copy link
Contributor

The Docker entrypoint script runs artisan commands as root, which creates directories under storage/framework/cache/data that are owned by root. This causes errors when running the panel, as it's unable to write to parts of the cache. By running the commands as www-data instead, the permissions are kept unaffected.

I added the Dockerfile to the compose file, so that the image can easily be built from source if desired.

As the yarn install command can take a long time, I changed the Dockerfile to copy just the necessary package/yarn files for the installation first. Once that is completed, it copies the rest and builds the app. This sped up cached builds on my computer by about 80 seconds.

Copy link
Contributor

github-actions bot commented Jan 8, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@PseudoResonance
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@RMartinOscar
Copy link
Contributor

RMartinOscar commented Jan 8, 2025

We should run the entrypoint it self as www-data instead of forcing www-data on each commands.
I believe this change would be in supervisord
Or we can use su www-data on top of the entryscript (not sure about that tho)

Also now that we are using vite yarn needs to access vendor

import preset from './vendor/filament/support/tailwind.config.preset'

@PseudoResonance
Copy link
Contributor Author

I didn't think running the whole entrypoint script as www-data would be good as entrypoint itself runs chown at the end, seemingly as a final attempt at making sure everything is good.

@PseudoResonance
Copy link
Contributor Author

Also now that we are using vite yarn needs to access vendor

import preset from './vendor/filament/support/tailwind.config.preset'

It shouldn't need it to install the dependencies though, right? I don't know much about Vite.

@RMartinOscar
Copy link
Contributor

Also now that we are using vite yarn needs to access vendor

import preset from './vendor/filament/support/tailwind.config.preset'

It shouldn't need it to install the dependencies though, right? I don't know much about Vite.

It does require it to build and its been changed to yarn build too

@PseudoResonance
Copy link
Contributor Author

It does require it to build and its been changed to yarn build too

Ah, I see now. I think I got it working properly with this.

Copy link
Member

@lancepioch lancepioch left a comment

Choose a reason for hiding this comment

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

I've left you some feedback, thanks

composer.json Outdated Show resolved Hide resolved
compose.yml Outdated
Comment on lines 31 to 32
build:
dockerfile: ./Dockerfile
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this assumed by default, does this change anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just tested, and docker compose build or docker compose run --build will only build images with a build section specified. Although I realized build: ./ can serve the same purpose as this, if the Dockerfile has the default name.

# Copy the Caddyfile to the container
COPY Caddyfile /etc/caddy/Caddyfile
# Install dependencies with Composer
COPY composer.json composer.lock ./
Copy link
Member

Choose a reason for hiding this comment

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

Confused why this is necessary when we do the copy below?

COPY Caddyfile /etc/caddy/Caddyfile
# Install dependencies with Composer
COPY composer.json composer.lock ./
COPY app/helpers.php ./app/
Copy link
Member

Choose a reason for hiding this comment

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

This definitely confuses me even more. Definitely don't want to reference this file anywhere in Docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 3 files are the bare minimum required to run composer install. The reason app/helpers.php is required as well, is because it's specified as an autoload file in composer.json, so composer install --optimize-autoloader looks for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the reason behind splitting the install and build up like this is just optimizing for build caching. By only copying the dependency lists first, those first layers only need to rerun when the dependencies change. Then the entire codebase is copied as late as possible to minimize needlessly rerunning. GitHub actions can also be configured to utilize caching (although I didn't do that in this PR), which, with these changes, should help speed up the builds a bit.

Although this doesn't have as big of an impact anymore because of the Vite frontend rewrite. The majority of the build time is now taken up by yarn run build, but regardless, I tried to move it down as far as possible.

@lancepioch lancepioch added 🐛 bug Something isn't working 🟡 medium Somewhat challenging labels Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🟡 medium Somewhat challenging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants