Update to 0.10.2, added logrotate, change url and fail2ban

YunoHost-Apps / shaarli_ynh

Shaarli package for YunoHost

GNU General Public License v3.0

20 stars 13 forks source link

Update to 0.10.2, added logrotate, change url and fail2ban #36

Closed lapineige closed 5 years ago

lapineige commented 5 years ago

Problem

The app is not up to date with the upstream.Fix #35

Solution

Update the package

PR Status

[x] Code finished.
[x] Fix or enhancement tested.
[x] Upgrade from last version tested.
[x] Can be reviewed and tested.

Validation

Minor decision

Upgrade previous version :
[x] Code review :
[ ] Approval (LGTM) :
[ ] Approval (LGTM) :

When the PR is marked as ready to merge, you have to wait for 3 days before really merging it.

anmol26s commented 5 years ago

@lapineige

Sources have to be updated here: https://github.com/YunoHost-Apps/shaarli_ynh/blob/testing/conf/app.src

lapineige commented 5 years ago

Oh thanks for the reminder.

anmol26s commented 5 years ago

@lapineige Upgrade was giving warning: WARNING ./upgrade: line 116: [: -eq: unary operator expected So fixed it by changing the is_public to latest Yunohost package and removed unnecessary helpers in common.sh

anmol26s commented 5 years ago

Can someone review the changes and test the app? Any help to fix check_url would be appreciated.

anmol26s commented 5 years ago

Have a look at YunoHost-Apps/nextcloud_ynh#138. We need to remove php.ini and merge its content to pool file.

anmol26s commented 5 years ago

@lapineige @Rafi594 Can you review and test the testing branch? The Ci is having some issues with change_url, so its failing. This can be solved after the Ci is fixed. For now this could be merged after test.

lapineige commented 5 years ago

Upgrade works, new features too. I've not tested fail2ban.

Change url work, but I don't understand why we have this warning with change_url : we never use it, don't we ?

anmol26s commented 5 years ago

I did test by entering wrong password 5 times and fail2ban worked for me. Install,upgrade,backup and restore are working too. Regarding change_url issue see this https://github.com/YunoHost/issues/issues/1217. We can merge this and fix the fail build after this issue gets resolved in next release.

lapineige commented 5 years ago

LGTM :)

lapineige commented 5 years ago

We need to remove php.ini and merge its content to pool file.

It seems to raise an exception during backup:

Attention : Source path '/etc/php5/fpm/conf.d/20-shaarli.ini' does not exist

anmol26s commented 5 years ago

Indeed, I forgot to remove php.ini backup and restore commands. Thanks for the hinting that.

lapineige commented 5 years ago

@Rafi594 could you do an additional test (at least install and/or upgrade) ?

Then I think we can merge :)

anmol26s commented 5 years ago

Any update on this?

lapineige commented 5 years ago

@Rafi594 if you have no objection, I'm ok to merge :)

anmol26s commented 5 years ago

I think we can merge.