Download sources during installation and upgrade to 1.0.36 #38

JimboJoe · 2016-12-27T18:18:35Z

No description provided.

…e to 1.0.35

Upgrade to 1.0.36

JimboJoe · 2017-01-02T09:39:49Z

Finally implemented dependencies download during install/upgrade with composer
Upgraded upstream version to 1.0.36

mbugeia

Hi, thank you for the package update, I've put my remarks but didn't test the package yet. Other thing LGTM.

In a more general notice while managing the dependency throught composer seems a good idea it not always the case. Composer take a lot (and I mean a lot) of RAM during install so the kanboard install will probably fail on small hardware. Related bug: https://dev.yunohost.org/issues/363 (plus there is a bunch of open bug for failed roundcube install because of this).
Did you notice that the link https://kanboard.net/kanboard-latest.zip always contain dependency while the github one don't ? Maybe it would be better to use this link and don't install composer at all. But I don't like that we can't point a specific version :/

mbugeia · 2017-01-02T10:10:27Z

scripts/install

-# Uncompress
-sudo unzip -qq kanboard.zip -d ..
+# Create tmp directory and install app inside
+TMPDIR=$(ynh_mkdir_tmp)


This is deprecated, use mktemp since it's bash native.

mbugeia · 2017-01-02T10:26:33Z

scripts/_common.sh

+    sudo sudo -u "$USER" $@
+  fi
+}
+


This is so over-engineered IMO. This complicate the script for exactly no gain, the whole script is run as "admin" user by YunoHost and this is used only to run thing as admin. Plus I don't think apps script should contain such generic helper, if one is really needed it should be in yunohost core.

mbugeia · 2017-01-02T10:27:03Z

scripts/_common.sh

+exec_composer() {
+  local AS_USER=$1
+  local WORKDIR=$2
+  shift 2


shift is useless here

mbugeia · 2017-01-02T10:30:54Z

scripts/_common.sh

+  # update dependencies to create composer.lock
+  exec_composer "$AS_USER" "$DESTDIR" install --no-dev \
+    || ynh_die "Unable to update application core dependencies"
+}


I think exec_composer and init_composer function should be merged since:

they are never called independently

they can be simplified by removing all the exec_as thing

mbugeia · 2017-01-02T10:32:52Z

scripts/_common.sh

+  local AS_USER=${2:-admin}
+
+  # install composer
+  curl -sS https://getcomposer.org/installer \


this is not the best way to install composer, the composer website provide a way more secure script here: https://getcomposer.org/doc/faqs/how-to-install-composer-programmatically.md

JimboJoe · 2017-01-03T19:55:11Z

Thanks for taking time to review the PR!
I took the roundcube app as a reference, which obviously seems like a bad idea... We clearly don't want to restrict hardware targets if we can avoid doing that.
You had a good idea to check the kanboard-latest.zip for dependencies... I blindly checked that URL: https://kanboard.net/kanboard-1.0.36.zip
And bingo! Dependencies are there!
I'll update the PR to revert to the first commit, updating to that link.

mbugeia · 2017-01-04T19:40:39Z

Cool, I didn't know it worked. Using the zip of kanboard.net with the dependencies was the way I updated the package before (as noted is the readme).
I look forward to your update, fetching the source and using a common script are good practices :)

JimboJoe · 2017-01-04T20:41:27Z

Just did another commit: now downloading Kanboard sources from kanboard.net website, avoiding the need to use composer.

mbugeia

Thanks again!

mbugeia · 2017-01-04T20:50:04Z

scripts/_common.sh

