Use getops #561

maniackcrudelis · 2018-10-16T15:20:11Z

The problem

getopts is now available for all our official helpers

Solution

Migrate all our helpers to getopts

PR Status

ynh_use_logrotate: Fully tested (mainly because it was a specific case)
ynh_add_systemd_config: Legacy mode ?? | getopts mode ??
ynh_remove_systemd_config: Legacy mode ?? | getopts mode ??
ynh_backup: Legacy mode OK | getopts mode OK
ynh_restore_file: Legacy mode OK | getopts mode OK
ynh_store_file_checksum: Legacy mode OK | getopts mode OK
ynh_backup_if_checksum_is_different: Legacy mode OK | getopts mode OK
ynh_delete_file_checksum: Already done
ynh_secure_remove: Legacy mode OK | getopts mode OK
ynh_validate_ip: Legacy mode ?? | getopts mode ??
ynh_validate_ip4: Legacy mode ?? | getopts mode ??
ynh_validate_ip6: Legacy mode ?? | getopts mode ??
ynh_mysql_connect_as: Legacy mode OK | getopts mode OK
ynh_mysql_execute_as_root: Legacy mode OK | getopts mode OK
ynh_mysql_execute_file_as_root: Legacy mode ?? | getopts mode ??
ynh_mysql_dump_db: Legacy mode OK | getopts mode OK
ynh_mysql_user_exists: Legacy mode ?? | getopts mode OK
ynh_mysql_setup_db: Legacy mode OK | getopts mode OK
ynh_mysql_remove_db: Legacy mode OK | getopts mode OK
ynh_sanitize_dbid: Legacy mode OK | getopts mode OK
ynh_normalize_url_path: Legacy mode OK | getopts mode OK
ynh_find_port: Legacy mode ?? | getopts mode ??
ynh_webpath_available: Legacy mode OK | getopts mode OK
ynh_webpath_register: Legacy mode OK | getopts mode OK
ynh_install_nodejs: Legacy mode ?? | getopts mode ??
ynh_package_is_installed: Legacy mode ?? | getopts mode ??
ynh_package_version: Legacy mode ?? | getopts mode ??
ynh_die: Legacy mode ?? | getopts mode OK
ynh_print_info: Legacy mode ?? | getopts mode ??
ynh_print_warn: Legacy mode ?? | getopts mode ??
ynh_print_err: Legacy mode ?? | getopts mode ??
ynh_app_setting_get: Legacy mode OK | getopts mode OK
ynh_app_setting_set: Legacy mode OK | getopts mode OK
ynh_app_setting_delete: Legacy mode OK | getopts mode OK
ynh_string_random: Legacy mode ?? | getopts mode OK
ynh_replace_string: Legacy mode OK | getopts mode OK
ynh_replace_special_string: Legacy mode ?? | getopts mode ??
ynh_user_exists: Legacy mode ?? | getopts mode ??
ynh_user_get_info: Legacy mode ?? | getopts mode ??
ynh_system_user_exists: Legacy mode ?? | getopts mode OK
ynh_system_user_create: Legacy mode OK | getopts mode OK
ynh_system_user_delete: Legacy mode OK | getopts mode OK
ynh_setup_source: Legacy mode OK | getopts mode OK

How to test

Although it's not finished, I'm not going to go back unless there's an error to fix.
So don't hesitate to help by testing the helpers.

To test, replace the official helpers by this version. (/usr/share/yunohost/helpers.d/)

Validation

Principle agreement 0/2 :
Quick review 0/1 :
Simple test 0/1 :
Deep review 0/1 :

maniackcrudelis · 2018-12-31T12:47:33Z

I tried to test with a full replacement of the helpers.
Tried with wordpress directly, which isn't using getopts args.
So, getopts is used in Legacy mode , whith many weird errors...

Here my errors so far, for those who want to try as well.

First, this echo is injected in the middle of the $arguments array.
~~Don't even know it can be possible...~~
Found, https://github.com/YunoHost-Apps/wordpress_ynh/blob/master/scripts/install#L40 is getting stdout from the helper, so all echo with it...
$key, even if local in this helper prevent any use of key as a variable in a helper. Maybe because the variable will be define here, https://github.com/YunoHost/yunohost/blob/use_getops/data/helpers.d/getopts#L185 So we can't define twice the same variable.
So that I replaced $key by $option_flag in the helper in my code.
Looks like later, there's another error with mysql, where db_name isn't correctly set.

