Your application has been elected to become official !

[frju365]

Hello, Since the last week, the community members have voted to elect the next two future official applications. (https://forum.yunohost.org/t/elis-les-futures-apps-officielles-elect-the-next-official-apps/3484)

Today, we announce to you that your app has been chosen to become official. To be sure that you agree, we need a confirmation from you within 2 weeks.

By entering in the official group of application, you will have to keep maintaining your application. You will have to create PR before commiting to the master branche and your PRs will have to be reviewed by the Apps Group of Yunohost following our guidline. Of course, you will get help from the App Group to help you maintaining this application.

No confirmation of you means that you disagree with the decision of the community and another application will join the official group instead of ampache.

Thanks, The Apps Group

[aymhce]

Hello, i am happy to this good news. I'm not the initial creator but I'have done last updates. Of course I've agree. :)

[stevenroose]

Personally, I've abandoned this project. I don't use Apache anymore and I can no longer keep maintaining this. I'm sorry.

If aymhce is happy to do the job, perfect!

[frju365]

ok. So, @aymhce, do you agree with the fact of being the (official) maintainer of the application ?

[aymhce]

@frju365 No problem, during time that I'm user of Ampache :)

[maniackcrudelis]

Congratulation \o/

And now, it's time to suffer ! Mouhahaha

Indeed, this app can become official, but... Before that, there's a lot of work to do. In fact, we (together, not you alone) have to refactoring this app to fit to the official packaging standard.

But, obviously we will be here with you to do that. And let's do the things step by step. First, have a quick look at the example app. https://github.com/YunoHost/example_ynh/blob/master/scripts/install

The first thing to do is to give to your code some room, by separate each part with a big uppercase comment (like the example app above). It's quite simple to do, but you have to do that because you know your code. After that, it will be really more simple for us to review the code and help you to go forward.

[aymhce]

Ok i look that for the few next days

[aymhce]

@maniackcrudelis First step done

[JimboJoe]

Thanks for that! Now a few more requests (don't hesitate to look at the example_ynh application):

• depend on YunoHost 2.7.2 to use latest helpers
• remove sudo prefixes
• set and use a final_path variable
• use the new ynh_backup syntax (e.g. ynh_backup "/etc/nginx/conf.d/$domain.d/$app.conf") use ynh_restore_file accordingly
• you don't need to [check the user existence](https://github.com/YunoHost-Apps/ampache_ynh/blob/master/scripts/install#L34 (it's done automatically by yunohost as you declared a user type)
• use ynh_string_random to generate your passwords
• use ynh_string_replace instead of sed substitutions
• use ynh_install_dependencies (and ynh_remove_dependencies) instead of apt-get update/apt-get install; this allows to remove unneeded dependencies without breaking any app when removing your app
• use ynh_secure_remove instead of rm -f; it prevents (now and in a future when someone modifies a script) from removing important files
• use systemd syntax: systemctl reload/restart <service>
• rename _common to _common.sh
• use ynh_setup_source (with app.src file) to download the application source)
• use ynh_local_curl to execute your curl call

I know it does look like a heavy Christmas list, but don't forget we're here to help! Don't hesitate to ask any question, and proceed step-by-step. You can report your progress here and call for intermediate reviews. You can also join our apps XMPP chat room 🙂

Good luck! 😉

EDIT : Don't forget to work on a separate branch, and open a Pull Request so that we can interact while keeping your application functional for the community 😉

[aymhce]

I have done a PR. Just, I haven't applying the 2 last points.

[JimboJoe]

Thanks for your work! That looks already much cleaner now! 👍

• depend on YunoHost 2.7.2 to use latest helpers --> OK
• remove sudo prefixes --> OK
• set and use a final_path variable --> OK
• use the new ynh_backup syntax (e.g. ynh_backup "/etc/nginx/conf.d/$domain.d/$app.conf") --> OK use ynh_restore_file accordingly --> OK
• you don't need to [check the user existence](https://github.com/YunoHost-Apps/ampache_ynh/blob/master/scripts/install#L34 (it's done automatically by yunohost as you declared a user type) --> OK
• use ynh_string_random to generate your passwords --> OK
• use ynh_string_replace instead of sed substitutions --> OK
• use ynh_install_dependencies (and ynh_remove_dependencies) instead of apt-get update/apt-get install; this allows to remove unneeded dependencies without breaking any app when removing your app --> Partially OK: you need to take care of existing instances: you should use the ynh_install_dependencies in the upgrade script as well, otherwise there will be an error at removal when executing ynh_remove_dependencies.
• use ynh_secure_remove instead of rm -f; it prevents (now and in a future when someone modifies a script) from removing important files --> OK
• use systemd syntax: systemctl reload/restart <service> --> OK
• rename _common to _common.sh --> OK

