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

Add docker support #251

Merged
merged 5 commits into from
Sep 17, 2022
Merged

Add docker support #251

merged 5 commits into from
Sep 17, 2022

Conversation

KagurazakaNyaa
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Aug 30, 2022

CLA assistant check
All committers have signed the CLA.

@mdziekon
Copy link
Owner

mdziekon commented Sep 1, 2022

Hi @KagurazakaNyaa, thank you for your contribution! This addition will definitely greatly improve development experience for all potential contributors, myself included. Incidentally, this will also close an old issue #16, which is great.

I'll try to review & test this locally during the upcoming weekend, however one thing that already comes up to my mind about this is related to the webserver you've used in the Dockerfile. Seems like you've used Apache (https://github.com/mdziekon/UniEngine/pull/251/files#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R1). Since I'm not really experienced with Apache, initially I was thinking about using Nginx as the base webserver for the dev environment, mostly because I'm already familiar with it and use it professionally on a day to day basis.

Therefore, my question is - could you refactor your Pull Request to use Nginx instead of Apache? Locally, I've been using richarvey/nginx-php-fpm as a quick solution for a local dev environment, so that might be a good place to start. But... I haven't really done any research beyond that, so there might be something better.

@mdziekon mdziekon added pr:enhancement_request All pull requests related to enhancements proposals pr:first_contribution Label for all contributions made for the first time by a new contributor labels Sep 1, 2022
@mdziekon mdziekon self-requested a review September 1, 2022 18:13
@mdziekon mdziekon linked an issue Sep 1, 2022 that may be closed by this pull request
@KagurazakaNyaa
Copy link
Contributor Author

Ok, I'm using apache because it's a native tag for the _/php image, I'll modify it to a multi-container architecture, using php:7.3-fpm and nginx:stable.

@KagurazakaNyaa
Copy link
Contributor Author

I created a commit to use php:7.3-fpm and nginx:stable in place of php:7.3-apache, I tested it locally, but it may not be comprehensive. Maybe you can test further to determine if there is a potential bug.

I did not directly share /var/www/html between the two containers, I built the basic content in the composer image, and then mounted the parts that need to be persisted externally, and /var/www/html/tmp is shared using docker volumes. But given that the nginx container and the php container use different users, I'm not sure if there is a permissions issue, but in my tests locally it works fine.

@mdziekon
Copy link
Owner

mdziekon commented Sep 5, 2022

My initial review & testing of your PR shows no problems so far, however I didn't have enough time to test any places that could potentially be problematic with permissions (eg. places which save something to the drive, except for the installation script). I'd like to take some more time for testing, but overall I think it should be mergeable, once I'm done with the testing. Since I'm going on vacation in a few days, unfortunatelly this will have to wait until next week, sorry for that :(

In the meantime, if you don't mind, some things could be improved (I'll leave some comment / proposals). However, none of them are show stoppers, so if you don't feel like changing them right now, I can do that myself after we merge your original proposal.

Anyway, great job!

.dockerignore Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mdziekon
Copy link
Owner

mdziekon commented Sep 12, 2022

@KagurazakaNyaa I was finally able to test your PR more thoroughly, with the most recent changes. So far, the only problems I've found seem to be non-fatal, and are related to write permissions to some of the cache directories:

  • generate_sig.php script is unable to access & write anything related to Signatures, leading to broken Signature images (as seen in the Overview page).
  • admin/autostatbuilder.php is unable to write to cache/data/last_stats_update.php file. This does not break the Statistics calculation script, but throws a warning on each run, and if I recall correctly, this file is being used in the generate_sig.php file.

I haven't found other scripts that break like that, so right after fixing that (and the sudo comment in README), this PR should be ready for merge 🎉

add sudo on readme
@KagurazakaNyaa
Copy link
Contributor Author

Maybe the problem with the cache wasn't a write permission issue, but it wasn't properly shared between the two containers, which I solved by adding a shared volume. When I visit the overview, I can notice in the php log that /generate_sig.php has returned 200.

172.19.0.3 -  13/Sep/2022:09:03:30 +0800 "GET /generate_sig.php" 200
172.19.0.3 -  13/Sep/2022:09:03:28 +0800 "GET /overview.php" 200

@KagurazakaNyaa
Copy link
Contributor Author

generate_sig.php still seems to have issues, it won't find some files, I need further research to fix this, please don't merge for now, I'll create some more commits to fix it

@KagurazakaNyaa
Copy link
Contributor Author

The error in generate_sig.php is because the target directory does not exist correctly. In a non-cross-container environment, this is not a problem. But when we need to share a volume across containers, it may have some subdirectories that do not exist, so we need to do extra processing for this situation.

@mdziekon
Copy link
Owner

What's the current status of your work here? Does the "please don't merge" comment still apply?


As far as I can tell, the previous signature generation problem has been fixed, however now it thrown an error about a missing function:

Fatal error: Uncaught Error: Call to undefined function imagettfbbox() in /var/www/html/generate_sig.php:167

Looking at the code and the installed extensions in the Docker files, I have no clue why this is occuring, as all the necessary things are seemingly there.


As for the stats calculation problem, it's still there, however the "mkdir in PHP" approach seems to work on that problem as well.

@KagurazakaNyaa
Copy link
Contributor Author

If nothing else, I think it's ready to be merged. But I also have no clue about the problem with the imagettfbbox function, and if it does not cause any symptoms, I think it can be ignored for now.

@mdziekon
Copy link
Owner

I agree, signatures are a non-essential feature (and works fine on other environments), so merging it is still beneficial overall. I'll create a ticket for it, to be fixed in the future. As for the stats generation, that can be fixed in a separate PR as well.

@mdziekon mdziekon merged commit 77f8090 into mdziekon:master Sep 17, 2022
@mdziekon
Copy link
Owner

@KagurazakaNyaa PR has been merged, thank you again for your contribution!

@mdziekon
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:enhancement_request All pull requests related to enhancements proposals pr:first_contribution Label for all contributions made for the first time by a new contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add basic Docker / Vagrant configuration
3 participants