maniackcrudelis · 2019-01-04T15:34:12Z

Finally found the error with mysql, and that's bigger than I thought it would be...
An associative array isn't sorted like a indexed one is. Which means that we can't know in which order the array will be stored.

In ynh_mysql_setup_db, the array is set that way:

declare -Ar args_array=( [u]=db_user= [n]=db_name= [p]=db_password= )

Most of the time, for other helpers, the array will be set as:

[ u ] - [db_user=]
[ n ] - [db_name=]
[ p ] - [db_password=]

But, during my tests, the array is set that way

[ n ] - [db_name=]
[ p ] - [db_password=]
[ u ] - [db_user=]

That's not a problem when using getopts parameters, because we're using the key, n, p or u to access the array. But, when using the legacy mode, there's no way to know which argument is supposed to match which option, unless by using arguments in the same order than the array.
For this example, we assume that $1 is db_user, $2 db_name and $3 db_password. But when the array isn't sorted the same way than the line that create it. How can we know which option was the first one, and so on.

This issue will need to rethink a part of the legacy mode, and also to add a variable in the helper that call ynh_handle_getopts_args to fix that order in a strict way.

There's no reason in that loop where we're looking for values of an option to have a blank cell in the array of arguments. Unless for an option with a missing value. In that case, it's better to ignore this condition and store a blank value in the variable. Otherwise, in case of missing value for an option, with this condition, we enter in an infinite loop, because the shift will stay at 0.

maniackcrudelis · 2019-01-04T18:30:36Z

I guess now the code is stable enough to really starting to test the helpers...

Many core backup script source /usr/share/yunohost/helpers.d/filesystem only. Not sure it's a wonderful idea...

maniackcrudelis · 2019-01-05T14:16:06Z

I've tested most of the helpers with an app and package check, without any errors.
Some aren't tested yet, feel free to do it if you want. I do not expect any errors either.
For me this PR is finally ready to be reviewed.

maniackcrudelis · 2019-01-27T22:07:11Z

#455 has been merged in unstable a few hours ago.
This implementation of getopts needs the fix of ynh_handle_getopts_args, currently in this PR.

So, please this PR needs reviews.

alexAubin · 2019-01-27T22:20:32Z

Uh I didn't realize that ... Maybe we can revert and reopen #455 then ? Or merge only the new getopts file from this PR ... I'm a bit unconfortable with merging all this PR considering the number of changes and that the goal is to go for a stable on ~Tuesday

maniackcrudelis · 2019-01-27T22:22:47Z

I'll make another PR to fix ynh_handle_getopts_args then.
After the stable release though, I would be interesting to merge this one. So we would have some test with it.

alexAubin · 2019-01-27T22:25:12Z

Yup, same 👍

Fix ynh_handle_getopts_args from #561 - 2094557#diff-a1f538c2eae234f6ba78e0bed0ae54db - 976f160#diff-a1f538c2eae234f6ba78e0bed0ae54db - 50f3291#diff-a1f538c2eae234f6ba78e0bed0ae54db - 540291a#diff-a1f538c2eae234f6ba78e0bed0ae54db

maniackcrudelis · 2019-01-28T15:44:05Z

@YunoHost/apps

alexAubin · 2019-02-05T17:07:35Z

So do we agree to merge this as is to run tests via the C.I ? 😉

JimboJoe · 2019-02-05T20:07:48Z

This is a great piece of work, and while I haven't tested it myself, it's totally LGTM! 👍

JimboJoe

LGTM 👍

maniackcrudelis · 2019-02-06T10:51:53Z

So do we agree to merge this as is to run tests via the C.I ?

Yep, would be great !
At least for unstable test to begin.

maniackcrudelis added 2 commits October 10, 2018 21:15

Use getopts for ynh_use_logrotate

16e00fb

Use getopts for ynh_add_systemd_config and ynh_remove_systemd_config

f354c4b

maniackcrudelis requested review from Josue-T and JimboJoe October 16, 2018 15:20

maniackcrudelis added the work needed label Oct 16, 2018

