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

Fix poo#15566 #2992

Merged
merged 1 commit into from Jun 2, 2017
Merged

Fix poo#15566 #2992

merged 1 commit into from Jun 2, 2017

Conversation

fschilling
Copy link
Contributor

action #15566: qam-kgraft: testers need to know whether zypper is actually
doing something (https://progress.opensuse.org/issues/15566)

Stop testing immediately if kgraft-patch is already installed on the SUT.

@fschilling fschilling force-pushed the poo15566 branch 3 times, most recently from 3df0a80 to a8df3fe Compare June 1, 2017 11:20
mimi1vx
mimi1vx requested changes Jun 1, 2017
View reviewed changes
tests/qam-kgraft/update_kgraft.pm Outdated
# warm up system
sleep 15;

die "Patch isnt valid for this system" if is_patch_needed($patch);

#die if patch is already installed on the system
Copy link
Member

Choose a reason for hiding this comment

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

not needed , it duplicates line 123 and comment says 100 and 101 returncode but check is for 101/102

@mimi1vx
Copy link
Member

mimi1vx commented Jun 1, 2017

btw tools/tidy from os-autoinst is your friend

mimi1vx
mimi1vx requested changes Jun 1, 2017
View reviewed changes
tests/qam-kgraft/update_kgraft.pm Outdated
@@ -19,21 +19,27 @@ use strict;
use warnings;

sub timemark {
my ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $isdst) = localtime(time);
return sprintf("%02d%02d%02d.%02d%02d.%02d", $year % 100, $mon + 1, $mday, $hour, $min, $sec);
my ( $sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $isdst ) =
Copy link
Member

Choose a reason for hiding this comment

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

why ? it brokes tidy check

tests/qam-kgraft/update_kgraft.pm Outdated
script_run("kgr status | tee /dev/$serialdev", 0);
my $out = wait_serial([$kgr_ready, $kgr_in_progress], 10);
if ($out =~ $kgr_ready) {
script_run( "kgr status | tee /dev/$serialdev", 0 );
Copy link
Member

Choose a reason for hiding this comment

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

same as above

tests/qam-kgraft/update_kgraft.pm Outdated
return 1;
}
elsif ($out =~ $kgr_in_progress) {
elsif ( $out =~ $kgr_in_progress ) {
Copy link
Member

Choose a reason for hiding this comment

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

dtto

tests/qam-kgraft/update_kgraft.pm Outdated
@@ -46,15 +52,18 @@ sub kgr_block {

while (1) {
script_run("kgr poke");
script_run(qq{BLOCK="kgr-\$(kgr blocking)"; echo \$BLOCK > /dev/$serialdev}, 0);
$out = wait_serial([$kgr_block_hwrandom, $kgr_block_other, $kgr_block_free], 30);
script_run(
Copy link
Member

Choose a reason for hiding this comment

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

dtto

tests/qam-kgraft/update_kgraft.pm Outdated
select_console('sut');
select_console("root-console");

capture_state("before");
script_run(qq{if \$(zypper lr | grep test-kgraft -q);then zypper rr test-kgraft ; fi });
script_run(
Copy link
Member

Choose a reason for hiding this comment

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

dtto

tests/qam-kgraft/update_kgraft.pm Outdated
#patch system
zypper_call(
"in -t patch $patch",
exitcode => [0, 102, 103],
exitcode => [ 0, 102, 103 ],
Copy link
Member

Choose a reason for hiding this comment

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

dtto , and all changes to the end

@fschilling
Copy link
Contributor Author

I used perltidy provided by leap. Let me clean up and check-in again.

@okurz
Copy link
Member

okurz commented Jun 1, 2017

@fschilling I think your problem is that you are missing a local .perltidyrc from the os-autoinst project. Take a look into the Makefile for os-autoinst-distri-opensuse. There is a make prepare target

@fschilling fschilling force-pushed the poo15566 branch 9 times, most recently from 0947385 to e234f9d Compare June 1, 2017 15:13
@fschilling
Copy link
Contributor Author

"zypper -n info -t patch $patch" in "sub is_patch_needed" in lib/qam.pm will have
Status: Installed
as part of the output. And not "Not needed". Therefore a second check is needed.

asmorodskyi
asmorodskyi reviewed Jun 2, 2017
View reviewed changes
tests/qam-kgraft/update_kgraft.pm Outdated
# are acceptible for patch-checking the repository containing the
# kgraft-patch to test.
# Stop testing immediately if it is anything else.
if (!zypper_call("patch-check -r test-kgraft", exitcode => [101, 102])) {
Copy link
Member

Choose a reason for hiding this comment

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

I am confused - your comment describing 100 and 101 , but in zypper_call you enum 101 and 102 . Typo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was a typo. Now corrected.

@mimi1vx
Copy link
Member

mimi1vx commented Jun 2, 2017

@fschilling then will better fix this function to cover this situation than extending test itself ..

@okurz
Copy link
Member

okurz commented Jun 2, 2017

Please be a bit more specific in the git commit subject line

Fix poo#15566 (in qam-kgraft: let test die if patch is already instal…
ef9fdda 
…led)

action os-autoinst#15566: qam-kgraft: testers need to know whether zypper is actually
doing something (https://progress.opensuse.org/issues/15566)

Stop testing immediately if kgraft-patch is already installed on the SUT.
@fschilling
Copy link
Contributor Author

@okurz changed git commit subject line

@okurz
Copy link
Member

okurz commented Jun 2, 2017

LGTM

@fschilling
Copy link
Contributor Author

@mimi1vx
The function in question is called "is_patch_needed". If a patch is already installed it is NOT "Not needed" and also NOT "needed" but installed. Therefore I would advise not to extend the function without also changing its name.
Also I'm only aware of update_kgraft.pm where we really want to know whether zypper would exit with 0 and the message "Nothing to do."

@asmorodskyi asmorodskyi merged commit 8fd0f80 into os-autoinst:master Jun 2, 2017
@fschilling fschilling deleted the poo15566 branch June 2, 2017 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmorodskyi asmorodskyi asmorodskyi left review comments

@mimi1vx mimi1vx mimi1vx requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@fschilling @mimi1vx @okurz @asmorodskyi