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
Create default Storage folders #228
Comments
Will do in the future. I know this is one of the issues blocking us from adding it to cosmos. |
I'm busted :p |
Sorry, there are just so many things to do, it will take a lot of time to address all the requests. What other issues are there that prevent linguacafe to be added to Cosmos? I remember copying into the dictionaries folder being one. |
NP i was just investigating :) |
Docker permissions have been problematic for us when using this approach, most notably you wouldn't be able to put dictionaries in folders created by the docker process unless you do it as root, hence why simjanos mentioned the dictionaries. Also Laravel logs, I should go back to that detail this week.
Ideally the only interaction outside the app would be to create/download a |
What is the problem with laravel logs? It's not a directory users copy into. 2 people also mentioned the 777 permissions, which is there because of the shared temp folder between the two containers. v0.12 will probably also have a fonts folder inside storage, I will use file upload for it.
Please take your time with it. I only asked so I can understand it, it's not urgent. |
No but it is a folder Laravel writes into, which runs as its own user instead of the root user. So unless you |
Are users expected to copy those files manually? Or is it a script doing it? When I did the setup I just had dictionnaries pre-installed I think I am assuming your main process is running as root in the container? |
Yes, as per setup instructions in the wiki.
The webserver (Laravel) runs as its own user, the python service does run as root. |
Not sure if it's useful, but Laravel has its own docker tools. At the time I wrote the docker file myself to learn about docker. |
I have managed to "skip" the
If I create the storage folder before mounting it I still have problems, specifically:
Which takes us back to OP's request to create the folders if they are not present. I also don't know what would happen if you try to run this as a root like some run their home servers. |
Can we create them in the entrypoint, and set the permissions for them?
Would this be a custom docker setup? Can I do something to help with this issue, other than converting the dictionary system? |
Not a solution for that bit since I meant the storage folder on the host that gets mounted as a volume, not the one inside the container. However, this might work for the subfolders inside, will tinker with this idea a bit.
Not really, I just meant logging in a server as the root user and running the docker commands to start the service. People shouldn't do that but that means someone out there is doing it.
I am not sure, I think I can handle the rest and you also have your own backlog of things to do. I will keep you informed if I make progress. |
I think this change from copying the files to uploading them will be confusing for some users, because they won't check the update notes. Should I use a new directory for the dictionaries, like |
I think the Ideal solution would be to keep supporting copying the files for a couple of versions but mark it as deprecated in the wiki and encourage the file upload instead, using a single folder for everything. New users will use the new method and old users won't be affected . By the time the old method gets removed it would have been several weeks or a few months so asking them to check the wiki won't be as annoying. |
I can change the name of the logfile in laravel config, so it will create one with the correct permissions. |
I think I have been able to make it work by both having the process inside the container run as non root and by using a named volume in the compose file. The good:
The bad:
The ugly:
See the drafts at #238 and #239 and let me know if they are solid, and I am building a test image from my fork if you want an easier time testing it. I also want to take this chance to hear what does @azukaar think of this solution, if it meets his expectations, and if there are other blocking issues from this project being published to the Cosmos store. |
Thank you so much! I've read it all, I will take a deeper look at it later today. I am working on font type management now, will finish it in a few days. After that I will only change small things, and will release v0.12 sometime in May. I'll merge the 2 PR-s in afterwards if that works for you. Probably will make it its own update. |
Yes, that is fine by me, I will probably keep testing all the edge cases to make sure they are properly documented. |
I will finish the font type management system today. What should I do with the 2 default font files for v0.12? Should I just upload them to the deploy branch? |
I haven't actually followed that feature, but if it makes sense for them to be mounted, because the location must be accessible for the other container or because user content will be persisted there, go ahead. |
That is the reason. I was asking because we will copy these files from the image to the mounted folder automatically in the future if they are missing, and wanted to know if I should implement that now (and if yes, where/how should I do it), or should I just do it with the old deploy branch method. |
If you don't mind delaying the release a bit I can test if placing the files in the storage folder of the repo and building the image works on already existing installs, since that is one of the cases I want to test. I know it will work with new installs just like the default book cover gets placed in its own path just fine. |
I Read The FM:
But I also have no idea how it would work with already existing files for existing users which we want to keep.
If there were no further things to add, I wanted to release it next friday (17). I'm okay with pushing it back as much as it needed. Does this mean that you will want me to merge the the 2 PR-s for v0.12?
Can I leave the testing of the docker process changes to you, and is it okay if I only test it once? My laptop would freeze a lot during it. |
Yes, this is specifically what I want to test. Where would the font files go? I don't see them in
This time frame sounds reasonable, weekend should be plenty for me to test. Maybe not to fix, in which case I will be notifying you to go with the original plan.
If all goes well, yes. However there is still the error that pops up when installing new languages even when the Python service returns code 200 that should be addressed before this goes live; it only happens after my changes to the image. I have a test image at
I think that's fine, I have gotten more through on my testing as I get used to all the corner cases I have to be careful about. |
"linguacafe/storage/app/fonts/". The 2 noto font files would be the default font files from "LinguaCafeDev/public/fonts", but without any subdirectory.
I forgot about that. Since you said it is on the PHP side, I will fix this one.
In that case I will have to rewrite the dictionary upload part. I think v0.12 might will be ready around the end of the month instead.
Thank you so much! I really appreciate this. This whole docker task would have taken me months to do by myself. @azukaar |
Well I have tested it and the fonts folder wasn't added to the already existing volume when switching the image so there goes my plan. I works with new installs but old ones would have to add the folder manually and that's not a good user experience. I was also reflecting: if the fonts already exist So I think we are back to the drawing board. OP's initial proposal of having the service check if the folder exists on startup and create it with fitting permissions if it doesn't is probably the best for the user since it gets rid of the git step and allows us to simplify to a single I'm sorry if it sounds like I'm evading this task and handing it back to you, but I think that's the long term solution that keeps compatibility and does not bother the users too much when updating. |
They won't be duplicated, they will be moved.
Flag images are not being extended by users, there is just a set number of them, so I used the public folder. Fonts and cover images are uploaded, so they must go to the storage folder. Since every user uploaded font and dictionary goes to the storage folder, I saw no reason to separate the default ones to the public. I would also have to add a few lines of code so if the default file is requested, the program would have to retrieve it from a different place. In the case of fonts, I would also have to rename default files so I can identify them by filename, and handle uploaded files that contains the "default" in their filename. Which is not a big deal, I just saw no reason to do it differently, because this was the simplest way. Would it be better to use the public folder?
Not at all, I'm happy to help if I can. But honestly, I do not understand how copying from the image to the volume should work. Where should I create the empty folders and copy the empty files in the startup process? I think we could do it in entrypoint.sh. The part that I don't really understand: where should I copy the files from to where? If I understand it correctly, we will store the default empty folders and files in the storage directory, which will be mounted as a volume. I'll try to replicate the docker changes to the dev environment. |
I think so. It is good practice to have your own assets separate from the ones provided by the user. Currently the docker images are technically not self contained since they need the default book cover to be mounted on a volume, and users can modify or remove it which beats the purpose. Defaults should be read only to ensure they can fulfill their role of being a something you can fall back to if everything else fails.
Oh no, this is what I tried to do with the named volume and while it worked on a fresh install it fails to add new folders when updating. My proposal was to have PHP check if the subfolders on the storage directory exists before trying to write to them, and if they don't then create them to prevent any errors. If new folders are needed they would be created by the container without user interaction. However your idea of adding it to the entry point makes even more sense since it would run on startup, I will give it a go once I'm back from work. |
Those are very good points, I'll make these changes for v0.12. I'll make sure that linguacafe will work without these folders:
I think /storage/framework and /storage/logs folders and their subfolders will have to be created without Laravel, before anything Laravel related code runs.
Thank you so much! (But please only work on it if you feel like it. The discussed dates for the next updates are arbitrary, I'm okay with however long things take.) |
Well now those are created at startup from the entrypoint before the Apache server is launched so you can skip that if you want.
Those are created too ;) I closed the PR to the The |
So the sudo chmod is not needed at all, or is it still required to update from v0.11 to v0.12?
I forgot about that!
In that case, I fix the fake error message, move the default files, and everything will be finished for v0.12 today on my part. I will keep using and testing it until that. But again, we can release whenever, there are no deadlines. The windows bat install file will also have to be modified, but since the deploy branch won't change anymore, it can stay the same for a while.
We can just do a smaller v0.13 update when it gets ready. I probably will be working on code staff that does not affect users, and small UI changes. Thank you so much for this! This will be a huge step towards a standard docker process. |
I've updated my dev environment again, because it threw an error. I copy-pasted the production one, and removed 2 lines from it. I found no errors. I created a dev image, pulled, started the server, used sudo chmod 777 -R on the storage folder, and found no error in that so far either. I think everything is ready for v0.12, the only thing missing is user manual and readme. |
I updated the readme file with the new process. Also removed parts that were duplicated in the user manual. Can you please take a look at it, and check if everything is correct in the installation and update sections? Is the windows script still usable? It still pulls the deploy branch. I would prefer to update that in v0.13. I'm also not sure if we will need that anymore, because the whole process became literally 1 or 2 commands. I assume windows won't need the Thank you again for working on this, this will be a huge update! I plan on releasing on Friday morning if there won't be any new issues. I've been using it, and found no problems. |
I think if we are going to point them to download the compose file we might as well place it into the dev/main branches too. We removed it from there because people tried to use it when they couldn't but nowadays we actually want them to do that. Also remember we only need to affect the storage folder so the command should be
If that is your intention then yes, it doesn't need any changes. For next release we could update it to merely check if docker is installed and running and to create the
I don't really know, I'm not familiar with running docker in Windows. If we never encountered issues then we probably won't this time either, and if we do we at least know the solution. |
I changed that based on your suggestion. I also noticed
Fixed that. |
On windows Laravel threw errors about not having permission for the log file. The user deleted the storage/logs and storage/framework folders, and it worked afterwards. Surprisingly it did not have any problem with the storage/app directory. We tested book cover images, fonts, dictionaries and ebook imports, it had no problem with any of those. Edit: |
I am going to replace the dictionary file copy method with file upload in the next few days. |
I changed the dictionary import process. Users now can upload the dictionary files, instead of having to copy them into the storage folder. I think this was the last problem in the code that blocked linguacafe from being added to Cosmos. I will test it again on my personal production linguacafe server before I release v0.13. I'll also create a dev image. |
The app should create the storage folder itself upon first start rather than having to mount a bunch of empty folders manually
this way you can let people manage their folders easily via customizing the mount.
Note that without that it's impossible to use a volume for the mount without crashing the app
The text was updated successfully, but these errors were encountered: