Merge pull request #5035 from Martchus/carry-over-message

Add note about hook script in carry over comment
os-autoinst · Mar 15, 2023 · d3ce35d · d3ce35d
2 parents 691c69c + df0f3db
commit d3ce35d
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 25 deletions.
diff --git a/lib/OpenQA/Schema/Result/Jobs.pm b/lib/OpenQA/Schema/Result/Jobs.pm
@@ -1658,6 +1658,22 @@ sub _failure_reason ($self) {
     return keys %failed_modules ? (join(',', sort keys %failed_modules) || $self->result) : 'GOOD';
 }
 
+=head2 hook_script
+
+Returns the hook script for this job depending on its result and settings and the global configuration.
+
+=cut
+sub hook_script ($self) {
+    my $trigger_hook = $self->settings_hash->{_TRIGGER_JOB_DONE_HOOK};
+    return undef if defined $trigger_hook && !$trigger_hook;
+    return undef unless my $result = $self->result;
+    my $hooks = OpenQA::App->singleton->config->{hooks};
+    my $key = "job_done_hook_$result";
+    my $hook = $ENV{'OPENQA_' . uc $key} // $hooks->{lc $key};
+    $hook = $hooks->{job_done_hook} if !$hook && ($trigger_hook || $hooks->{"job_done_hook_enable_$result"});
+    return $hook;
+}
+
 sub _carry_over_candidate ($self) {
     my $current_failure_reason = $self->_failure_reason;
     my $app = OpenQA::App->singleton;
@@ -1708,22 +1724,18 @@ result in the same scenario.
 =cut
 sub carry_over_bugrefs ($self) {
     if (my $group = $self->group) { return undef unless $group->carry_over_bugrefs }
-
-    my $prev = $self->_carry_over_candidate;
-    return undef if !$prev;
+    return undef unless my $prev = $self->_carry_over_candidate;
 
     my $comments = $prev->comments->search({}, {order_by => {-desc => 'me.id'}});
-
-    while (my $comment = $comments->next) {
-        next if !($comment->bugref);
-
+    for my $comment ($comments->all) {
+        next unless $comment->bugref;
         my $text = $comment->text;
         my $prev_id = $prev->id;
-        if ($text !~ "Automatic takeover") {
-            $text .= "\n\n(Automatic takeover from t#$prev_id)\n";
-        }
-        my %newone = (text => $text);
-        $newone{user_id} = $comment->user_id;
+        $text .= "\n\n(Automatic takeover from t#$prev_id)" if $text !~ qr/Automatic takeover/;
+        $text .= "\n(The hook script will not be executed.)"
+          if $text !~ qr/The hook script will not be executed/ && defined $self->hook_script;
+        $text .= "\n" unless substr($text, -1, 1) eq "\n";
+        my %newone = (text => $text, user_id => $comment->user_id);
         $self->comments->create_with_event(\%newone, {taken_over_from_job_id => $prev_id});
         return 1;
     }

diff --git a/lib/OpenQA/Task/Job/FinalizeResults.pm b/lib/OpenQA/Task/Job/FinalizeResults.pm
@@ -43,20 +43,11 @@ sub _finalize_results ($minion_job, $openqa_job_id = undef, $carried_over = unde
 }
 
 sub _run_hook_script ($minion_job, $openqa_job, $app, $guard) {
-    my $settings = $openqa_job->settings_hash;
-    my $trigger_hook = $settings->{_TRIGGER_JOB_DONE_HOOK};
-
-    return undef if defined $trigger_hook && !$trigger_hook;
-    return undef unless my $result = $openqa_job->result;
-
-    my $hooks = $app->config->{hooks};
-    my $key = "job_done_hook_$result";
-    my $hook = $ENV{'OPENQA_' . uc $key} // $hooks->{lc $key};
-    $hook = $hooks->{job_done_hook} if !$hook && ($trigger_hook || $hooks->{"job_done_hook_enable_$result"});
-    return undef unless $hook;
+    return undef unless my $hook = $openqa_job->hook_script;
 
     my $timeout = $ENV{OPENQA_JOB_DONE_HOOK_TIMEOUT} // '5m';
     my $kill_timeout = $ENV{OPENQA_JOB_DONE_HOOK_KILL_TIMEOUT} // '30s';
+    my $settings = $openqa_job->settings_hash;
     my $delay = $settings->{_TRIGGER_JOB_DONE_DELAY} // $ENV{OPENQA_JOB_DONE_HOOK_DELAY} // ONE_MINUTE;
     my $retries = $settings->{_TRIGGER_JOB_DONE_RETRIES} // $ENV{OPENQA_JOB_DONE_HOOK_RETRIES} // 1440;
     my $skip_rc = $settings->{_TRIGGER_JOB_DONE_SKIP_RC} // $ENV{OPENQA_JOB_DONE_HOOK_SKIP_RC} // 142;

diff --git a/t/17-labels_carry_over.t b/t/17-labels_carry_over.t
@@ -27,6 +27,7 @@ assume_all_assets_exist;
 
 my $comment_must
   = '<a href="https://bugzilla.suse.com/show_bug.cgi?id=1234">bsc#1234</a>(Automatic takeover from <a href="/tests/99962">t#99962</a>)';
+my $carry_over_note = "\n(The hook script will not be executed.)";
 sub comments ($url) {
     $t->get_ok("$url/comments_ajax")->status_is(200)->tx->res->dom->find('.media-comment > p')->map('content');
 }
@@ -61,7 +62,9 @@ subtest '"happy path": failed->failed carries over last issue reference' => sub
         is_deeply(comments('/tests/99963'), [], 'no bugrefs carried over');
     };
 
-    subtest 'carry over enabled in job group settings' => sub {
+    subtest 'carry over enabled in job group settings, note about hook script' => sub {
+        local $ENV{OPENQA_JOB_DONE_HOOK_FAILED} = 'foo';
+
         $t->app->log->level('debug');
         $group->update({carry_over_bugrefs => 1});
         my $output = combined_from {
@@ -70,7 +73,7 @@ subtest '"happy path": failed->failed carries over last issue reference' => sub
         $t->app->log->level('error');
 
         my @comments_current = @{comments('/tests/99963')};
-        is(join('', @comments_current), $comment_must, 'only one bugref is carried over');
+        is(join('', @comments_current), $comment_must . $carry_over_note, 'only one bugref is carried over');
         like($comments_current[0], qr/\Q$second_label/, 'last entered bugref found, it is expanded');
         like $output, qr{\Q_carry_over_candidate(99963): _failure_reason=amarok:none};
         like $output, qr{\Q_carry_over_candidate(99963): checking take over from 99962: _failure_reason=amarok:none};