Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Validate ADDONURL test variable #5709

Merged
merged 1 commit into from Sep 11, 2018
Merged

Validate ADDONURL test variable #5709

merged 1 commit into from Sep 11, 2018

Conversation

mloviska
Copy link
Contributor

@mloviska mloviska commented Sep 5, 2018

Validate value of ADDONURL agains WORKAROUND_MODULES test variable to prevent skipping modules which are intended to be installed as addon over FTP in addon_product_sle.

rwx788
rwx788 reviewed Sep 10, 2018
View reviewed changes
tests/installation/addon_products_sle.pm
sub test_addonurl {
my $testvalue = get_var('ADDONURL');
my @missing_modules;
my @test_modules = split(/,/, get_var('WORKAROUND_MODULES'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused here, we use WORKAROUND_MODULES to set ADDONURL variables, so I would say we need to justify final result and show missing modules for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups, I have forgotten to replace this one with the list of modules. I was just lazy to look after all of the possible modules, while I was writing the code. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I recall why I have chosen this solution. https://openqa.suse.de/tests/1824434/file/vars.json WORKAROUND_MODULES is basically hardcoded in openQA test definition, therefore always available and we are selecting only a portion of all possible modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I guess I was confused by the message we write in case of failure. So what if we write something like:
"Missing modules in ADDONURL which are set in WORKAROUND_MODULES: ..."
I guess we also could validate if we can reach url here instead of trying to add some modules and fail after, so not to waste time trying to do something when we know that we will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to access it on L#193

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I saw that, but as I've said we could do quick check here as we validate settings before trying to do anything about it. So in case of unavailable module we will fail earlier and hence save some computing time.
Later on we can also wait when YaST will fail, as it won't be able to access the link and will give transparent error message. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense, so in case we have the variable set properly, test will die immediately if the repo is not accessible.

tests/installation/addon_products_sle.pm
@@ -126,6 +127,21 @@ sub handle_addon {
}
}

sub test_addonurl {
my $testvalue = get_var('ADDONURL');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's worth to checking if ADDONURL is set when we expect it to be set. If now we will fail to set it, we won't even call this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are setting ADDONURL only when the test variables WORKAROUND_MODULES & ALL_MODULES are set. So the subroutine should be called only under these conditions as long as I have not missed anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, in a normal case, but when we would call die https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/products/sle/main.pm#L299 previously, now we will ignore such scenario, even though that's most likely an error that ADDONURL was not set in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And as we are currently discussing this topic, I have not seen any usage of ALL_MODULES variable among openQA test suites definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In such a case, it should die on L#181

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but it won't get there...right.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can easily be the case, as this approach was used to start testing of SLE 15 which didn't work with modules yet, so we had to add all modules as addons, so I guess we can remove this part. But WORKAROUND_MODULES are still used in staging. So we set ADDONURL when:

sle_version_at_least('15') && !check_var('SCC_REGISTER', 'installation') && (get_var('ALL_MODULES') || get_var('WORKAROUND_MODULES'))

rwx788
rwx788 reviewed Sep 10, 2018
View reviewed changes
tests/installation/addon_products_sle.pm
@@ -126,6 +127,21 @@ sub handle_addon {
}
}

sub test_addonurl {
my $testvalue = get_var('ADDONURL');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, in a normal case, but when we would call die https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/products/sle/main.pm#L299 previously, now we will ignore such scenario, even though that's most likely an error that ADDONURL was not set in the end.

tests/installation/addon_products_sle.pm Outdated
if (match_has_tag('inst-addon')) {
send_key 'alt-k'; # install with addons
}
else {
send_key 'alt-a';
}
for my $addon (split(/,/, get_var('ADDONURL'))) {
for my $addon (split(/,/, get_required_var('ADDONURL'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_required_var is not really required here, as on L#174 we already checked that it's set.

@mloviska mloviska force-pushed the addonurl_checker branch 4 times, most recently from 32c77de to 603974e Compare September 10, 2018 14:58
rwx788
rwx788 approved these changes Sep 10, 2018
View reviewed changes
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mloviska
Copy link
Contributor Author

New VRs:

@rwx788 rwx788 merged commit 92819e3 into os-autoinst:master Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwx788 rwx788 rwx788 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mloviska @rwx788