search

YunoHost-Apps / dokuwiki_ynh

Dokuwiki package for YunoHost
https://www.dokuwiki.org/dokuwiki
GNU General Public License v3.0
11 stars 16 forks source link

Refactor #37

Closed Gofannon closed 6 years ago

Gofannon commented 6 years ago

Problem

Solution

Package has been tested on a VM with copy of my wiki There is still "legacy code" that should be checked later

PR Status

Validation

Minor decision

maniackcrudelis commented 6 years ago

Good job, thanks for this huge contribution. If you need or would like any help, don't hesitate to ask.

Gofannon commented 6 years ago

Thanks! Where and how can I do a feedback about the packaging process ? There are lacks in the documentation about how to manage files in "conf" folder for example. Should I modify them in the conf folder and then copy them in the $final_path or copy file in $final_path and then modify them there

maniackcrudelis commented 6 years ago

You can do that right here, when you want. Any feedback would be welcome.

Should I modify them in the conf folder and then copy them in the $final_path or copy file in $final_path and then modify them there

Since a few time, because the conf folder will be kept for later uses (especially in change_url), it's better to copy your file in the right place before modifying it.

Gofannon commented 6 years ago

  1. I didn't test manually the new commits !!

  2. package_check does not pass for step >> Upgrade from Create check_process... [Test 5/10]

I don't know why. Any idea ?

>> Upgrade from Create check_process... [Test 5/10]

Preliminary install...
Start the LXC container.
Create witness files...
Working time: 11 seconds.
Installation successful. (0)
Warning: moulinette.interface __call__ - « yunohost app checkurl » est déprécié et sera bientôt supprimé
Warning: yunohost.hook <lambda> - [1006.1] « yunohost app checkurl » est déprécié et sera bientôt supprimé

Upgrade...
A LXC container is already running.
Working time: 7 minutes, 15 seconds.
Upgrade failed. (61)
Warning: yunohost.app app_upgrade - [1124.1] Mise à jour de l'application dokuwiki...
Warning: yunohost.hook <lambda> - [1124.1] 2018-06-28 00:02:20 URL:https://download.dokuwiki.org/src/dokuwiki/dokuwiki-2018-04-22a.tgz [3749191/3749191] -> "app.tar.gz" [1]
Warning: yunohost.hook <lambda> - [1124.1] md5sum: /var/www/dokuwiki/conf/local.protected.php: Aucun fichier ou dossier de ce type
Warning: yunohost.hook <lambda> - [1124.1] /usr/share/yunohost/helpers.d/setting: ligne 17: $3 : variable sans liaison
Warning: yunohost.hook <lambda> - [1124.1] !!
Warning: yunohost.hook <lambda> - [1124.1]   dokuwiki's script has encountered an error. Its execution was cancelled.
Warning: yunohost.hook <lambda> - [1124.1] !!
Warning: yunohost.hook <lambda> - [1124.1] Upgrade failed.
Warning: yunohost.backup mount - [2192.1] Le montage de l’archive de sauvegarde a échoué
Warning: yunohost.hook <lambda> - [1124.1] Le montage de l’archive de sauvegarde a échoué
Warning: moulinette.interface __call__ - « yunohost app checkurl » est déprécié et sera bientôt supprimé
Warning: yunohost.hook <lambda> - [2192.1] « yunohost app checkurl » est déprécié et sera bientôt supprimé
Warning: yunohost.hook <lambda> - [1124.1] « yunohost app checkurl » est déprécié et sera bientôt supprimé
Warning: yunohost.hook <lambda> - [1124.1] The app was restored to the way it was before the failed upgrade.
Error: yunohost.app app_upgrade - [1124.1] Impossible de mettre à jour dokuwiki
Error: yunohost cli - Aucune application à mettre à jour
  1. Need to rework on unix permissions for "install" and "upgrade"
maniackcrudelis commented 6 years ago

