Merge pull request #6047 from Martchus/var-exp

Extend variable expansion to consider variables from worker config

docs/UsersGuide.asciidoc

@@ -193,28 +193,36 @@ by different means:
 
 
 === Variable expansion
+Any job setting can refer to another variable using this syntax: `%NAME%`. When
+the test job is created, the string will be substituted with the value of the
+specified variable at that time.
 
-Any variable defined in Test Suite, Machine, Product or Job Template table can
-refer to another variable using this syntax: `%NAME%`. When the test job is created,
-the string will be substituted with the value of the specified variable at that time.
+The variable expansion applies to job settings defined in test suites, machines,
+products and job templates. It also applies to job settings specified when
+creating a single set of jobs and to variables specified in the worker config.
 
-For example this variable defined for Test Suite:
+Consider this example where a variable is defined within a test suite:
 
 [source,sh]
 --------------------------------------------------------------------------------
 PUBLISH_HDD_1 = %DISTRI%-%VERSION%-%ARCH%-%DESKTOP%.qcow2
 --------------------------------------------------------------------------------
 
-may be expanded to this job variable:
+It may expanded to this job setting:
 
 [source,sh]
 --------------------------------------------------------------------------------
 PUBLISH_HDD_1 = opensuse-13.1-i586-kde.qcow2
 --------------------------------------------------------------------------------
 
-=== Variable precedence
+NOTE: Variables from the worker config are not considered if such a variable is
+also present in any of the other mentioned places. To give variable values from
+the worker config precedence, use double percentage signs. Expansions by the
+worker will *not* be shown in the "Settings" tab on the web UI. They are only
+present in `vars.json` and `worker-log.txt`.
 
-It's possible to define the same variable in multiple places that would all be
+=== Variable precedence
+It is possible to define the same variable in multiple places that would all be
 used for a single job - for instance, you may have a variable defined in both
 a test suite and a product that appear in the same job template. The precedence
 order for variables is as follows (from lowest to highest):

lib/OpenQA/JobSettings.pm

@@ -52,29 +52,32 @@ sub generate_settings ($params) {
 }
 
 # replace %NAME% with $settings{NAME} (but not %%NAME%%)
