Drop use of _ADVISORY_REPO_DONE, always use file tests

This is safer if the advisory stuff was done on a previous test
run. Hilariously, this exposed a dumb mistake I made years ago
in installedtest.pm and never noticed before: the calls to
advisory_* at the bottom of that file are meant to be in the
post_fail_hook, but they weren't, which meant they got called
by the scheduler. This didn't cause any failures because the
first line caused them to return immediately based on a get_var
call (which it's OK to do in the scheduler), but changing it
to a script_run call (which it's *not* OK to do in scheduling)
caused all the tests to blow up immediately and confused me
*a lot* until I spotted this!

Signed-off-by: Adam Williamson <awilliam@redhat.com>

lib/installedtest.pm

@@ -158,12 +158,13 @@ sub post_fail_hook {
         upload_logs "/var/tmp/imgbuild/lorax.log", failok => 1;
         upload_logs "/var/tmp/imgbuild/program.log", failok => 1;
     }
-}
 
-# For update tests, let's do the update package info log stuff,
+    # For update tests, let's do the update package info log stuff,
-# it may be useful for diagnosing the cause of the failure
+    # it may be useful for diagnosing the cause of the failure
-advisory_get_installed_packages;
+    advisory_get_installed_packages;
-advisory_check_nonmatching_packages(fatal => 0);
+    advisory_check_nonmatching_packages(fatal => 0);
+
+}
 
 1;
 

lib/utils.pm

@@ -553,7 +553,7 @@ sub _repo_setup_compose {
 sub _repo_setup_updates {
     # Appropriate repo setup steps for testing a Bodhi update
     # Check if we already ran, bail if so
-    return if (get_var("_ADVISORY_REPO_DONE"));
+    return unless script_run "test -f /mnt/updatepkgs.txt";
     my $version = get_var("VERSION");
     my $currrel = get_var("CURRREL", "0");
     my $arch = get_var("ARCH");
@@ -677,9 +677,6 @@ sub _repo_setup_updates {
         type_string "exit\n";
         wait_still_screen 5;
     }
-    # mark via a variable that we've set up the update/task repo and done
-    # all the logging stuff above
-    set_var('_ADVISORY_REPO_DONE', '1');
 }
 
 sub repo_setup {
@@ -1106,12 +1103,14 @@ sub quit_with_shortcut {
 
 }
 
+# For update tests (this only works if we've been through
+# _repo_setup_updates), figure out which packages from the update
+# are currently installed. This is here so we can do it both in
+# _advisory_post and post_fail_hook.
 sub advisory_get_installed_packages {
-    # For update tests (this only works if we've been through
+    # bail out if the file doesn't exist: this is in case we get
-    # _repo_setup_updates), figure out which packages from the update
+    # here in the post-fail hook but we failed before creating it
-    # are currently installed. This is here so we can do it both in
+    return if script_run "test -f /mnt/updatepkgs.txt";
-    # _advisory_post and post_fail_hook.
-    return unless (get_var("_ADVISORY_REPO_DONE"));
     assert_script_run 'rpm -qa --qf "%{SOURCERPM} %{EPOCH} %{NAME}-%{VERSION}-%{RELEASE}\n" | sort -u > /tmp/allpkgs.txt', timeout => 90;
     # this finds lines which appear in both files
     # http://www.unix.com/unix-for-dummies-questions-and-answers/34549-find-matching-lines-between-2-files.html
@@ -1137,7 +1136,9 @@ sub advisory_check_nonmatching_packages {
         fatal => 1,
         @_
     );
-    return unless (get_var("_ADVISORY_REPO_DONE"));
+    # bail out if the file doesn't exist: this is in case we get
+    # here in the post-fail hook but we failed before creating it
+    return if script_run "test -f /mnt/updatepkgnames.txt";
     # if this fails in advisory_post, we don't want to do it *again*
     # unnecessarily in post_fail_hook
     return if (get_var("_ACNMP_DONE"));