#1473 PR `merged`: Use meaningful variable names for automated UserInput¶

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2017-09-05 14:36:¶

Now each UserInput function call requires
a specified '-I user_input_ID' option and
the autogenerated user_input_ID is gone.

Now the user_input_ID option value can and must be
a valid bash variable name e.g. 'choose_replacement_disk'
so that the user can (if he wants) specify a variable named
UserInput_choose_replacement_disk
(i.e. the user_input_ID option value with prefix 'UserInput_')
where its value is used as automated input.

This way meaningful variable names for automated UserInput
are implemented in a generic way using plain bash syntax
without any restriction how the user needs to specify
his particular automated input settings.

The user can do this statically in his local.conf like
UserInput_choose_replacement_disk="/dev/sda"
or via whatever sophisticated scripting magic as he likes
(e.g. also local.conf is sourced and executed as a script).

jsmeix commented at 2017-09-05 14:44:¶

@gozora
I used in your restore/BORG/default/300_load_archives.sh

UserInput -I BORGBACKUP_archive_to_recover ...

but I think the user input there is run in a loop where the user
can add several BorgBackup archives to restore.
With a fixed UserInput_BORGBACKUP_archive_to_recover
variable value only one automated user input is currently possible
(which is already better than no automated user input before ;-)
but I think you need to enhance that script so that for each
UserInput call a different '-I user_input_ID' option is used
e.g. with a trailing number

UserInput -I BORGBACKUP_archive_to_recover_1 ...
UserInput -I BORGBACKUP_archive_to_recover_2 ...
UserInput -I BORGBACKUP_archive_to_recover_3 ...
...

so that the user can specify as many BorgBackup archives
as he needs e.g. via

UserInput_BORGBACKUP_archive_to_recover_1="archive1"
UserInput_BORGBACKUP_archive_to_recover_1="archive2"
UserInput_BORGBACKUP_archive_to_recover_1="archive3"
...

jsmeix commented at 2017-09-05 14:45:¶

@schabrolles
in your prepare/GNU/Linux/210_load_multipath.sh I used

UserInput -I multipath_failed_to_list_device ...

Please comment if that '-I user_input_ID' option value
is o.k. for you or if you prefer a different one.

jsmeix commented at 2017-09-05 15:02:¶

Intentionally I named the UserInput and my other new functions
for user input and output as the existing functions for user output
were named in ReaR so that all functions for user I/O
are named consistently.
I cannot clean up all the syntactical mess in ReaR.
I like to implement actual functionality but it feels as if
I do most of my time clean up mess that is not from me.
I wished I could clean up e.g. function names via script
but function names like 'Print' make that impossible for me
because I cannot do a semantics testing script because
one cannot simply replace all 'Print' with 'print' - and 'print'
(the lowercase word) is a too simple/generic word so that
it should not be used as a ReaR-specific (function) name.

jsmeix commented at 2017-09-05 15:10:¶

I think it is misleading to lowercase things that are used
like "proper names" for example things like
EFI, USB, SELinux, DRBD, NBKDC.

Perhaps BORGBACKUP should be even correctly
spelled BorgBackup but somewhere we decided
to use all-caps for config variables and that is
wherefrom to took BORGBACKUP.

None of the non-lowercase spellings is my own spelling.
I carefully tried to use "correct" spelling - whatever "correct"
might actually mean in each particular case.

gozora commented at 2017-09-05 15:12:¶

Hello @jsmeix,

but I think the user input there is run in a loop where the user
can add several BorgBackup archives to restore.

Wrong ;-), and thank you for teaching me how comments can be useful ;-)

Reading comment:

# Display BORGBACKUP_ARCHIVE_CACHE file content
# and prompt user for archive to restore.
# Always ask which archive to restore (even if there is only one).
# This gives possibility to abort restore if repository doesn't contain
# desired archive, hence saves some time.

So that loop is there only for user to choose archive to restore or exit if desired archive is missing, no other fancy functionalities.
You certainly can break to loop if BORGBACKUP_archive_to_recover is set and continue with recovery, or if you are busy, I can patch it after your PR is merged ;-).

V.