Some more comments:

• Use ynh_add/remove_nginx_config helper (you'll then also avoid reloading nginx service in your scripts)
• No need for set -eu/set -u in install/remove/upgrade script
• add this block in the restore script

For ynh_setup_source and ynh_local_curl, do you just need some time or do you need some explanations?

[aymhce]

Ok thanks. No problem for ynh_setup_source, ynh_local_curl, just a problem of time for me. I can continue this weekend or next week.

[aymhce]

Hello, I think the app is up to date with all the recommendations, except for set -eu because the app isn't ​​longer valid in package_linter ...

[maniackcrudelis]

Sorry, I had a lot of work on something else, thanks to JimboJoe to took care or you. You've done a wonderful job so far aymhce.

I see some things though:

• Please add the real license of ampache here https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/manifest.json#L11
• Level 4 can't be auto https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/check_process#L25. Either your app supports ldap and ssowat, so you should put at 1 (with an explanation), either your app doesn't support yet, put it at 0. Or there no support of ldap and it's not relevant, put it at 'na'.
• Why do you not keep the ssowat button in nginx ? https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/conf/nginx.conf#L51 It's a real question.
• That's my point of view, but I really prefer you avoid to use that kind of function in common. https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/_common.sh Because it's really not pleasant to read that kind of code. With a function which you have to find in another file.
• You can remove your set -eu at the beginning of the scripts. And don't care about linter.
• Please use source instead of . https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/install#L20-L21 Just to be more readable.
• And common before yunohost/helpers, because in case of conflict, officials helpers shall be load in priority.
• Please store also domain and path, https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/install#L42
• https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/install#L57 Not needed, just remove the password in ynh_mysql_setup_db call. The helper gonna create it, and store it in the config.
• Bad copy paste, https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/install#L72 And you can use ynh_string_random directly in your ynh_replace_string without using a variable.
• https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/_common.sh#L24 Not really a problem, but have a look to pushd/popd or, better, use parentheses to start a sub shell. In a sub shell, any cd will affect only this sub shell.
• What's this ? https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/scripts/install#L115

Sorry, other scripts for the next time ;)

[aymhce]

Ok, next week I'll work about your recommandations. Thanks for watching

[aymhce]

Hello, I currently have a professional mission that fascinates me and takes all my time. Don't worry, I will not forget ampache. But sorry for that.

[JimboJoe]

We completely understand that, take your time and thanks for your work 👍

[aymhce]

Hello ! So I'm back :) with a new commit. @maniackcrudelis can you verify it please ?

[maniackcrudelis]

Hey nice to see you back o/

There's some point though:

• You didn't answer me about that

  Why do you not keep the ssowat button in nginx ? https://github.com/aymhce/ampache_ynh/blob/0b3f9e2611bdf49b938b3ea235d93a5e59b6ad51/conf/nginx.conf#L51 It's a real question.

• Ok nice for the level 4, but can you link a proof about the support of ldap and sso ? Just to avoid to someone else ask also why this app has the level 4 at 1. we use to add a link or open an issue to explain that.
• It's a new thing we find out a few time ago. Can you apply this fix to your app please, https://github.com/YunoHost/example_ynh/pull/45

After that, I think is going to be all good :) (I think but I have not enough time right now to perform a complete test. Maybe this evening.)

PS: Just like that, I don't know what is "prettyphoto", but it encounter a lot of 404 in my test.

[maniackcrudelis]

Just like that, I don't know what is "prettyphoto", but it encounter a lot of 404 in my test.

That's because your nginx conf file is not correct.

