Module: tools Branch: master Commit: 9925dc0c1389bc3c99430de5a6ad6e381300cfed URL: https://source.winehq.org/git/tools.git/?a=commit;h=9925dc0c1389bc3c99430de5...
Author: Francois Gouget fgouget@codeweavers.com Date: Mon May 30 18:04:07 2022 +0200
testbot: Unify the Job cancel and restart actions.
Most of the code can be shared and this simplifies expanding the set of job control actions.
Signed-off-by: Francois Gouget fgouget@codeweavers.com Signed-off-by: Alexandre Julliard julliard@winehq.org
---
testbot/bin/Engine.pl | 42 ++++-------- testbot/lib/WineTestBot/Engine/Notify.pm | 29 ++------- testbot/web/JobDetails.pl | 107 ++++++++----------------------- 3 files changed, 43 insertions(+), 135 deletions(-)
diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index 7880b8e..da72ffb 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -329,50 +329,31 @@ sub HandleJobStatusChange($$$) return "1OK"; }
-sub HandleJobCancel($) +sub HandleJobControl($) { - my ($JobKey) = @_; + my ($Action, $JobKey) = @_;
- my $Job = CreateJobs()->GetItem($JobKey); - if (! $Job) + if ($Action !~ /^(cancel|restart)$/) { - LogMsg "JobCancel for nonexistent job $JobKey\n"; - return "0Job $JobKey not found"; + LogMsg "Invalid $Action JobControl command\n"; + return "0Invalid $Action JobControl command"; } - # We've already determined that JobKey is valid, untaint it - $JobKey =~ m/^(.*)$/; - $JobKey = $1; - - my $ErrMessage = $Job->Cancel(); - if (defined($ErrMessage)) - { - LogMsg "Cancel problem: $ErrMessage\n"; - return "0$ErrMessage"; - } - - ScheduleJobs(); - - return "1OK"; -} - -sub HandleJobRestart($) -{ - my ($JobKey) = @_; + $Action = $1;
my $Job = CreateJobs()->GetItem($JobKey); - if (! $Job) + if (!$Job) { - LogMsg "JobRestart for nonexistent job $JobKey\n"; + LogMsg "$Action JobControl for nonexistent job $JobKey\n"; return "0Job $JobKey not found"; } # We've already determined that JobKey is valid, untaint it $JobKey =~ m/^(.*)$/; $JobKey = $1;
- my $ErrMessage = $Job->Restart(); + my $ErrMessage = $Action eq "cancel" ? $Job->Cancel() : $Job->Restart(); if (defined($ErrMessage)) { - LogMsg "Restart problem: $ErrMessage\n"; + LogMsg "$Action problem: $ErrMessage\n"; return "0$ErrMessage"; }
@@ -567,8 +548,7 @@ sub HandleGetScreenshot($)
my %Handlers=( "getscreenshot" => &HandleGetScreenshot, - "jobcancel" => &HandleJobCancel, - "jobrestart" => &HandleJobRestart, + "jobcontrol" => &HandleJobControl, "jobstatuschange" => &HandleJobStatusChange, "ping" => &HandlePing, "shutdown" => &HandleShutdown, diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm index de73425..82221c9 100644 --- a/testbot/lib/WineTestBot/Engine/Notify.pm +++ b/testbot/lib/WineTestBot/Engine/Notify.pm @@ -29,8 +29,8 @@ WineTestBot::Engine::Notify - Engine notification
use Exporter 'import'; our $RunningInEngine; -our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobCancel - JobRestart RescheduleJobs VMStatusChange +our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl + RescheduleJobs VMStatusChange WinePatchMLSubmission WinePatchWebSubmission GetScreenshot); our @EXPORT_OK = qw($RunningInEngine);
@@ -115,31 +115,14 @@ sub JobStatusChange($$$) return substr($Reply, 1); }
-sub JobCancel($) +sub JobControl($$) { - my ($JobKey) = @_; + my ($Action, $JobKey) = @_;
- my $Reply = SendCmdReceiveReply("jobcancel $JobKey\n"); + my $Reply = SendCmdReceiveReply("jobcontrol $Action $JobKey\n"); if (length($Reply) < 1) { - return "The Engine did not acknowledge the JobCancel command"; - } - if (substr($Reply, 0, 1) eq "1") - { - return undef; - } - - return substr($Reply, 1); -} - -sub JobRestart($) -{ - my ($JobKey) = @_; - - my $Reply = SendCmdReceiveReply("jobrestart $JobKey\n"); - if (length($Reply) < 1) - { - return "The Engine did not acknowledge the JobRestart command"; + return "The Engine did not acknowledge the $Action JobControl command"; } if (substr($Reply, 0, 1) eq "1") { diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl index 99c68bc..87a5694 100644 --- a/testbot/web/JobDetails.pl +++ b/testbot/web/JobDetails.pl @@ -158,73 +158,33 @@ sub GetItemActions($) # Actions handling #
-sub GetActions($) +sub CanDoJobControl($$) { - my ($self) = @_; - - # These are mutually exclusive - return ["Cancel job"] if (!defined $self->CanCancel()); - return ["Restart job"] if (!defined $self->CanRestart()); - return []; -} - -sub CanCancel($) -{ - my ($self) = @_; + my ($self, $Action) = @_;
my $Job = $self->{EnclosingPage}->GetJob(); - my $Status = $Job->Status; - if ($Status ne "queued" && $Status ne "running") - { - return "Job already $Status"; - } - my $Session = $self->{EnclosingPage}->GetCurrentSession(); - if (! defined($Session)) + my $CurrentUser = $Session->User if (defined $Session); + if (!$CurrentUser or + (!$CurrentUser->HasRole("admin") and + $Job->User->GetKey() ne $CurrentUser->GetKey())) { - return "You are not authorized to cancel this job"; - } - my $CurrentUser = $Session->User; - if (! $CurrentUser->HasRole("admin") && - $Job->User->GetKey() ne $CurrentUser->GetKey()) - { - return "You are not authorized to cancel this job"; + return "You are not authorized to $Action this job"; }
- return undef; + my %AllowedStatus = ( + "cancel" => {"queued" => 1, "running" => 1}, + "restart" => {"boterror" => 1, "canceled" => 1}, + ); + return $AllowedStatus{$Action}->{$Job->Status} ? undef : + "Cannot $Action a ". $Job->Status ." job"; }
-sub CanRestart($) +sub OnJobControl($$) { - my ($self) = @_; - - my $Job = $self->{EnclosingPage}->GetJob(); - my $Status = $Job->Status; - if ($Status ne "boterror" && $Status ne "canceled") - { - return "Not a failed / canceled Job"; - } - - my $Session = $self->{EnclosingPage}->GetCurrentSession(); - if (! defined($Session)) - { - return "You are not authorized to restart this job"; - } - my $CurrentUser = $Session->User; - if (! $CurrentUser->HasRole("admin") && - $Job->User->GetKey() ne $CurrentUser->GetKey()) # FIXME: Admin only? - { - return "You are not authorized to restart this job"; - } - - return undef; -} - -sub OnCancel($) -{ - my ($self) = @_; + my ($self, $Action) = @_;
- my $ErrMessage = $self->CanCancel(); + my $ErrMessage = $self->CanDoJobControl($Action); if (defined $ErrMessage) { $self->{EnclosingPage}->SetError(undef, $ErrMessage); @@ -232,7 +192,7 @@ sub OnCancel($) }
my $JobId = $self->{EnclosingPage}->GetJob()->Id; - $ErrMessage = JobCancel($JobId); + $ErrMessage = JobControl($Action, $JobId); if (defined $ErrMessage) { $self->{EnclosingPage}->SetError(undef, $ErrMessage); @@ -240,43 +200,28 @@ sub OnCancel($) }
# Ideally this would use something like GetMoreInfoLink() to rebuild a Get - # URL to preserve which logs and screenshots have been expanded. But the - # anchor would likely be lost anyway. + # URL to preserve which logs and screenshots have been expanded. But if the + # job got restarted those would be gone anyway. # So just do a basic reload to refresh the tasks' status. exit($self->{EnclosingPage}->Redirect($ENV{"SCRIPT_NAME"} ."?Key=$JobId")); }
-sub OnRestart($) +sub GetActions($) { my ($self) = @_;
- my $ErrMessage = $self->CanRestart(); - if (defined $ErrMessage) - { - $self->{EnclosingPage}->SetError(undef, $ErrMessage); - return !1; - } - - my $JobId = $self->{EnclosingPage}->GetJob()->Id; - $ErrMessage = JobRestart($JobId); - if (defined $ErrMessage) - { - $self->{EnclosingPage}->SetError(undef, $ErrMessage); - return !1; - } - - # Reload to refresh the tasks' status. Note that all logs and screenshots - # have been cleared so there is no point specifying which one to open or - # adding an anchor. - exit($self->{EnclosingPage}->Redirect($ENV{"SCRIPT_NAME"} ."?Key=$JobId")); + # These are mutually exclusive + return ["Cancel job"] if (!defined $self->CanDoJobControl("cancel")); + return ["Restart job"] if (!defined $self->CanDoJobControl("restart")); + return []; }
sub OnAction($$) { my ($self, $Action) = @_;
- return $Action eq "Cancel job" ? $self->OnCancel() : - $Action eq "Restart job" ? $self->OnRestart() : + return $Action eq "Cancel job" ? $self->OnJobControl("cancel") : + $Action eq "Restart job" ? $self->OnJobControl("restart") : $self->SUPER::OnAction($Action); }