Package fails because of that md5sum: /var/www/dokuwiki/conf/local.protected.php: Aucun fichier ou dossier de ce type, you added local.protected.php in this PR, so this file didn't exist previously. Any upgrade will fail because this file doesn't exist before the upgrade. Your upgrade should consider that case.

Gofannon commented 6 years ago

Thanks. I didn't think about this case. It should be corrected now. package_check passes and I tested quickly on a VM by upgrading a wiki and it still works

Should be tested more carefully latter and correct file permissions

maniackcrudelis commented 6 years ago

Except that last comment, all seems good to me in your code.

JimboJoe commented 6 years ago

This heavy rework looks very good (and very well commented) to me! Thanks a lot for your contribution :heart_eyes:

Gofannon commented 6 years ago

Testing

Package has been tested on a virtual environment. I do not use it daily Here is a summary of what I tested and how

package_check

No errors reported

Installation

Yunohost runs on jessie VM installation with sub domain and on "/"

Access to wiki URL Edit landing page Add new page and upload a picture inside

Access to "admin panel"

Admin > Extension Manager Search and install a plugin (Example : OpenOffice.org/LibreOffice.org Export) Search and install a template (example : Bootstrap3 Template)

Admin > Configuration Settings Change lang Change template to the previously installed like Bootstrap3

Check permission inside "lib" folder

root@example:/yunohost/dokuwiki_ynh# ls -al /var/www/dokuwiki3/lib/index.html -rw-r--r-- 1 root root 241 May 3 08:36 /var/www/dokuwiki3/lib/index.html

Upgrade

Upgraded the test wiki Upgraded the copy of a production wiki

It works I guess :)

There is an issue with plugin "Site Export Manager" as it needs write access to inc/preload.php (I did not check during the "install" step and the issue should be there too)

Could not create/modify preload.php. Please check the write permissions for your DokuWiki/inc directory.

Shall I create a preload.php too ? It could create potential security issues but it could annoy users too if something does not work as expected

See

JimboJoe commented 6 years ago

Thanks for that extensive testing and report!! Did you test any upgrade from the current version in master by any chance? I would vote in favor of creating the preload.php file so that everything gets as smooth as possible for users...

Gofannon commented 6 years ago

Did you test any upgrade from the current version in master by any chance?

Yes with step Upgraded the copy of a production wiki To explain it with more details : On the VM, there is "copy" of my production server. I installed a fresh Debian Jessie, then installed Yunohost and then imported the backup of production server by running yunohost backup restore XXX So the app Dokuwiki should be in the same state and condition as on the production server.

This makes me think : Is there any way of writing a "test scenario" with all the important use case to test and validate an application ? My way of using DokuWiki might not be common to other people so while testing, I might miss something.

I would vote in favor of creating the preload.php file so that everything gets as smooth as possible for users...

Me too in fact. I didn't want to influence your choice by giving mine. For me, in this case, user friendliness is more important than security.

Gofannon commented 6 years ago

change_url

I could not manage to implement change_url using information from https://github.com/YunoHost/example_ynh/commit/d3f411736981ea2b748b9a1459429b4653556bc7

  1. Is there something else to do to make it work ?
  2. Shall the Yunohost minimum version be updated to a new version : https://github.com/YunoHost/example_ynh/blob/master/manifest.json#L18
  3. Is there a handler to update ? (see test log below)

Shall I commit and push the script even if it does not work ? I do not know how to create a branch in this case (create from master or from refactor ?)

Testing

How I tested

After test2, I get :

# head /etc/nginx/conf.d/wiki.example.org.d/dokuwiki.conf

