New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global upgrade #68

Merged
merged 11 commits into from Feb 17, 2019

Conversation

Projects
None yet
3 participants
@maniackcrudelis
Copy link
Contributor

maniackcrudelis commented Feb 10, 2019

Not that much changes, comparing to what I was expecting...
The next stable release will be more interesting, with many new helpers.

Feel free to modify any text in ynh_print_info

@maniackcrudelis maniackcrudelis requested a review from YunoHost/apps Feb 10, 2019

@JimboJoe
Copy link
Member

JimboJoe left a comment

Thanks for that, packages will be awesome! ❤️
I've made a couple of cosmetic comments/proposals.

Show resolved Hide resolved scripts/_common.sh Outdated
Show resolved Hide resolved scripts/_common.sh Outdated
Show resolved Hide resolved scripts/change_url Outdated
Show resolved Hide resolved scripts/change_url Outdated
Show resolved Hide resolved scripts/install Outdated
# Find a free port
port=$(ynh_find_port 8095)
# Open this port
yunohost firewall allow --no-upnp TCP $port 2>&1
ynh_exec_warn_less yunohost firewall allow --no-upnp TCP $port

This comment has been minimized.

@JimboJoe

JimboJoe Feb 10, 2019

Member

Maybe comment on why we're using the ynh_exec_warn_less helper here...?

This comment has been minimized.

@maniackcrudelis

maniackcrudelis Feb 10, 2019

Author Contributor

You mean why we don't want to have stdout ? Or why we were using 2>&1 ?

Show resolved Hide resolved scripts/upgrade Outdated
Show resolved Hide resolved scripts/upgrade Outdated
Show resolved Hide resolved scripts/backup Outdated
Show resolved Hide resolved scripts/change_url Outdated
Show resolved Hide resolved scripts/install Outdated

JimboJoe and others added some commits Feb 10, 2019

Update scripts/_common.sh
Co-Authored-By: maniackcrudelis <maniackcrudelis@users.noreply.github.com>
Update scripts/_common.sh
Co-Authored-By: maniackcrudelis <maniackcrudelis@users.noreply.github.com>
Update scripts/upgrade
Co-Authored-By: maniackcrudelis <maniackcrudelis@users.noreply.github.com>

@maniackcrudelis maniackcrudelis referenced this pull request Feb 11, 2019

Open

Package upgrade #44

8 of 8 tasks complete
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 13, 2019

Bump

If you want to modify some texts, please just do it.
I need this PR to be merged in order to update all Official apps.

Show resolved Hide resolved scripts/backup Outdated

ynh_backup "/etc/nginx/conf.d/$domain.d/$app.conf"

#=================================================
# BACKUP THE PHP-FPM CONFIGURATION
#=================================================
ynh_print_info "Backing up php-fpm configuration..."

ynh_backup "/etc/php/7.0/fpm/pool.d/$app.conf"

This comment has been minimized.

@alexAubin

alexAubin Feb 13, 2019

Member

So, uh, can't we just have block like

#=================================================
# BACKUP CONFIGURATION FILES
#=================================================
ynh_print_info "Backing configuration files..."

# Nginx
ynh_backup "/etc/nginx/conf.d/$domain.d/$app.conf"

# php-fpm
ynh_backup "/etc/php/7.0/fpm/pool.d/$app.conf"

# logrotate
ynh_backup "/etc/logrotate.d/$app"

# systemd
ynh_backup "/etc/systemd/system/$app.service"

# cron job
ynh_backup "/etc/cron.d/$app"

It looks a bit weird to me to have 5 huge blocks just to have so many similar "small" instructions (those literally just backup one file each time) with the mysql backup (which actually does takes some time) in the middle ...

This comment has been minimized.

@maniackcrudelis

maniackcrudelis Feb 13, 2019

Author Contributor

Hum, I actually prefer the current way because it clearer when you're looking for a part of the backup in the script.
But that's not really important.
Also, I thought, when writing those lines, that it could be interesting for the final user to know what exactly will be backup.

This comment has been minimized.

@alexAubin

alexAubin Feb 13, 2019

Member

Meh but that's just way too low level ... I mean, in the spirit of YunoHost, maybe a tech-savvy user will know what nginx and php-fpm will refer to, but a regular user shouldn't need to know what those are ...

Imho the purpose of those messages are essentially to let the user know "what the machine is doing right now that is taking so much time". But displaying "5 different messages with technical terms for stuff that lasted basically 0 seconds" looks more like debug than info ... sure it might help a technical user in knowing what's happening under the hood, but it tends to frustrate non-technical user and (from my experience at least) when seeing this they will say "this is so complicated, I have no idea what those mean".

This comment has been minimized.

@maniackcrudelis

maniackcrudelis Feb 13, 2019

Author Contributor

As usual, I'm more in favor of using real name for things than to hide what's really going on.
So I'd rather prefer to add those message to explain what's going to be backup instead of thinking that it's too complicated, so we should hide that from our users.
If an user doesn't understand what is it, he will simply ignore it. For other, It's really useful to know what's going on.

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Feb 13, 2019

Applied a few tweaks (I removed a few ynh_print_info which, like JimboJoe, I think are really too technical / not a "meaningful step"...)

Feel free to discuss ¯\_(ツ)_/¯

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 13, 2019

Ok for me.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

maniackcrudelis commented Feb 15, 2019

Do we agree on the last changes about ynh_print_info ?

@maniackcrudelis maniackcrudelis referenced this pull request Feb 15, 2019

Open

Package upgrade #34

5 of 8 tasks complete
@alexAubin
Copy link
Member

alexAubin left a comment

(for me that's okay, but I'm not really in the app team so 😉 )

@maniackcrudelis maniackcrudelis merged commit 4574548 into master Feb 17, 2019

@maniackcrudelis maniackcrudelis deleted the global_upgrade branch Feb 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment