Ensure that various directories are writeable

CanastaWiki / Canasta

MediaWiki Docker image for Canasta, an all-in-one MediaWiki stack for easy deployment and management of enterprise-ready MediaWiki on production environments.

https://www.canasta.wiki

MIT License

38 stars 28 forks source link

Ensure that various directories are writeable #413

Closed naresh-kumar-babu closed 4 months ago

naresh-kumar-babu commented 4 months ago

Partial Fix #388 .

yaronkoren commented 4 months ago

This looks good - although I would change the commit message, because this patch does not add the function make_dir_writable() (the previous patch already did that).

naresh-kumar-babu commented 4 months ago

This looks good - although I would change the commit message, because this patch does not add the function make_dir_writable() (the previous patch already did that).

Updated.

yaronkoren commented 4 months ago

Sorry for the delay. I have two comments:

This is pretty minor, but I think a better wording in the commit summary would "Ensure that various directories are writeable...", rather than "Make various directories writeable...", because most of the time make_dir_writable() does nothing. (Maybe the function should have a better name, given that, but that's another story.)
I think I never noticed before that the Wikiteq code contains that tiny update-images-permissions.sh script. It seems like a silly script - it basically just calls one line of code. Even if you add in the print command and that (maybe pointless?) sleep command, that's still just line three lines of code. I think it would be better to put this code directly into run-all.sh.

naresh-kumar-babu commented 4 months ago

Sorry for the delay. I have two comments:

This is pretty minor, but I think a better wording in the commit summary would "Ensure that various directories are writeable...", rather than "Make various directories writeable...", because most of the time make_dir_writable() does nothing. (Maybe the function should have a better name, given that, but that's another story.)

I think I never noticed before that the Wikiteq code contains that tiny update-images-permissions.sh script. It seems like a silly script - it basically just calls one line of code. Even if you add in the print command and that (maybe pointless?) sleep command, that's still just line three lines of code. I think it would be better to put this code directly into run-all.sh.

Done

naresh-kumar-babu commented 4 months ago

This looks better, but see my comments.

Updated.

yaronkoren commented 4 months ago

@naresh-kumar-babu - sorry, I have to apologize! I realize now why they created that separate update-images-permissions.sh file, even though it's so small - it's so that the rest of the setup process can still happen while make_dir_writable() runs across all the updated files/images. (If the code is in a separate script, it will run concurrently/asynchronously, instead of having to finish before the rest of the code runs.) There is a comment that explains this, although I think it's not that clear.

Please restore the code to back to what is was - sorry about the extra work. Although if you could make the comment clearer, that would be helpful. Also, a minor point I realized later: two of their print commands misspell "MediaWiki" as "Mediawiki".

naresh-kumar-babu commented 4 months ago

@naresh-kumar-babu - sorry, I have to apologize! I realize now why they created that separate update-images-permissions.sh file, even though it's so small - it's so that the rest of the setup process can still happen while make_dir_writable() runs across all the updated files/images. (If the code is in a separate script, it will run concurrently/asynchronously, instead of having to finish before the rest of the code runs.) There is a comment that explains this, although I think it's not that clear.

Please restore the code to back to what is was - sorry about the extra work. Although if you could make the comment clearer, that would be helpful. Also, a minor point I realized later: two of their print commands misspell "MediaWiki" as "Mediawiki".

Updated.