Refactor config functions code

[schaefi]

Reorganize the code into more readable areas like methods
present as helpers, methods for customers, methods which are
distribution specific and also methods that are deprecated
and give a good reason why they are deprecated when they
get called. This is related to Issue #1828

[schaefi]

@dcermak I know this is hard to review because I moved code blocks into more readable areas of the file. Most interesting is the data in the section about deprecated methods. Please see the reasons I gave with any deprecation and if you think I did something wrong just veto in :)

Other than that this is only moving existing code and fixing some long line issues

If we are ok with that (and I'm willing to take the blame from customers) I think we should apply your test suite concept on the rest of the code.

[dcermak]

I must confess that I did not check if anything changed in the code when you moved a lot of the functions around, so I trust you here 😉.

I am not entirely sure whether a deprecation warning is really enough though, as I think most of these functions are simply kept to rot in legacy config.sh scripts on OBS & IBS where no one looks at the logs, unless they blow up. So maybe we should add some more drastic measures here like failing the build unless you set a environment variable (or do that in a year or so…).

[schaefi]

I am not entirely sure whether a deprecation warning is really enough though, as I think most of these functions are simply kept to rot in legacy config.sh scripts on OBS & IBS where no one looks at the logs, unless they blow up. So maybe we should add some more drastic measures here like failing the build unless you set a environment variable (or do that in a year or so…).

I agree, we should be more "aggressive" and fail the script in these cases. I'll adapt

[Conan-Kudo]

This won't work for non-x86_64 machines.

[Conan-Kudo]

This should be flipped. Also, os-release can be sourced in a shell script, so you can simplify this.

[dcermak]

Neal Gompa (ニール・ゴンパ) ***@***.***> writes:

> + if [ -f /etc/SuSE-brand ];then + prod=$(head /etc/SuSE-brand -n 1) + elif [ -f /etc/os-release ];then + prod=$(grep "^NAME" /etc/os-release | cut -d '=' -f 2 | cut -d '"' -f 2) This should be flipped. Also, os-release can be sourced in a shell script, so you can simplify this.

Sadly not on some older distros that omit the '"' after the '='.

[Conan-Kudo]

@dcermak when you source them, the quotes are already stripped when you echo the variable

[schaefi]

@dcermak @Conan-Kudo thanks for the review. Please keep in mind I haven't changed the code, just re-organized by moving and dropped obsolete code. I'm sure the code has issues too, but can we first finish the refactor and look at the code in detail afterwards and create followup PRs for each issue we find ?

Thanks

[dcermak]

Neal Gompa (ニール・ゴンパ) ***@***.***> writes:

@dcermak when you source them, the quotes are already stripped when you echo the variable

That's not what I meant, on some older systems I've seen /etc/os-release look like this: ``` NAME=openSUSE Leap ``` and sourcing that doesn't work. It might not be an issue anymore, but the current code handles this (broken) case better.

[schaefi]

I'd like to move forward with this one now. My plan would be

• merge the refactor from here, without substantial code changes from non deprecated functions
• submit to Staging, check integration test results. If deprecated methods are used the build will now fail
• On failed builds check why and update either the description or the methods from here
• Continue improving the code in functions.sh which is left. Taking into account what @Conan-Kudo and @dcermak said here
• Continue adding the changes from the other PRs from @dcermak
• Continue adding the testsuite as suggested by @dcermak

Unless there are objections I'll go ahead now with this approach

[dcermak]

Sounds good to me @schaefi