Add support for SLE_PRODUCT specific regcodes #4088
Conversation
lib/registration.pm
Outdated
| @@ -459,6 +459,11 @@ sub get_addon_fullname { | |||
|
|
|||
|
|
|||
| sub fill_in_reg_server { | |||
| # Set product specific SCC_REGCODE if it was provided. Defaults to SCC_REGCODE if set | |||
| my $scc_code = get_required_var('SCC_REGCODE') if get_var('SCC_REGCODE'); | |||
There was a problem hiding this comment.
No point to call get_required_var just after you check it's existance, you can simplify by calling just get_var instead.
There was a problem hiding this comment.
If I know perl correctly then it will only even define the variable if it is set, i.e. cause an ugly Perl warning otherwise.
I recommend to use just the second argument of get_var in both lines about the regcode setting, e.g.
my $regcode = get_var('SCC_REGCODE_' . uc(get_var('SLE_PRODUCT', '')), get_var ('SCC_REGCODE', ''));
...
type_string $regcode if $regcode;
There was a problem hiding this comment.
Should I also change this as well?
https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/registration.pm#L471
get_required_var() if get_var() was already in the code, so I left it that way even though it seemed redundant to me.
There was a problem hiding this comment.
sure, it's not about that you did something wrong . You can just improve code while modifying it
|
You added a nice description in the PR which would also be very helpful in the commit message itself. Could you also write it threw |
lib/registration.pm
Outdated
| @@ -459,6 +459,11 @@ sub get_addon_fullname { | |||
|
|
|||
|
|
|||
| sub fill_in_reg_server { | |||
| # Set product specific SCC_REGCODE if it was provided. Defaults to SCC_REGCODE if set | |||
| my $scc_code = get_required_var('SCC_REGCODE') if get_var('SCC_REGCODE'); | |||
There was a problem hiding this comment.
using get_required_var if you do not require the variable is strange. I think get_var('SCC_REGCODE', '') is more in line
lib/registration.pm
Outdated
| @@ -459,6 +459,11 @@ sub get_addon_fullname { | |||
|
|
|||
|
|
|||
| sub fill_in_reg_server { | |||
| # Set product specific SCC_REGCODE if it was provided. Defaults to SCC_REGCODE if set | |||
| my $scc_code = get_var('SCC_REGCODE', '') if get_var('SCC_REGCODE'); | |||
There was a problem hiding this comment.
this assignment needs to be unconditionally
lib/registration.pm
Outdated
| @@ -459,6 +459,11 @@ sub get_addon_fullname { | |||
|
|
|||
|
|
|||
| sub fill_in_reg_server { | |||
| # Set product specific SCC_REGCODE if it was provided. Defaults to SCC_REGCODE if set | |||
| my $scc_code = get_var('SCC_REGCODE', '') if get_var('SCC_REGCODE'); | |||
| $scc_code = get_var('SCC_REGCODE_' . uc(get_var('SLE_PRODUCT')), '') | |||
There was a problem hiding this comment.
please assign get_var('SCC_REGCODE_' . uc(get_var('SLE_PRODUCT')), '') to another variable and do not duplicate the construct.
Adds support for product specific registration codes using the
SLE_PRODUCT variable. The content of SCC_REGCODE will be treated as the
default registration code, which in turn can be overwritten by the
variable SCC_REGCODE_ . getvar('SLE_PRODUCT').
This helps to streamline test suites, helping to avoid the need to
create product/architecture specific tests.
In fill_in_reg_server(), get_required_var() was being used after checking the existence of the variable, so this streamlines the code by changing those calls to get_var() and also adding default empty strings to the get_var() calls
Remove conditional assignments in fill_in_reg_server()
Adds support for product specific registration codes using the SLE_PRODUCT variable.
The content of SCC_REGCODE will be treated as the default registration code, which in turn can be overwritten by the variable SCC_REGCODE_ . getvar('SLE_PRODUCT').
This helps to streamline test suites, helping to avoid the need to create product/architecture specific tests.