#sub_path_only rewrite ^/$ / permanent;
location / {

  # Path to source
  alias __FINALPATH__/ ;

  if ($scheme = http) {
    rewrite ^ https://$server_name$request_uri? permanent;
  }

That's why I think that there should be something in ynh_add_nginx_config to correct

maniackcrudelis commented 6 years ago

It's just that you need to load final_path as you can do in other scripts, https://github.com/YunoHost-Apps/dokuwiki_ynh/pull/37/files#diff-5eed0b3fcd28fe6d787e5405728a2ca8R19

Gofannon commented 6 years ago

Thanks for the input. After adding it, it does not work and I don't understand why :/ I looked at the log of package_check without much help (I can publish the log somewhere if needed but I guess that the CI will produce the log anyway ?)

I pushed the commit to the repo anyway so people could have a look at the issue too

Gofannon commented 6 years ago

Well, it works on the VM :)

I did the following tests

same domain + sub path

change domain + sub path

old_domain + /test => new_domain + /

maniackcrudelis commented 6 years ago

All seems good to me. Just one last thing, if you put the yunohost requirement at 2.7.14 (last jessie version), you can remove any reference to #sub_path_only in nginx into your install script, as well as upgrade and change_url scripts. This fix, alias_traversal, is now handled by the helper itself.

Gofannon commented 6 years ago

Oh, I will change that then. Is there any place where this kind of information has been published by any chance? A changelog or a post on the forum ? I found this and it seems a little bit outdated : https://github.com/YunoHost/yunohost/blob/master/debian/changelog

Or said in an other way, how can I "follow" these changes ? https://github.com/YunoHost/yunohost/releases is not really helpful

I had planned to commit these "fixes" in the example_yhn app

maniackcrudelis commented 6 years ago

The changelog is here, https://github.com/YunoHost/yunohost/blob/stable/debian/changelog, but there's nothing about this change, just an entry for the PR https://github.com/YunoHost/yunohost/pull/462.

The example app should indeed been updated.

Unfortunately, we do not have any official channel for that kind of information, neither do we have anyone to take care of that.

Gofannon commented 6 years ago

I cannot test anymore with package_check after changing https://github.com/YunoHost-Apps/dokuwiki_ynh/pull/37#issuecomment-404597751

Error: yunohost cli - Les pré-requis de dokuwiki ne sont pas satisfaits, le paquet yunohost (2.7.10) doit être >> 2.7.14 Installation failed... --- FAIL ---

maniackcrudelis commented 6 years ago

That means your container isn't up to date. It can happens if you're running package check on a virtual machine, otherwise the container should be updated every night. Anyway, to update it, run package_check/sub_scripts/lxc_upgrade.sh

JimboJoe commented 6 years ago

Just updated my production instance with your branch. I'm using a custom theme, plenty of plugins (some of which were outdated)... The upgrade went fine. I got warning messages due to outdated "wrap" plugin: I just updated every plugin through the admin interface and everything was fixed. So, that looks very good for me, thanks again!! :+1: :heart:

Gofannon commented 6 years ago

Glad to know for it worked on your server 👍 I did not test it on mine yet and my computer does not work anymore at the moment. I need to fix it before being able to work again on this PR.

I should check about the configuration files changes I made (splitting the main config in two files (one for Yunohost LDAP and one for the user config)

This PR begins to be long. Maybe it's better to merge it in testing and I will do other PR if needed ?

Gofannon commented 6 years ago

I will also do the post in the forum in the dedicated "app section" if it is okay for you ? To announce a testing version for Dokuwiki and the rest

JimboJoe commented 6 years ago

Well, as we have all the required reviews, I propose to merge in 3 days, unless someone steps in :wink:

JimboJoe commented 6 years ago

@Gofannon as you proposed that, could you possibly create the official Dokuwiki forum topic and advertise your massive update and the new testing? :sunglasses:

Gofannon commented 6 years ago

@JimboJoe done. See https://forum.yunohost.org/t/official-app-dokuwiki/5366

Thank you for your support and patience to all of you (@maniackcrudelis , @JimboJoe, @Psycojoker)

JimboJoe commented 6 years ago

Thank you so much for your work and your very consistent/precise approach, that is much appreciated! :+1:

I've added a link to your topic in the Official apps index on the forum.