+  wget -q -O "$archive" "$APPLICATION_SOURCE_URL" \
+    || ynh_die "Unable to download application archive"
+  # Here we process with unzip as would tar "--strip-component" option
+  unzip -qq "$archive" -d "$DESTDIR" && rm "$archive" && f=("$DESTDIR"/*) && mv "$DESTDIR"/*/* "$DESTDIR" && rm -f "${f[@]}"/.htaccess && rmdir "${f[@]}" \


Wow! Why is this needed? I remember having to deal with the lack of strip-component option in unzip before but I don't remember how I solved it.
Here is how I handle the gogs zip archive for example: https://github.com/YunoHost-Apps/gogs_ynh/blob/master/scripts/_common.sh#L32-L47
If it's really needed please move it to its own function since it's not obvious how it work.

When looking for a substitute to --strip-component, I also stumbled upon rsync-based solutions. I do prefer using bash-only when possible.
But I agree it's ugly, so I'll make it cleaner in a specific function.

Oh ok so it was probably Gogs. But since rsync is installed in debian like unzip why bother? I prefer not to have complicated rm -f while rsync do the job very well.

mbugeia · 2017-01-04T20:52:36Z

scripts/install

@@ -46,9 +46,9 @@ ynh_app_setting_set $app adminusername $admin
 ynh_app_setting_set $app is_public $is_public

 # Create tmp directory and install app inside
-TMPDIR=$(ynh_mkdir_tmp)
+TMPDIR=`mktemp -d`


I'm a little picky here but could you use $(command) instead of backquotes theses are deprecated. Also consistency with the rest of the script.

OK, I fell into old habits here :-)

mbugeia · 2017-01-05T16:01:44Z

Ok for me, tested the upgrade on my production instance yesterday without issue. I'd like to have another person to test it before merging since it's an official app.

* Create check_process * Syntax fix * Download sources during installation and upgrade to 1.0.36 (YunoHost-Apps#38) * Download sources (except dependencies) during installation and upgrade to 1.0.35 * Download dependencies during installation Upgrade to 1.0.36 * Download kanboard zip from kanboard.net website * Take review into account

JimboJoe added 2 commits December 27, 2016 19:11

Download sources (except dependencies) during installation and upgrad…

5eadb53

…e to 1.0.35

Download dependencies during installation

6e4f673

Upgrade to 1.0.36

JimboJoe changed the title ~~Download sources during installation and upgrade to 1.0.35~~ Download sources during installation and upgrade to 1.0.36 Jan 2, 2017

mbugeia suggested changes Jan 2, 2017

View reviewed changes

Download kanboard zip from kanboard.net website

01f5b2a

JimboJoe force-pushed the master branch from 07afc0d to 01f5b2a Compare January 4, 2017 20:38

mbugeia reviewed Jan 4, 2017

View reviewed changes

Take review into account

9750280

mbugeia approved these changes Jan 5, 2017

View reviewed changes

mbugeia merged commit 7ae3c03 into YunoHost-Apps:master Jan 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download sources during installation and upgrade to 1.0.36 #38

Download sources during installation and upgrade to 1.0.36 #38

JimboJoe commented Dec 27, 2016

JimboJoe commented Jan 2, 2017

mbugeia left a comment

mbugeia Jan 2, 2017

mbugeia Jan 2, 2017

mbugeia Jan 2, 2017

mbugeia Jan 2, 2017

mbugeia Jan 2, 2017

JimboJoe commented Jan 3, 2017 •

edited

mbugeia commented Jan 4, 2017

JimboJoe commented Jan 4, 2017

mbugeia left a comment

mbugeia Jan 4, 2017

JimboJoe Jan 4, 2017

mbugeia Jan 4, 2017

mbugeia Jan 4, 2017

JimboJoe Jan 4, 2017

mbugeia commented Jan 5, 2017

Download sources during installation and upgrade to 1.0.36 #38

Download sources during installation and upgrade to 1.0.36 #38

Conversation

JimboJoe commented Dec 27, 2016

JimboJoe commented Jan 2, 2017

mbugeia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimboJoe commented Jan 3, 2017 • edited

mbugeia commented Jan 4, 2017

JimboJoe commented Jan 4, 2017

mbugeia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbugeia commented Jan 5, 2017

JimboJoe commented Jan 3, 2017 •

edited