jsmeix commented at 2017-09-05 15:16:¶

@gozora
now even I see the 'break' statement in the while loop
when one valid BorgBackup archive was specified
so that the current UserInput call is sufficient.

schlomo commented at 2017-09-05 15:18:¶

Sorry @jsmeix I disagree. IMHO mixed spelling is the worst. So within a single name or ID the spelling should be consistent. Either uppercase like in global variables or lowercase as in function names.

Good:

get_size_for_efi_boot_in_mib
SET_SIZE_FOR_EFI_BOOT_IN_MIB

Bad:

Set_size_for_EFI_boot_in_MiB
... other mixtures ...

I am also fine with uppercasing all the user input ID names. This is consistent with our coding style to upper case global variables.

Yes, there is a lot of old mess in ReaR. That is why I believe it to be better to introduce new stuff following our new guidelines instead of matching new stuff to the wrong ways of the past.

Print is indeed a problem, maybe user_print is a good alternative name. Same for Error which might be better named raise_error or abort_with_error or such.

gozora commented at 2017-09-05 15:22:¶

@schlomo just to ease my curiosity (really have no preference in naming and I'm still looking for "perfect" formatting style), why do you think Set_size_for_EFI_boot_in_MiB is bad ?

schlomo commented at 2017-09-05 15:25:¶

@gozora the main reason that is bad is that you have an additional dimension of potential errors. As a user you not only have to get the words right you on top of that also have to get the spelling right. IMHO this is a totally needles source of errors that doesn't help anybody.

If we want to force users to always copy-paste those IDs then we should be using random IDs or UUIDs instead. If we want users to be able to just type them in then we should use a very consistent and as easy as possible way of writing those variables.

schlomo commented at 2017-09-11 14:03:¶

Thanks a lot @jsmeix I know that this is a lot of work.

jsmeix commented at 2017-09-12 10:25:¶

In shameless self-praise mode I am impressed:
That thingy seems to "just work great" - at least for me
(and in any case it is now better than it was ever before ;-)
Here a "rear recover" excerpt with predefined user input:

Welcome to Relax-and-Recover. Run "rear recover" to restore your system !

RESCUE e205:~ # export USER_INPUT_LAYOUT_MIGRATION_REPLACEMENT_DISK="1"

RESCUE e205:~ # export USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS="yEs"

RESCUE e205:~ # rear recover
...
Comparing disks.
Device sda has size 22548578304, 21474836480 expected
Switching to manual disk layout configuration.
Original disk /dev/sda does not exist (with same size) in the target system
Choose an appropriate replacement for /dev/sda
1) /dev/sda
2) /dev/sdb
3) Do not map /dev/sda
4) Use Relax-and-Recover shell and return back to here
(default 1 timeout 300 seconds)
UserInput: Will use predefined input in 'USER_INPUT_LAYOUT_MIGRATION_REPLACEMENT_DISK'='1'
Hit any key to interrupt the automated input (timeout 5 seconds)
Using /dev/sda (chosen by user) for recreating /dev/sda
Current disk mapping table (source -> target):
    /dev/sda /dev/sda
Confirm or edit the disk mapping
1) Confirm disk mapping and continue 'rear recover'
2) Edit disk mapping (/var/lib/rear/layout/disk_mappings)
3) Use Relax-and-Recover shell and return back to here
4) Abort 'rear recover'
(default 1 timeout 300 seconds)
UserInput: Will use predefined input in 'USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS'='1'
Hit any key to interrupt the automated input (timeout 5 seconds)
User confirmed disk mapping
...

schlomo commented at 2017-09-12 11:22:¶

Cool! how come that USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS="yEs" comes out as 1 during the recovery?

jsmeix commented at 2017-09-12 11:29:¶

Because I coded it in layout/prepare/default/300_map_disks.sh

# When USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS has any 'true' value be liberal in what you accept and
# assume choices[0] 'Confirm mapping' was actually meant which is shown with choice number 1 (not 0) and
# a predefined user input must match the choice number that is shown to the user (not the choice index):
is_true "$USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS" && USER_INPUT_LAYOUT_MIGRATION_CONFIRM_MAPPINGS="1"

