Francois Gouget : testbot: Unify the Job cancel and restart actions.
Alexandre Julliard
julliard at winehq.org
Mon May 30 15:31:27 CDT 2022
Module: tools
Branch: master
Commit: 9925dc0c1389bc3c99430de5a6ad6e381300cfed
URL: https://source.winehq.org/git/tools.git/?a=commit;h=9925dc0c1389bc3c99430de5a6ad6e381300cfed
Author: Francois Gouget <fgouget at 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 at codeweavers.com>
Signed-off-by: Alexandre Julliard <julliard at 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);
}
More information about the wine-cvs
mailing list