Skip to content

Commit

Permalink
web: replace 'errormsg' with 'errormsg IS NULL' in most cases
Browse files Browse the repository at this point in the history
This is implement in an extremely hacky way due to poor DBIx feature
support. Ideally, what we'd need is a way to tell DBIx to ignore the
errormsg column unless explicitly requested, and to automatically add a
computed 'errormsg IS NULL' column in others. Since it does not support
that, this commit instead hacks some support via method overrides while
taking care to not break anything obvious.
  • Loading branch information
delroth committed Apr 12, 2024
1 parent 258e931 commit 0795f6c
Show file tree
Hide file tree
Showing 14 changed files with 69 additions and 14 deletions.
1 change: 1 addition & 0 deletions package.nix
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ let
DateTime
DBDPg
DBDSQLite
DBIxClassHelpers
DigestSHA1
EmailMIME
EmailSender
Expand Down
8 changes: 7 additions & 1 deletion src/lib/Hydra/Controller/Jobset.pm
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,13 @@ sub errors_GET {

$c->stash->{template} = 'eval-error.tt';

$self->status_ok($c, entity => $c->stash->{jobset});
my $jobsetName = $c->stash->{params}->{name};
my $jobset = $c->stash->{project}->jobsets->find(
{ name => $jobsetName },
{ '+columns' => { 'errormsg' => 'errormsg' } }
);

$self->status_ok($c, entity => $jobset);
}

# Redirect to the latest finished evaluation of this jobset.
Expand Down
1 change: 1 addition & 0 deletions src/lib/Hydra/Controller/JobsetEval.pm
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ sub errors_GET {

$c->stash->{template} = 'eval-error.tt';

my $eval = $c->model('DB::JobsetEvals')->find($c->stash->{eval}->id, { prefetch => 'evaluationerror' });
$self->status_ok($c, entity => $c->stash->{eval});
}

Expand Down
3 changes: 1 addition & 2 deletions src/lib/Hydra/Helper/Nix.pm
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,7 @@ sub getEvals {

my @evals = $evals_result_set->search(
{ hasnewbuilds => 1 },
{ order_by => "$me.id DESC", rows => $rows, offset => $offset
, prefetch => { evaluationerror => [ ] } });
{ order_by => "$me.id DESC", rows => $rows, offset => $offset });
my @res = ();
my $cache = {};

Expand Down
2 changes: 2 additions & 0 deletions src/lib/Hydra/Schema/Result/EvaluationErrors.pm
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,6 @@ __PACKAGE__->add_column(
"+id" => { retrieve_on_insert => 1 }
);

__PACKAGE__->mk_group_accessors('column' => 'has_error');

1;
2 changes: 2 additions & 0 deletions src/lib/Hydra/Schema/Result/Jobsets.pm
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,8 @@ __PACKAGE__->add_column(
"+id" => { retrieve_on_insert => 1 }
);

__PACKAGE__->mk_group_accessors('column' => 'has_error');

