os-autoinst · okurz · Mar 27, 2020 · Mar 27, 2020
diff --git a/lib/OpenQA/JobSettings.pm → lib/OpenQA/ExpandPlaceholder.pm b/lib/OpenQA/JobSettings.pm → lib/OpenQA/ExpandPlaceholder.pm
@@ -1,4 +1,4 @@
-# Copyright (C) 2019-2020 SUSE LLC
+# Copyright (C) 2019 SUSE LLC
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -12,7 +12,7 @@
 #
 # You should have received a copy of the GNU General Public License
 
-package OpenQA::JobSettings;
+package OpenQA::ExpandPlaceholder;
 
 use strict;
 use warnings;
@@ -50,18 +50,4 @@ sub _expand_placeholder {
     return $value;
 }
 
-# allow some messing with the usual precedence order. If anything
-# sets +VARIABLE, that setting will be used as VARIABLE regardless
-# (so a product or template +VARIABLE beats a post'ed VARIABLE).
-# if *multiple* things set +VARIABLE, whichever comes highest in
-# the usual precedence order wins.
-sub handle_plus_in_settings {
-    my ($settings) = @_;
-    for (keys %$settings) {
-        if (substr($_, 0, 1) eq '+') {
-            $settings->{substr($_, 1)} = delete $settings->{$_};
-        }
-    }
-}
-
 1;
diff --git a/lib/OpenQA/Schema/Result/ScheduledProducts.pm b/lib/OpenQA/Schema/Result/ScheduledProducts.pm
@@ -25,7 +25,7 @@ use File::Basename;
 use Try::Tiny;
 use OpenQA::App;
 use OpenQA::Utils;
-use OpenQA::JobSettings;
+use OpenQA::ExpandPlaceholder;
 use OpenQA::JobDependencies::Constants;
 use OpenQA::Scheduler::Client;
 use Mojo::JSON qw(encode_json decode_json);