[...]
        #enable subsonic api
        if ( !-d $request_filename ) {
                rewrite ^/ampacherest/(.*)\.view$ /ampacherest/index.php?action=$1 last;
                rewrite ^/ampacherest/fake/(.+)$ /ampacheplay/$1 last;
[...]
        rewrite ^/ampacheplay/ssid/(.*)/type/(.*)/oid/([0-9]+)/uid/([0-9]+)/client/(.*)/noscrobble/([0-1])/player/(.*)/name/(.*)$ /ampacheplay/index.php?ssid=$1&type=$2&oid=$3&uid=$4&client=$5&noscrobble=$6&player=$7&name=$8 last;
        rewrite ^/ampacheplay/ssid/(.*)/type/(.*)/oid/([0-9]+)/uid/([0-9]+)/client/(.*)/noscrobble/([0-1])/bitrate/([0-9]+)/player/(.*)/name/(.*)$ /ampacheplay/index.php?ssid=$1&type=$2&oid=$3&uid=$4&client=$5&noscrobble=$6&bitrate=$7player=$8&name=$9 last;
        rewrite ^/ampacheplay/ssid/(.*)/type/(.*)/oid/([0-9]+)/uid/([0-9]+)/client/(.*)/noscrobble/([0-1])/transcode_to/(w+)/bitrate/([0-9]+)/player/(.*)/name/(.*)$ /ampacheplay/index.php?ssid=$1&type=$2&oid=$3&uid=$4&client=$5&noscrobble=$6&transcode_to=$7&bitrate=$8&player=$9&name=$10 last;
[...]
        location ^~ /ampachebin/ {
[...]
        location ^~ /ampacheconfig/ {
[...]
        rewrite ^/ampacheplay/ssid/(\w+)/type/(\w+)/oid/([0-9]+)/uid/([0-9]+)/name/(.*)$ /ampacheplay/index.php?ssid=$1&type=$2&oid=$3&uid=$4&name=$5 last;
        rewrite ^/ampacheplay/ssid/(\w+)/type/(\w+)/oid/([0-9]+)/uid/([0-9]+)/client/(.*)/noscrobble/([0-1])/name/(.*)$ /ampacheplay/index.php?ssid=$1&type=$2&oid=$3&uid=$4&client=$5&noscrobble=$6&name=$7 last;

        location /ampacherest {
[...]

Otherwise, all seems to work as expected.

Do you need any help with all that ?

[aymhce]

Hello @maniackcrudelis

• First, about the ssowat button, Ampache isn't designed to integrate it ... It hide some important elements.
• For explain level 4=1, you want the ldap integration portion code ? Also, where I can link this information ?
• Ok for YunoHost/example_ynh#45, I'll commit a fix
• For incorrect nginx conf, yes, can you help me please ?

[aymhce]

Ok I see the problem for nginx, sorry it been a while since I didn't work on ampache

[maniackcrudelis]

For the level 4, in fact I can see it works with my test, but, if there any code you can link, it's really better. You can simply, if it's only one link, put it just before the level 4 in the check_process file, with a # to comment it.

I did a complete test with Package check. It's almost all ok. I still have a problem with prettyphoto and also with some .js files in ampache/lib. And there're a lot of warnings from composer.

I think we can do something about that. I expect to have some free time tomorrow and saturday afternoon. I can try to help you about those problems.

[aymhce]

Ok thanks for feedback. I'll add a link for level 4. Also, I don't test manually my apache pkg yet. It will done tomorrow evening. So I will see all issues that you mean.

[maniackcrudelis]

I can't make any PR on your repository, I don't know why... So for the warnings print by composer, here a fix:

@@ -25,7 +25,8 @@ ampache_ynh_install () {
25          cd $final_path
26          php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');"
27      php composer-setup.php
28  -       php composer.phar install --prefer-source --no-interaction
29  +           php composer.phar update --no-interaction --no-dev
30  +           php composer.phar install --prefer-source --no-interaction --no-dev
31      # Set permissions to ampache directory
32      chown -R www-data: $final_path
33  )

[maniackcrudelis]

So... for the 404 errors, you can try to connect to that: https://domain.tld/ampache/lib/components/prettyphoto/images/prettyPhoto/default/loader.gif

The error seems to be in this part of the nginx conf:

    #enable subsonic api
    if ( !-d $request_filename ) {
        rewrite ^__PATH__/rest/(.*)\.view$ __PATH__/rest/index.php?action=$1 last;
        rewrite ^__PATH__/rest/fake/(.+)$ __PATH__/play/$1 last;
    }

But if you remove that, it doesn't work neither and you're going to fall on the portal. I guess it would be something bad in these rewrites. But I don't know why...

[aymhce]

Thanks for help. About the 404 errors, there is an issue from ampache source code...

• example: CSS are in error ...

  docker@yunohost:/var/www/ampache# fgrep -R pretty lib/components/require.css | head d -1
  div.pp_default .pp_top .pp_left { background: url(prettyphoto/images/prettyPhoto/default/sprite.png) -78px -93px no-repeat; } /* Top left corner */

• ... because, the searched file don't exist ...

docker@yunohost:/var/www/ampache# ls lib/components/prettyphoto/images/prettyPhoto/default/sprite.png
ls: cannot access prettyphoto/images/prettyPhoto/default/sprite.png: No such file or directory

docker@yunohost:/var/www/ampache# ls lib/components/prettyphoto/                 
css  js  prettyphoto-built.css  prettyphoto-built.js

[maniackcrudelis]

Ok, let's forget those 404 then.

It's all good for me. Let's merge your PR, see what the CI says. And make the ultimate PR \o/

[frju365]

it's a very good work, @aymhce !

[aymhce]

Hello, builds are done and ok,

• https://ci-apps.yunohost.org/jenkins/job/ampache%20(Community)/28/
• https://ci-apps.yunohost.org/jenkins/job/ampache%20(Community)%20(~ARM~)/14/

Go to do the last PR