-sub expand_placeholders ($settings) {
+sub expand_placeholders ($settings, $on_web_ui = 1) {
     for my $value (values %$settings) {
         next unless defined $value;
         my %visited_placeholders;
-        eval { $value =~ s/(%+)(\w+)(%+)/_expand_placeholder($settings, $2, $1, $3, \%visited_placeholders)/eg };
+        eval {
+            $value =~ s/(%+)(\w+)(%+)/_expand_placeholder($settings, $2, $1, $3, \%visited_placeholders, $on_web_ui)/eg;
+        };
         return "Error: $@" if $@;
     }
     return undef;
 }
 
-sub _expand_placeholder ($settings, $key, $start, $end, $visited_placeholders_in_parent_scope) {
-    return '' unless defined $settings->{$key};
+sub _expand_placeholder ($settings, $key, $start, $end, $visited_placeholders_in_parent_scope, $on_web_ui) {
+    # handle %CASEDIR% only on web UI; on the worker it is handled by _engine_workit_step_2 separately
+    return "$start$key$end" if !$on_web_ui && $key eq 'CASEDIR';
 
-    my %visited_placeholders = %$visited_placeholders_in_parent_scope;
-    die "The key $key contains a circular reference, its value is $settings->{$key}.\n"
-      if $visited_placeholders{$key}++;
-
-    # if the key is surrounded by more than one % on any side, return the key itself and strip one level of %
+    # return the key itself and strip one level of % if the key is surrounded by more than one % on any side
     return substr($start, 1) . ($key) . substr($end, 0, -1) unless $start eq '%' && $end eq '%';
 
-    # otherwise, substitute the whole %…% expression with the value of the other setting
-    my $value = $settings->{$key};
-    $value =~ s/(%+)(\w+)(%+)/_expand_placeholder($settings, $2, $1, $3, \%visited_placeholders)/eg;
+    # do not replace non-existing keys on web UI level to leave them to the worker
+    return $on_web_ui ? "$start$key$end" : '' unless defined(my $value = $settings->{$key});
+
+    # substitute the whole %…% expression with the value of the other setting
+    my %visited_placeholders = %$visited_placeholders_in_parent_scope;
+    die "The key $key contains a circular reference, its value is $value.\n" if $visited_placeholders{$key}++;
+    $value =~ s/(%+)(\w+)(%+)/_expand_placeholder($settings, $2, $1, $3, \%visited_placeholders, $on_web_ui)/eg;
     return $value;
 }
 

lib/OpenQA/Worker/Engines/isotovideo.pm

@@ -4,6 +4,7 @@
 package OpenQA::Worker::Engines::isotovideo;
 use Mojo::Base -signatures;
 use OpenQA::Constants qw(WORKER_SR_DONE WORKER_EC_CACHE_FAILURE WORKER_EC_ASSET_FAILURE WORKER_SR_DIED);
+use OpenQA::JobSettings;
 use OpenQA::Log qw(log_error log_info log_debug log_warning get_channel_handle format_settings);
 use OpenQA::Utils
   qw(asset_type_from_setting base_host locate_asset looks_like_url_with_scheme testcasedir productdir needledir);
@@ -319,6 +320,10 @@ sub engine_workit ($job, $callback) {
         WORKER_ID => $workerid,
         %$job_settings
     );
+
+    # do final variable expansion so placeholders of variables defined in worker config are replaced as well
+    OpenQA::JobSettings::expand_placeholders(\%vars, 0);
+
     log_debug "Job settings:\n" . format_settings(\%vars);
 
     # cache/locate assets, set ASSETDIR

t/24-worker-engine.t

@@ -353,7 +353,7 @@ subtest 'syncing tests' => sub {
     is $result, 'cache-dir/webuihost/tests', 'returns synced test directory on success' or diag explain $result;
 };
 
-subtest 'symlink testrepo, logging behavior' => sub {
+subtest 'symlink testrepo, logging behavior, variable expansion' => sub {
     my $pool_directory = tempdir('poolXXXX');
     my $worker = OpenQA::Test::FakeWorker->new(pool_directory => $pool_directory);
     my $client = Test::FakeClient->new;
@@ -393,7 +393,8 @@ subtest 'symlink testrepo, logging behavior' => sub {
     };
 
     my @custom_casedir_settings = (
-        CASEDIR => 'https://github.com/foo/os-autoinst-distri-example.git#master',
+        CASEDIR_DOMAIN => 'github.com',
+        CASEDIR => 'https://%CASEDIR_DOMAIN%/foo/os-autoinst-distri-example.git#master',
         NEEDLES_DIR => 'fedora/needles',
         DISTRI => 'fedora',
         JOBTOKEN => 'token99916',

t/40-job_settings.t

@@ -121,4 +121,17 @@ subtest 'handle_plus_in_settings' => sub {
     is_deeply($settings, {ISO => 'bar.iso', ARCH => 'x86_64', DISTRI => 'opensuse'}, 'handle the plus correctly');
 };
 
+subtest 'two-pass variable expansion' => sub {
+    my %settings = (FOO => 'http://%BAR%/', NEEDLES_DIR => '%%CASEDIR%%/needles');
+
+    OpenQA::JobSettings::expand_placeholders(\%settings);    # web UI pass
+    is $settings{FOO}, 'http://%BAR%/', 'placeholder referring to non-existing key not removed during web UI pass';
+    is $settings{NEEDLES_DIR}, '%CASEDIR%/needles', 'one level of %-signs stripped during web UI pass';
+
+    OpenQA::JobSettings::expand_placeholders(\%settings, 0);    # worker pass
+    is $settings{FOO}, 'http:///', 'placeholder referring to non-existing key finally removed by worker';
+    is $settings{NEEDLES_DIR}, '%CASEDIR%/needles',
+      '%CASEDIR% preserved during worker pass (handled instead by _engine_workit_step_2)';
+};
+
 done_testing;