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.

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

Fix setup of online repositories in Leap 15.1 #7279

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

czerw
Copy link
Contributor

@czerw czerw commented Apr 16, 2019

Fix poo#50309: Leap 15.1 changed behavior in setup of online
repositories. It is triggered earlier in online_repos. Step
installation_mode had to be updated too, to pass screen to next step.
Order of repositories in setup_online_repos was changed.

@czerw czerw requested a review from rwx788 April 16, 2019 10:29
@czerw czerw changed the title [WIP] Fix setup of online repositories in Leap 15.1 Fix setup of online repositories in Leap 15.1 Apr 16, 2019
Fix poo#50309: Leap 15.1 changed behavior in setup of online
repositories. It is triggered earlier in online_repos. Step
installation_mode had to be updated too, to pass screen to next step.
Order of repositories in setup_online_repos was changed.
@@ -642,6 +643,10 @@ sub noupdatestep_is_applicable {
return !get_var("UPGRADE") && !get_var("LIVE_UPGRADE");
}

sub installwithaddonrepos_is_applicable {
Copy link
Member

Choose a reason for hiding this comment

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

utils is overcrowded already . also currently this function called only in single place - onlinerepos . maybe better just move it there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not single place, it is in main_common.pm

if (installwithaddonrepos_is_applicable() && !get_var("LIVECD")) {
. And i moved it from main_common to utils. Or do you think that, there is better place than utils?

Copy link
Contributor

Choose a reason for hiding this comment

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

installwithaddonrepos_is_applicable It looks a bit strange to use underscore only partly, but that's not a function created in this commit (old function from 1bacc65). LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

Not single place, it is in main_common.pm

os-autoinst-distri-opensuse/lib/main_common.pm

Line 537 in f0acb2d
if (installwithaddonrepos_is_applicable() && !get_var("LIVECD")) {
. And i moved it from main_common to utils. Or do you think that, there is better place than utils?

ah miss that , so let's leave it here than

@@ -62,6 +63,9 @@ sub run {
# Test online repos dialog explicitly
if (get_var('DISABLE_ONLINE_REPOS')) {
disable_online_repos_explicitly;
} elsif (installwithaddonrepos_is_applicable() && !get_var("LIVECD")) {
Copy link
Member

Choose a reason for hiding this comment

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

with same reasoning ( function is called only here ) I woud move !get_var('LIVECD') inside a function. in current implementation it is not really clear why !get_var('UPGRADE') is part of function and !get_var('LIVECD') is not

Copy link
Contributor Author

@czerw czerw Apr 17, 2019

Choose a reason for hiding this comment

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

Same reason as above, function is used in main_common.pm.

Copy link
Member

Choose a reason for hiding this comment

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

and in main common you also have exactly same line actually
installwithaddonrepos_is_applicable() && !get_var("LIVECD")
so why !get_var("LIVECD") is outside this function , for me it is still looks like it shoppuld be inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it was already there in such state in main_common and I didn't want to touch what is not necessary.

@@ -642,6 +643,10 @@ sub noupdatestep_is_applicable {
return !get_var("UPGRADE") && !get_var("LIVE_UPGRADE");
}

sub installwithaddonrepos_is_applicable {
Copy link
Contributor

Choose a reason for hiding this comment

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

installwithaddonrepos_is_applicable It looks a bit strange to use underscore only partly, but that's not a function created in this commit (old function from 1bacc65). LGTM.

@asmorodskyi asmorodskyi merged commit 5f6ff0e into os-autoinst:master Apr 17, 2019
@czerw czerw deleted the poo#50309 branch July 24, 2019 12:07
@czerw czerw restored the poo#50309 branch July 24, 2019 12:07
@czerw czerw deleted the poo#50309 branch July 24, 2019 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants