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 PR#7951 #8020

Merged
merged 1 commit into from Jul 25, 2019
Merged

Fix PR#7951 #8020

merged 1 commit into from Jul 25, 2019

Conversation

DrMullings
Copy link
Contributor

Now only using root-ssh where it should exist

@DrMullings
Copy link
Contributor Author

DrMullings commented Jul 25, 2019

Verification: https://openqa.suse.de/tests//3131588

Now only using root-ssh where it should exist
@SergioAtSUSE
Copy link
Member

Waiting for travis

@SergioAtSUSE SergioAtSUSE merged commit 500bdc5 into os-autoinst:master Jul 25, 2019
@@ -682,7 +682,7 @@ sub activate_console {
assert_screen "inst-console";
}
}
elsif ($console =~ m/root-console$/ && is_remote_backend) {
elsif ($console =~ m/root-console$/ && get_var('BACKEND' =~ /ikvm|ipmi|spvm/)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This introduced a regression (found by @czerw, he's creating PR which fixes it)

@okurz
Copy link
Member

okurz commented Aug 9, 2019

@DrMullings we tried to replace the explicit checks for the BACKEND variable with methods with an easy to understand name. When is_remote_backend was not the right one, what would describe the correct requirement without mentioning the specific backends?

@pevik
Copy link
Contributor

pevik commented Aug 9, 2019

@DrMullings we tried to replace the explicit checks for the BACKEND variable with methods with an easy to understand name.

Good :). Yesterday, I was thinking to introduce something like this as well.

@SergioAtSUSE
Copy link
Member

@okurz, we also didn't came up with a good idea for the name, so we left the backends.

What do you think about

root_console_is_ssh()

@okurz
Copy link
Member

okurz commented Aug 14, 2019

"root_console_is_ssh()" sounds like an arbitrary choice in the test code itself, not describing the infrastructure. So to me it seems we are going back and forth with changes because some people still don't understand what's going on. The backends and especially their handling differ in test code but all of this can be changed where it makes sense. If a console behaves different on a certain backend but it shouldn't, change it! :)

@SergioAtSUSE
Copy link
Member

@okurz, the use of SSH for those specific backends is an arbitrary choice, that's the difficult part of chosing a name. So, we left the backend then.

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