maniackcrudelis added 5 commits October 16, 2018 23:41

Use getopts for helpers in filesystem

d59401f

Use getopts for ip's helpers

4c82dad

Use getopts for sql's helpers

35b483a

Fix bad copy paste

743bc0d

Fix bad copy paste

2840531

alexAubin added this to the 3.x milestone Oct 24, 2018

maniackcrudelis added 19 commits December 21, 2018 20:40

Use getopts for network's helpers

a701b42

Use getopts for nodejs helper

0aed8a6

Use getopts for package's helpers

0654c68

Use getopts for print's helpers

fe6a414

Use getopts for setting's helpers

f24b7a6

Use getopts for string's helpers

c057d38

Use getopts for user's helpers

6425117

Use getopts for ynh_setup_source helper

bc08c5c

Use getopts helpers in string

d3a501a

Use getopts helpers in mysql

8c600e1

Use getopts helpers in network

43e4a34

Use getopts helpers in package

68d6adf

Use getopts helpers in user

0aa8a2e

Use getopts helpers in getopts itself

2094557

Use getopts helpers in nodejs

2c6b76e

Use getopts helpers in utils

80c0b7b

Use getopts helpers in backend

705fe43

Use getopts helpers in filesystem

e3bebf0

Merge branch 'stretch-unstable' into use_getops

4bac17a

alexAubin added this to the 3.5.x milestone Dec 29, 2018

maniackcrudelis added 6 commits January 4, 2019 17:11

Fix getopts and its legacy mode

976f160

Use getopts helpers in mysql, again

7de184a

Keep db_pwd instead of db_password

1a4e661

Ignore useless parameters in legacy mode

540291a

db_pwd shouldn't be local

bf443f2

source getopts in filesystem

0a3e5c6

Many core backup script source /usr/share/yunohost/helpers.d/filesystem only. Not sure it's a wonderful idea...

maniackcrudelis added app helpers and removed tests needed labels Jan 6, 2019

Merge branch 'stretch-unstable' into use_getops

e14d897

maniackcrudelis mentioned this pull request Jan 27, 2019

[fix] Update ynh_handle_getopts_args #628

Merged

4 tasks

maniackcrudelis and others added 2 commits January 31, 2019 13:55

Merge branch 'stretch-unstable' into use_getops

5f0fc73

Merge branch 'stretch-unstable' into use_getops

8ba8b24

JimboJoe approved these changes Feb 5, 2019

View reviewed changes

alexAubin merged commit 69a896e into stretch-unstable Feb 6, 2019

alexAubin deleted the use_getops branch February 6, 2019 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use getops #561

Use getops #561

maniackcrudelis commented Oct 16, 2018 •

edited

maniackcrudelis commented Dec 31, 2018 •

edited

maniackcrudelis commented Jan 4, 2019

maniackcrudelis commented Jan 4, 2019

maniackcrudelis commented Jan 5, 2019

maniackcrudelis commented Jan 27, 2019

alexAubin commented Jan 27, 2019

maniackcrudelis commented Jan 27, 2019

alexAubin commented Jan 27, 2019

maniackcrudelis commented Jan 28, 2019

alexAubin commented Feb 5, 2019

JimboJoe commented Feb 5, 2019

JimboJoe left a comment

maniackcrudelis commented Feb 6, 2019

Use getops #561

Use getops #561

Conversation

maniackcrudelis commented Oct 16, 2018 • edited

The problem

Solution

PR Status

How to test

Validation

maniackcrudelis commented Dec 31, 2018 • edited

maniackcrudelis commented Jan 4, 2019

maniackcrudelis commented Jan 4, 2019

maniackcrudelis commented Jan 5, 2019

maniackcrudelis commented Jan 27, 2019

alexAubin commented Jan 27, 2019

maniackcrudelis commented Jan 27, 2019

alexAubin commented Jan 27, 2019

maniackcrudelis commented Jan 28, 2019

alexAubin commented Feb 5, 2019

JimboJoe commented Feb 5, 2019

JimboJoe left a comment

Choose a reason for hiding this comment

maniackcrudelis commented Feb 6, 2019

maniackcrudelis commented Oct 16, 2018 •

edited

maniackcrudelis commented Dec 31, 2018 •

edited