and similar things in
format/USB/default/200_check_usb_layout.sh
layout/prepare/GNU/Linux/150_include_drbd_code.sh
restore/FDRUPSTREAM/default/270_selinux_considerations.sh
and some kind of opposite logic in
restore/NBKDC/default/400_restore_backup.sh
to avoid that a predefined user input variable could cause harm
because in this case no automated proceed is ever wanted.

jsmeix commented at 2017-09-12 11:39:¶

I made
https://github.com/rear/rear/issues/1484
for the further general cleanup changes.

For now I think this pull request is good enough to be merged.

The main issue was that variables for predefined user input
had not been only with uppercase letters but such variables are
user config variables and we cannot rename user config variables
without causing regressions for our users or implememting
awkward backward-compatibility stuff. Therefore variables
for predefined user input are now only with uppercase letters.

The names of user input/output functions can be changed
via https://github.com/rear/rear/issues/1484 because
function names are implementation details but no
user interface (in contrast to config variables).

I prefer consistent function names
(all user input/output functions in CamelCase)
over strict compliance with our intended coding style
only for the new user input/output function names.
In other words:
I will either change all user input/output function names
to get all in compliance with our intended coding style
or none.

schlomo commented at 2017-09-12 11:45:¶

I see, nice trick. I think that this is a pattern that you might want to add to our coding docs as it is not obvious and it requires the developer to perceive the user input variable as a multi-value variable that can be either true or a number from the list. It also forces the developer to have the first list item as the "true" value.

schlomo commented at 2017-09-12 11:49:¶

I think that you should slowly get to merge this so that we can collect some feedback from users.

About backwards compatible: In general I agree, in particular I like to understand where the backwards compatibility creates a lot of complexity in ReaR. In those cases I would like us to discuss a breaking change, especially if it something where we learned over time that previous assumptions were less than optimum.

jsmeix commented at 2017-09-12 11:53:¶

@schlomo
regarding your
https://github.com/rear/rear/pull/1473#pullrequestreview-60622391

I am strongly :-) against positional parameters/arguments
because I think positional parameters/arguments
are one of the worst things ever.

I strongly ;-) suggest to prefer named parameters/arguments
regardless if one is required or optional - except one single
kind of arguments that are the trailing mass-arguments.
This way also the unnamed arguments have same meaning
because all mass-arguments are of one same kind.

If the ID parameter was a positional parameter I could not
test if a ID parameter was specified at all or if it was valid
because I could not reliably distinguish a right call like

UserInput FOO BAR BAZ

where FOO is meant as ID and BAR BAZ are choices
from a same looking but wrong working call

UserInput FOO BAR BAZ

where no ID is specified because FOO BAR BAZ are
all meant as choices.
In the latter case I would take FOO as ID but because it was
not meant this way that UserInput call would work but it
would behave wrong (hopefully someone detects that)
in contrast to now where I can test the syntax and
immediately BugError out for syntactically wrong calls.

schlomo commented at 2017-09-12 11:55:¶

@jsmeix very good reasoning which convinces me, you probably also would love programming in Python 😄

jsmeix commented at 2017-09-12 12:05:¶

@schabrolles
I simply assume you like in particular how the migration mode
can now be automated at least to some initial extent, cf.
layout/prepare/default/300_map_disks.sh

# TODO: Currently only one single
# USER_INPUT_LAYOUT_MIGRATION_REPLACEMENT_DISK
# can be predefined (which is at least better
# than nothing) but that dialog can appear
# several times for several unmapped
# original 'disk' devices and 'multipath' devices:

jsmeix commented at 2017-09-12 12:08:¶

FYI:
Intentionally the UserInput function variables
are not yet documented in default.conf because
I still like to be able to change things as needed.

jsmeix commented at 2017-09-12 14:31:¶

https://github.com/rear/rear/pull/1486
implements the missing part in
https://github.com/rear/rear/pull/1473#issuecomment-328832250

It was unexpectedly easy :-)

[Export of Github issue for rear/rear.]

#1473 PR merged: Use meaningful variable names for automated UserInput¶