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

Update dockerfile #665

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Update dockerfile #665

merged 1 commit into from
Dec 3, 2024

Conversation

turbo124
Copy link
Member

No description provided.

@benbrummer
Copy link
Collaborator

benbrummer commented Dec 1, 2024

I think we have have only the option to have a single nginx-php-fpm conatiner and only mount the storage volume
or have two containers mounting the appölication from https://github.com/invoiceninja/invoiceninja, after it has been added as submodule to this repo.

# Ensure symlink for storage/app/public
if [ ! -L /var/www/html/public/storage ]; then
  echo "Creating symlink for storage/app/public..."
  ln -sfn /var/www/html/storage/app/public /var/www/html/public/storage
fi

could be replaced with php artisan storage:link

If we go with a single container, we could go with frankenphp.

Update:

&& php artisan storage:link
is already takling care about the symlink so no need do handle it in init.sh

At the momment I think the "workaround" solution is to
run docker volume rm image_public when pulling a new version

@turbo124
Copy link
Member Author

turbo124 commented Dec 1, 2024

@benbrummer I wanted to avoid comingling nginx + php images as we've had criticism on that front before as it then is not optimal for a user who want to use caddy etc etc.

The submodule idea is good, but we have the added complexity here of merging the react project also. This would not be a deal breaker, but it does add some additional overhead I think. I've never tinkered with this kind of idea. However I think ultimately we are still in trouble as we will always need to expose the public/ directory even if :ro which then circles us back to the original problem where the volume mount excludes the images public/ dir....

@benbrummer
Copy link
Collaborator

benbrummer commented Dec 1, 2024

With the submodule, we only need to update the commit hash to get the new version. A merge is not needed.

Update: Got it react project is not part of the invoiceninja repo

@benbrummer
Copy link
Collaborator

benbrummer commented Dec 1, 2024

I wanted to avoid comingling nginx + php images as we've had criticism on that front before as it then is not optimal for a user who want to use caddy etc etc.

  1. Base image
    invoiceninja/app
  2. extend the base
    from invoiceninja/app => invoiceninja/app-nginx, incoiceninja/app-apache, incoiceninja/app-caddy...

@turbo124
Copy link
Member Author

turbo124 commented Dec 1, 2024

This has certainly gone beyond my experience levels with docker, so I am in your hands for this 😆

@benbrummer
Copy link
Collaborator

benbrummer commented Dec 1, 2024

I will try to come up with a proposal.

Can you tell me the purpuose of /var/www/html/public/uploads, https://laravel.com/docs/11.x/structure. As it is not in the storage or public/storage folder it will not persist. It is created at

RUN mkdir -p \

as it is not inside /var/www/html/public/storage, this folder is not persitently stored. Is it just a temporary upload location?

@turbo124
Copy link
Member Author

turbo124 commented Dec 3, 2024

I will try to come up with a proposal.

Can you tell me the purpuose of /var/www/html/public/uploads, https://laravel.com/docs/11.x/structure. As it is not in the storage or public/storage folder it will not persist. It is created at

RUN mkdir -p \

as it is not inside /var/www/html/public/storage, this folder is not persitently stored. Is it just a temporary upload location?

I believe public/uploads it is redundant.

I will merge this PR as a temporary solution so that as we tag new images, the files can at least update until a new master image has been created.

@turbo124 turbo124 merged commit 84dcb8c into invoiceninja:debian Dec 3, 2024
1 check passed
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