sub supportsDynamicRunCommand {
my ($self) = @_;

Expand Down
22 changes: 22 additions & 0 deletions src/lib/Hydra/Schema/ResultSet/EvaluationErrors.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package Hydra::Schema::ResultSet::EvaluationErrors;

use strict;
use utf8;
use warnings;

use parent 'DBIx::Class::ResultSet';

__PACKAGE__->load_components('Helper::ResultSet::RemoveColumns');

# Exclude expensive error message values unless explicitly requested, and
# replace them with a summary field describing their presence/absence.
sub search_rs {
my ( $class, $query, $attrs ) = @_;

unless (exists $attrs->{'select'} || exists $attrs->{'columns'}) {
$attrs->{'+columns'}->{'has_error'} = 'errormsg IS NULL';
}
push @{ $attrs->{'remove_columns'} }, 'errormsg';

return $class->next::method($query, $attrs);
}
22 changes: 22 additions & 0 deletions src/lib/Hydra/Schema/ResultSet/Jobsets.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package Hydra::Schema::ResultSet::Jobsets;

use strict;
use utf8;
use warnings;

use parent 'DBIx::Class::ResultSet';

__PACKAGE__->load_components('Helper::ResultSet::RemoveColumns');

# Exclude expensive error message values unless explicitly requested, and
# replace them with a summary field describing their presence/absence.
sub search_rs {
my ( $class, $query, $attrs ) = @_;

unless (exists $attrs->{'select'} || exists $attrs->{'columns'}) {
$attrs->{'+columns'}->{'has_error'} = 'errormsg IS NULL';
}
push @{ $attrs->{'remove_columns'} }, 'errormsg';

return $class->next::method($query, $attrs);
}
4 changes: 2 additions & 2 deletions src/root/common.tt
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ BLOCK renderEvals %]
ELSE %]
-
[% END %]
[% IF eval.evaluationerror.errormsg %]
[% IF eval.evaluationerror.has_error %]
<span class="badge badge-warning">Eval Errors</span>
[% END %]
</td>
Expand Down Expand Up @@ -639,7 +639,7 @@ BLOCK renderJobsetOverview %]
<td>[% HTML.escape(j.description) %]</td>
<td>[% IF j.lastcheckedtime;
INCLUDE renderDateTime timestamp = j.lastcheckedtime;
IF j.errormsg || j.fetcherrormsg; %]&nbsp;<span class = 'badge badge-warning'>Error</span>[% END;
IF j.has_error || j.fetcherrormsg; %]&nbsp;<span class = 'badge badge-warning'>Error</span>[% END;
ELSE; "-";
END %]</td>
[% IF j.get_column('nrtotal') > 0 %]
Expand Down
4 changes: 2 additions & 2 deletions src/root/jobset-eval.tt
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ c.uri_for(c.controller('JobsetEval').action_for('view'),
[% END %]
<li class="nav-item"><a class="nav-link" href="#tabs-inputs" data-toggle="tab">Inputs</a></li>

[% IF eval.evaluationerror.errormsg %]
[% IF eval.evaluationerror.has_error %]
<li class="nav-item"><a class="nav-link" href="#tabs-errors" data-toggle="tab"><span class="text-warning">Evaluation Errors</span></a></li>
[% END %]
</ul>
Expand Down Expand Up @@ -165,7 +165,7 @@ c.uri_for(c.controller('JobsetEval').action_for('view'),
[% END %]
</div>

[% IF eval.evaluationerror.errormsg %]
[% IF eval.evaluationerror.has_error %]
<div id="tabs-errors" class="tab-pane">
<iframe src="[% c.uri_for(c.controller('JobsetEval').action_for('errors'), [eval.id], params) %]" loading="lazy" frameBorder="0" width="100%"></iframe>
</div>
Expand Down
6 changes: 3 additions & 3 deletions src/root/jobset.tt
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
[% END %]

<li class="nav-item"><a class="nav-link active" href="#tabs-evaluations" data-toggle="tab">Evaluations</a></li>
[% IF jobset.errormsg || jobset.fetcherrormsg %]
[% IF jobset.has_error || jobset.fetcherrormsg %]
<li class="nav-item"><a class="nav-link" href="#tabs-errors" data-toggle="tab"><span class="text-warning">Evaluation Errors</span></a></li>
[% END %]
<li class="nav-item"><a class="nav-link" href="#tabs-jobs" data-toggle="tab">Jobs</a></li>
Expand All @@ -79,7 +79,7 @@
<th>Last checked:</th>
<td>
[% IF jobset.lastcheckedtime %]
[% INCLUDE renderDateTime timestamp = jobset.lastcheckedtime %], [% IF jobset.errormsg || jobset.fetcherrormsg %]<em class="text-warning">with errors!</em>[% ELSE %]<em>no errors</em>[% END %]
[% INCLUDE renderDateTime timestamp = jobset.lastcheckedtime %], [% IF jobset.has_error || jobset.fetcherrormsg %]<em class="text-warning">with errors!</em>[% ELSE %]<em>no errors</em>[% END %]
[% ELSE %]
<em>never</em>
[% END %]
Expand Down Expand Up @@ -117,7 +117,7 @@

</div>

[% IF jobset.errormsg || jobset.fetcherrormsg %]
[% IF jobset.has_error || jobset.fetcherrormsg %]
<div id="tabs-errors" class="tab-pane">
<iframe src="[% c.uri_for('/jobset' project.name jobset.name "errors") %]" loading="lazy" frameBorder="0" width="100%"></iframe>
</div>
Expand Down
2 changes: 1 addition & 1 deletion t/evaluator/evaluate-constituents-broken.t
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ like(
"The stderr record includes a relevant error message"
);

$jobset->discard_changes; # refresh from DB
$jobset->discard_changes({ '+columns' => {'errormsg' => 'errormsg'} }); # refresh from DB
like(
$jobset->errormsg,
qr/aggregate job ‘mixed_aggregate’ failed with the error: constituentA: does not exist/,
Expand Down
4 changes: 2 additions & 2 deletions t/lib/CliRunners.pm
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ our @EXPORT = qw(
sub evalSucceeds {
my ($jobset) = @_;
my ($res, $stdout, $stderr) = captureStdoutStderr(60, ("hydra-eval-jobset", $jobset->project->name, $jobset->name));
$jobset->discard_changes; # refresh from DB
$jobset->discard_changes({ '+columns' => {'errormsg' => 'errormsg'} }); # refresh from DB
if ($res) {
chomp $stdout; chomp $stderr;
utf8::decode($stdout) or die "Invalid unicode in stdout.";
Expand All @@ -29,7 +29,7 @@ sub evalSucceeds {
sub evalFails {
my ($jobset) = @_;
my ($res, $stdout, $stderr) = captureStdoutStderr(60, ("hydra-eval-jobset", $jobset->project->name, $jobset->name));
$jobset->discard_changes; # refresh from DB
$jobset->discard_changes({ '+columns' => {'errormsg' => 'errormsg'} }); # refresh from DB
if (!$res) {
chomp $stdout; chomp $stderr;
utf8::decode($stdout) or die "Invalid unicode in stdout.";
Expand Down
2 changes: 1 addition & 1 deletion t/queue-runner/direct-indirect-constituents.t
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ my $constituentBuildA = $builds->{"constituentA"};
my $constituentBuildB = $builds->{"constituentB"};

my $eval = $constituentBuildA->jobsetevals->first();
is($eval->evaluationerror->errormsg, "");
is($eval->evaluationerror->has_error, 0);

subtest "Verifying the direct aggregate" => sub {
my $aggBuild = $builds->{"direct_aggregate"};
Expand Down

0 comments on commit 0795f6c

Please sign in to comment.