@@ -559,11 +559,20 @@ sub _generate_jobs {
             $settings{PRIO}     = defined($priority) ? $priority : $job_template->prio;
             $settings{GROUP_ID} = $job_template->group_id;
 
-            OpenQA::JobSettings::handle_plus_in_settings(\%settings);
+            # allow some messing with the usual precedence order. If anything
+            # sets +VARIABLE, that setting will be used as VARIABLE regardless
+            # (so a product or template +VARIABLE beats a post'ed VARIABLE).
+            # if *multiple* things set +VARIABLE, whichever comes highest in
+            # the usual precedence order wins.
+            for (keys %settings) {
+                if (substr($_, 0, 1) eq '+') {
+                    $settings{substr($_, 1)} = delete $settings{$_};
+                }
+            }
 
             # variable expansion
             # replace %NAME% with $settings{NAME}
-            my $error = OpenQA::JobSettings::expand_placeholders(\%settings);
+            my $error = OpenQA::ExpandPlaceholder::expand_placeholders(\%settings);
             $error_message .= $error if defined $error;
 
             if (!$args->{MACHINE} || $args->{MACHINE} eq $settings{MACHINE}) {

diff --git a/lib/OpenQA/Script.pm b/lib/OpenQA/Script.pm
@@ -42,7 +42,6 @@ sub clone_job_apply_settings {
     my ($argv, $depth, $settings, $options) = @_;
 
     delete $settings->{NAME};    # usually autocreated
-    $settings->{is_clone_job} = 1;    # used to figure out if this is a clone operation
 
     for my $arg (@$argv) {
         # split arg into key and value

diff --git a/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm b/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm
@@ -17,7 +17,7 @@ package OpenQA::WebAPI::Controller::API::V1::Job;
 use Mojo::Base 'Mojolicious::Controller';
 
 use OpenQA::Utils qw(:DEFAULT assetdir);
-use OpenQA::JobSettings;
+use OpenQA::ExpandPlaceholder;
 use OpenQA::Jobs::Constants;
 use OpenQA::Resource::Jobs;
 use OpenQA::Schema::Result::Jobs;
@@ -239,9 +239,8 @@ is mandatory and should be the name of the test.
 =cut
 
 sub create {
-    my $self         = shift;
-    my $params       = $self->req->params->to_hash;
-    my $is_clone_job = delete $params->{is_clone_job} // 0;
+    my $self   = shift;
+    my $params = $self->req->params->to_hash;
 
     # job_create expects upper case keys
     my %up_params = map { uc $_ => $params->{$_} } keys %$params;
@@ -253,15 +252,12 @@ sub create {
 
     my $json = {};
     my $status;
-    my $job_settings = \%params;
-    if (!$is_clone_job) {
-        my $result = $self->_generate_job_setting(\%params);
-        return $self->render(json => {error => $result->{error_message}}, status => 400)
-          if defined $result->{error_message};
-        $job_settings = $result->{settings_result};
-    }
+    my $result = $self->_generate_job_setting(\%params);
+    return $self->render(json => {error => $result->{error_message}}, status => 400)
+      if defined $result->{error_message};
+
     try {
-        my $job = $self->schema->resultset('Jobs')->create_from_settings($job_settings);
+        my $job = $self->schema->resultset('Jobs')->create_from_settings($result->{settings_result});
         $self->emit_event('openqa_job_create', {id => $job->id, %params});
         $json->{id} = $job->id;
 
@@ -855,9 +851,7 @@ sub _generate_job_setting {
     $settings{WORKER_CLASS} = join ',', sort @classes if @classes > 0;
     $settings{uc $_} = $args->{$_} for keys %$args;
 
-    OpenQA::JobSettings::handle_plus_in_settings(\%settings);
-
-    my $error_message = OpenQA::JobSettings::expand_placeholders(\%settings);
+    my $error_message = OpenQA::ExpandPlaceholder::expand_placeholders(\%settings);
     return {error_message => $error_message, settings_result => \%settings};
 }
 

diff --git a/t/35-script_clone_job.t b/t/35-script_clone_job.t
@@ -64,14 +64,12 @@ subtest 'clone job apply settings tests' => sub {
     $test_settings{HDD_1}        = 'new.qcow2';
     $test_settings{HDDSIZEGB}    = 40;
     $test_settings{WORKER_CLASS} = 'local';
-    $test_settings{is_clone_job} = 1;
     delete $test_settings{NAME};
     clone_job_apply_settings(\@argv, 0, \%child_settings, \%options);
     is_deeply(\%child_settings, \%test_settings, 'cloned child job with correct global setting and new settings');
 
-    %test_settings               = %parent_settings;
+    %test_settings = %parent_settings;
     $test_settings{WORKER_CLASS} = 'local';
-    $test_settings{is_clone_job} = 1;
     delete $test_settings{NAME};
     clone_job_apply_settings(\@argv, 1, \%parent_settings, \%options);
     is_deeply(\%parent_settings, \%test_settings, 'cloned parent job only take global setting');
@@ -80,16 +78,16 @@ subtest 'clone job apply settings tests' => sub {
 subtest '_GROUP and _GROUP_ID override each other' => sub {
     my %settings = ();
     clone_job_apply_settings([qw(_GROUP=foo _GROUP_ID=bar)], 0, \%settings, \%options);
-    is_deeply(\%settings, {_GROUP_ID => 'bar', is_clone_job => 1}, '_GROUP_ID overrides _GROUP');
+    is_deeply(\%settings, {_GROUP_ID => 'bar'}, '_GROUP_ID overrides _GROUP');
     %settings = ();
     clone_job_apply_settings([qw(_GROUP_ID=bar _GROUP=foo)], 0, \%settings, \%options);
-    is_deeply(\%settings, {_GROUP => 'foo', is_clone_job => 1}, '_GROUP overrides _GROUP_ID');
+    is_deeply(\%settings, {_GROUP => 'foo'}, '_GROUP overrides _GROUP_ID');
 };
 
 subtest 'delete empty setting' => sub {
     my %settings = ();
     clone_job_apply_settings([qw(ISO_1= ADDONS=)], 0, \%settings, \%options);
-    is_deeply(\%settings, {is_clone_job => 1}, 'all empty settings removed');
+    is_deeply(\%settings, {}, 'all empty settings removed');
 };
 
 subtest 'asset download' => sub {

diff --git a/t/40-job_settings.t → t/40-expand_placeholders.t b/t/40-job_settings.t → t/40-expand_placeholders.t
@@ -20,7 +20,7 @@ use warnings;
 use FindBin;
 use lib "$FindBin::Bin/lib";
 use Test::More;
-use OpenQA::JobSettings;
+use OpenQA::ExpandPlaceholder;
 
 my $settings = {
     BUILD_SDK                => '%BUILD_HA%',
@@ -57,7 +57,7 @@ my $settings = {
 };
 
 subtest expand_placeholders => sub {
-    my $error          = OpenQA::JobSettings::expand_placeholders($settings);
+    my $error          = OpenQA::ExpandPlaceholder::expand_placeholders($settings);
     my $match_settings = {
         BUILD_SDK                => '1234',
         BETA                     => 1,
@@ -109,21 +109,10 @@ subtest circular_reference => sub {
         MACHINE       => '64bit',
     };
     like(
-        OpenQA::JobSettings::expand_placeholders($circular_settings),
+        OpenQA::ExpandPlaceholder::expand_placeholders($circular_settings),
         qr/The key (\w+) contains a circular reference, its value is %\w+%/,
         "circular reference exit successfully"
     );
 };
 
-subtest 'handle_plus_in_settings' => sub {
-    my $settings = {
-        'ISO'    => 'foo.iso',
-        '+ISO'   => 'bar.iso',
-        '+ARCH'  => 'x86_64',
-        'DISTRI' => 'opensuse',
-    };
-    OpenQA::JobSettings::handle_plus_in_settings($settings);
-    is_deeply($settings, {ISO => 'bar.iso', ARCH => 'x86_64', DISTRI => 'opensuse'}, 'handle the plus correctly');
-};
-
 done_testing;
diff --git a/t/api/04-jobs.t b/t/api/04-jobs.t
@@ -30,7 +30,6 @@ use OpenQA::File;
 use OpenQA::Test::Case;
 use OpenQA::Jobs::Constants;
 use OpenQA::Client;
-use OpenQA::Script;
 use Mojo::IOLoop;
 use Mojo::File qw(path tempfile tempdir);
 use Digest::MD5;
@@ -1304,56 +1303,6 @@ subtest 'marking job as done' => sub {
     };
 };
 
-subtest 'handle + in settings when creating a job' => sub {
-    $t->app->schema->resultset('Products')->create(
-        {
-            version     => '12-SP5',
-            name        => '',
-            distri      => 'sle',
-            arch        => 'x86_64',
-            description => '',
-            flavor      => 'DVD',
-            settings    => [
-                {key => 'BETA',         value => '1'},
-                {key => '+ISO_MAXSIZE', value => '4700372992'},
-                {key => '+ISO',         value => 'foo.iso'}
-            ],
-        });
-    $t->app->schema->resultset('TestSuites')->create(
-        {
-            name        => 'handle_plus',
-            description => '',
-            settings    => [{key => 'DESKTOP', value => 'gnome'}, {key => 'ISO_MAXSIZE', value => '50000000'},],
-        });
-    my $params = {
-        VERSION => '12-SP5',
-        DISTRI  => 'sle',
-        ARCH    => 'x86_64',
-        FLAVOR  => 'DVD',
-        TEST    => 'handle_plus',
-        ISO     => 'SLE-12-SP5-Server-DVD-x86_64-GM-DVD1.iso'
-    };
-    $t->post_ok('/api/v1/jobs', form => $params)->status_is(200);
-    my $result = $jobs->find($t->tx->res->json->{id})->settings_hash;
-    $params->{ISO}          = 'foo.iso';
-    $params->{ISO_MAXSIZE}  = '4700372992';
-    $params->{BETA}         = '1';
-    $params->{DESKTOP}      = 'gnome';
-    $params->{WORKER_CLASS} = 'qemu_x86_64';
-    delete $result->{NAME};
-    is_deeply($result, $params, 'handle + in settings correctly');
-};
-
-subtest 'do not re-generate settings when cloning job' => sub {
-    my $job_settings = $jobs->search({test => 'handle_plus'})->first->settings_hash;
-    clone_job_apply_settings([qw(BETA= DESKTOP=)], 0, $job_settings, {});
-    $t->post_ok('/api/v1/jobs', form => $job_settings)->status_is(200);
-    my $new_job_settings = $jobs->find($t->tx->res->json->{id})->settings_hash;
-    delete $job_settings->{is_clone_job};
-    delete $new_job_settings->{NAME};
-    is_deeply($new_job_settings, $job_settings, 'did not re-generate settings');
-};
-
 # delete the job with a registered job module
 $t->delete_ok('/api/v1/jobs/99937')->status_is(200);
 $t->get_ok('/api/v1/jobs/99937')->status_is(404);