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

Add bios boot partition for storage-ng for RAID #4009

Merged
merged 3 commits into from
Dec 11, 2017

Conversation

rwx788
Copy link
Member

@rwx788 rwx788 commented Dec 5, 2017

As per bsc#1063957 there is a change in behavior and now bios boot
partition is required for storage-ng.

@@ -74,6 +74,7 @@ sub init_cmd {
entiredisk alt-e
guidedsetup alt-g
rescandevices alt-e
exp_part_finish alt-f
Copy link
Member

Choose a reason for hiding this comment

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

sorry I am totally out of concept behind this $cmd but what the point to define something in one place to redifine it in another ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here we define default key and for storage-ng these are different, so we redefine it, but in the code we can use same hash key without conditional switches.

Copy link
Member

Choose a reason for hiding this comment

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

and this is the way to go for testing localization eventually :)

Copy link
Member

Choose a reason for hiding this comment

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

ok

@@ -60,9 +61,14 @@ sub addpart {
send_key $cmd{donotformat};
send_key "tab";
send_key 'alt-i' if is_storage_ng; # Select file system
my $partition_type;
Copy link
Member

Choose a reason for hiding this comment

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

variable never used

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, that's remains of on if the approaches, thanks!

assert_screen 'partitioning_raid-disk_vdd_with_partitions-selected';
# fold the drive tree
send_key 'left';
assert_screen 'partitioning_raid-hard_disks-unfolded';
Copy link
Member

Choose a reason for hiding this comment

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

not saying it really should be like this but just thinking loud - can we replace all this send_key + assert_screen with send_key_until_needlematch ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do that, but in some places it's about single key press, so I don't see much value in it, except more stability, which is not an issue currently

Copy link
Member

Choose a reason for hiding this comment

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

@asmorodskyi I thought about the same. We can make it more explicit by adding a new function send_key_and_assert_screen left => 'partitioning_raid-hard_disks-unfolded' but I don't think it will help much. OTOH if by doing this we can encourage more users to do this by default and not try to call multiple send_key in a row without checking results in between then maybe it would help

Copy link
Member

Choose a reason for hiding this comment

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

ok

}
if (!get_var('UEFI')) { # partitioning type does not appear when GPT disk used, GPT is default for UEFI
if (is_storage_ng) {
record_soft_failure 'bsc#1055743'; # No partitioning type page ATM
Copy link
Member

Choose a reason for hiding this comment

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

check the bug - currently it has state "VERIFIED INVALID"

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, we can fix that here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have changed that in 2 places

As per bsc#1063957 there is a change in behavior and now bios boot
partition is required for storage-ng.

See [poo#28573](https://progress.opensuse.org/issues/28573).
@asmorodskyi
Copy link
Member

Needles PR still have WIP so can't merge

@rwx788
Copy link
Member Author

rwx788 commented Dec 6, 2017

@asmorodskyi This PR is also WIP, we are waiting for the bug fix first, which should happen in staging in TW and after that we can apply it to the production.

@okurz
Copy link
Member

okurz commented Dec 6, 2017

except for WIP LGTM

@rwx788 rwx788 removed the WIP Work in progress label Dec 7, 2017
@DimStar77
Copy link
Contributor

WIP label was removed 4 days ago - does this mean it's good to go? (and fixes by Staging:E ?)

@asmorodskyi asmorodskyi merged commit 882930a into os-autoinst:master Dec 11, 2017
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