JimboJoe
commented
7 years ago Processed points
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/conf/nginx.conf#L3 Are you sure it works without a equivalent setting in your php config ? Did you already test it ?
Just settled to 1h and 100Mo max, set on both nginx and PHP-FPM sides. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/15e1300f64b51c1918959e2af4c7eeac768666bf
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/backup#L25 Can you use the final_path stored in the config instead of set it here. You store this value in your instal script.
Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/81247871f786213f8542f35706a95d4d7c3cecd4
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/install#L123-L173 Can you add here more "big" comment to structure the code.
Added. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/8d061f1651ad36baaea955f3e2ef36e55fc79a22
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/remove#L4 I think it's really a bad idea, if this script fail, the app will be in a non-installed / non-removed status. So, you can't remove it, and you can't reinstall it.
Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/6780bd4a758f69a1339d4aceb41e8515a4d04f68
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/remove#L39 Are you sure you really want to remove all the pictures and photos of the users ?
Not really :wink: Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/6780bd4a758f69a1339d4aceb41e8515a4d04f68
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/remove#L54-L56 The previous helpers reload their service.
Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/6780bd4a758f69a1339d4aceb41e8515a4d04f68
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/upgrade#L27-L69 Please more comments and blank lines ;)
I can maybe compact lines with ";"...? Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/89eddb9e13c04236b965583bc3f98869965538ca
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/install#L121 It's already done by the helper just above.
Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/90d3303b28466641a4cc2b668b4fbaf318a5e58f
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/install#L2 Are you sure you really need that ? You don't use any glob inside a double quotes. And it works fine without double quotes. If you need that, just explain a little more what is it.
You need it for this syntax cp -a $TMPDIR/!(upload|galleries) $final_path
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/install#L127-L129 Why a sleep here ? And these 2 reload are not needed, the respective helpers reload their service.
Tested without any sleep on x86_64 and ARM, seems to be fine now. Fixed. https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/90d3303b28466641a4cc2b668b4fbaf318a5e58f https://github.com/YunoHost-Apps/piwigo_ynh/pull/12/commits/2c824f4be81c0d1c307f2f62f920cff5dca1bab6
No, used later on: https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/upgrade#L170
- And same things than the install script for the upgrade script.
install and upgrade script are now on par.
Remaining points
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/change_url#L64 Didn't we change this line somewhere else to have a more simple version of this sed ? Or I just tried and didn't make it ?
It's the latest proposal in example_ynh... To be further discussed.
- https://github.com/YunoHost-Apps/piwigo_ynh/blob/master/scripts/install#L103 Can you use instead the Picture directory of yunohost.multimedia ? In order to not duplicate this media.
Images are stored in upload and _data directories, and images/thumbnails and intermediate sizes are mixed together... so I think it won't fit well with yunohost.multimedia.
maniackcrudelis
Because is about to become an official app, there are some things we have to discuss about some parts of the code. Let's go :)