From 8666a6b87fe1e6f140fa49c0ff9e430bc7877322 Mon Sep 17 00:00:00 2001 From: Amrysliu Date: Tue, 28 May 2019 15:32:07 +0800 Subject: [PATCH] Rewrite code for job settings expansion * Move expand placeholders function to its own module. * rewrite the loop. See: https://progress.opensuse.org/issues/20464 --- lib/OpenQA/Schema/Result/ScheduledProducts.pm | 20 +-- lib/OpenQA/Settings.pm | 55 ++++++++ lib/OpenQA/Utils.pm | 25 ---- lib/OpenQA/WebAPI/Controller/API/V1/Job.pm | 22 ++-- t/40-settings.t | 120 ++++++++++++++++++ t/api/04-jobs.t | 105 +++++++++++++-- 6 files changed, 292 insertions(+), 55 deletions(-) create mode 100644 lib/OpenQA/Settings.pm create mode 100644 t/40-settings.t diff --git a/lib/OpenQA/Schema/Result/ScheduledProducts.pm b/lib/OpenQA/Schema/Result/ScheduledProducts.pm index 7e2f94d2e76c..de883901aa62 100644 --- a/lib/OpenQA/Schema/Result/ScheduledProducts.pm +++ b/lib/OpenQA/Schema/Result/ScheduledProducts.pm @@ -25,6 +25,7 @@ use DBIx::Class::Timestamps 'now'; use File::Basename; use Try::Tiny; use OpenQA::Utils; +use OpenQA::Settings; use OpenQA::JobDependencies::Constants; use Mojo::JSON qw(encode_json decode_json); use Carp; @@ -218,7 +219,9 @@ sub _schedule_iso { # Any arg name ending in _URL is special: it tells us to download # the file at that URL before running the job my $downloads = create_downloads_list($args); - my $jobs = $self->_generate_jobs($args, \@notes); + my $result = $self->_generate_jobs($args, \@notes); + return {error => $result->{expand_result}} if defined $result->{expand_result}; + my $jobs = $result->{settings_result}; # take some attributes from the first job to guess what old jobs to cancel # note: We should have distri object that decides which attributes are relevant here. @@ -507,6 +510,7 @@ sub _generate_jobs { # allow overriding the priority my $priority = delete $args->{_PRIORITY}; + my $expand_result; for my $product (@products) { # find job templates my $templates = $product->job_templates; @@ -565,23 +569,23 @@ sub _generate_jobs { # variable expansion # replace %NAME% with $settings{NAME} - my $new_settings = OpenQA::Utils::reset_settings(\%settings); + $expand_result = OpenQA::Settings::expand_placeholders(\%settings); - if (!$args->{MACHINE} || $args->{MACHINE} eq $new_settings->{MACHINE}) { + if (!$args->{MACHINE} || $args->{MACHINE} eq $settings{MACHINE}) { if (!@tests) { - $wanted{_settings_key($new_settings)} = 1; + $wanted{_settings_key(\%settings)} = 1; } else { foreach my $test (@tests) { - if ($test eq $new_settings->{TEST}) { - $wanted{_settings_key($new_settings)} = 1; + if ($test eq $settings{TEST}) { + $wanted{_settings_key(\%settings)} = 1; last; } } } } - push @$ret, $new_settings; + push @$ret, \%settings; } } @@ -602,7 +606,7 @@ sub _generate_jobs { splice @$ret, $i, 1; # not wanted - delete } } - return $ret; + return {expand_result => $expand_result, settings_result => $ret}; } =over 4 diff --git a/lib/OpenQA/Settings.pm b/lib/OpenQA/Settings.pm new file mode 100644 index 000000000000..864bf78041fe --- /dev/null +++ b/lib/OpenQA/Settings.pm @@ -0,0 +1,55 @@ +# 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 +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License + +package OpenQA::Settings; + +use strict; +use warnings; + +# replace %NAME% with $settings{NAME} +sub expand_placeholders { + my ($settings) = @_; + + my $error_string; + + for my $value (values %$settings) { + next unless defined $value; + + # loop the value until there is no %KEY% in it + while ($value =~ /%(\w+)%/) { + my $key = $1; + last unless defined $settings->{$key}; + + my $replaced_value = $settings->{$key}; + +# used to record already replaced value. If the value has been in this array which means there is a circular reference, then return an error message. + my @replaced_array = (); + + while ($replaced_value =~ /%(\w+)%/) { + last unless defined $settings->{$1}; + if (grep { $_ eq $1 } @replaced_array) { + return "The key $1 is a circular reference, value is " . $settings->{$1}; + } + else { + $replaced_value = $settings->{$1}; + push @replaced_array, $1; + } + } + $value =~ s/(%$key%)/$replaced_value/; + } + } + return undef; +} + +1; diff --git a/lib/OpenQA/Utils.pm b/lib/OpenQA/Utils.pm index e0a7517c20ee..8000ae37211b 100644 --- a/lib/OpenQA/Utils.pm +++ b/lib/OpenQA/Utils.pm @@ -1241,30 +1241,5 @@ sub any_array_item_contained_by_hash { return 0; } -#replace %NAME% with $settings{NAME} -sub reset_settings { - my ($settings) = @_; - - for my $value (values %$settings) { - if (defined $value) { - $value =~ s/(%\w+%)/_replace($settings, $1)/eig; - } - } - return $settings; -} - -#only used in reset_settings method -sub _replace { - my ($settings, $param) = @_; - - my $key = $param; - $key =~ s/%//g; - - if (defined $settings->{$key}) { - return $settings->{$key}; - } - return $param; -} - 1; # vim: set sw=4 et: diff --git a/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm b/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm index b6b1873889eb..53f29b05ae81 100644 --- a/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm +++ b/lib/OpenQA/WebAPI/Controller/API/V1/Job.pm @@ -16,11 +16,12 @@ package OpenQA::WebAPI::Controller::API::V1::Job; use Mojo::Base 'Mojolicious::Controller'; -use OpenQA::Utils; use OpenQA::Jobs::Constants; use OpenQA::Resource::Jobs; use OpenQA::Schema::Result::Jobs; use OpenQA::Events; +use OpenQA::Utils; +use OpenQA::Settings; use Try::Tiny; use DBIx::Class::Timestamps 'now'; use Mojo::Asset::Memory; @@ -235,10 +236,12 @@ sub create { my $json = {}; my $status; - my $new_params = $self->_generate_job_setting(\%params); + my $result = $self->_generate_job_setting(\%params); + return $self->render(json => {error => $result->{expand_result}}, status => 400) + if defined $result->{expand_result}; try { - my $job = $self->schema->resultset('Jobs')->create_from_settings($new_params); + 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; @@ -754,10 +757,10 @@ sub _generate_job_setting { my ($self, $args) = @_; my $schema = $self->schema; - my %settings; #include the machine, product, and test suit setting belongs to this job. - my @classes; #if the machine or product has worker_class setting, push them to this array. + my %settings; # Machines, product and test suite settings for the job + my @classes; # Populated with WORKER_CLASS settings from machines and products - #if args includes DISTRI, VERSION, FLAVOR, ARCH, get product setting. + # Populated with Product settins if there are DISTRI, VERSION, FLAVOR, ARCH in arguments. if ( defined $args->{DISTRI} && defined $args->{VERSION} && defined $args->{FLAVOR} @@ -781,7 +784,7 @@ sub _generate_job_setting { } } - #if args includes MACHINE, get machine setting. + # Populated with machine settings if there is MACHINE in arguments. if (defined $args->{MACHINE}) { my $machines = $schema->resultset('Machines')->search( { @@ -798,7 +801,7 @@ sub _generate_job_setting { } } - #TEST is mandatory, find the test suit settings. + # TEST is mandatory, so populate with TestSuit settings. my $test_suites = $schema->resultset('TestSuites')->search( { name => $args->{TEST}, @@ -821,7 +824,8 @@ sub _generate_job_setting { $settings{uc $_} = $args->{$_}; } - return OpenQA::Utils::reset_settings(\%settings); + my $expand_result = OpenQA::Settings::expand_placeholders(\%settings); + return {expand_result => $expand_result, settings_result => \%settings}; } 1; diff --git a/t/40-settings.t b/t/40-settings.t new file mode 100644 index 000000000000..614ab2cb9284 --- /dev/null +++ b/t/40-settings.t @@ -0,0 +1,120 @@ +#! /usr/bin/perl + +# 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 +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +use strict; +use warnings; + +BEGIN { + unshift @INC, 'lib'; +} + +use FindBin; +use lib "$FindBin::Bin/lib"; +use Test::More; +use OpenQA::Settings; + +my $settings = { + BUILD_SDK => '%BUILD_HA%', + BETA => 1, + ISO_MAXSIZE => '4700372992', + SHUTDOWN_NEEDS_AUTH => 1, + HDD_1 => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2', + PUBLISH_HDD_1 => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2', + ANOTHER_JOB => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2', + ARCH => 'x86_64', + BACKEND => 'qemu', + BUILD => '1234', + BUILD_SLE => '%BUILD%', + MACHINE => '64bit', + PATCH => 1, + UPGRADE => 1, + ISO => 'SLE-%VERSION%-%FLAVOR%-%MACHINE%-Build%BUILD%-Media1.iso', + WORKER_CLASS => 'qemu_x86_64', + VERSION => '15-SP1', + FLAVOR => 'Installer-DVD', + ADDONURL_SDK => 'ftp://openqa.suse.de/SLE-%VERSION%-SDK-POOL-%ARCH%-Build%BUILD_SDK%-Media1/', + DEPENDENCY_RESOLVER_FLAG => 1, + DESKTOP => 'textmode', + DEV_IMAGE => 1, + HDDSIZEGB => 50, + INSTALLONLY => 1, + PATTERNS => 'base,minimal', + PUBLISH_PFLASH_VARS => + 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed-uefi-vars.qcow2', + SEPARATE_HOME => 0, + BUILD_HA => '%BUILD%', + BUILD_SES => '%BUILD%', + WORKAROUND_MODULES => 'base,desktop,serverapp,script,sdk', +}; + +subtest expand_placeholders => sub { + my $str = OpenQA::Settings::expand_placeholders($settings); + my $match_settings = { + BUILD_SDK => '1234', + BETA => 1, + ISO_MAXSIZE => '4700372992', + SHUTDOWN_NEEDS_AUTH => 1, + HDD_1 => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed.qcow2', + PUBLISH_HDD_1 => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed.qcow2', + ANOTHER_JOB => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed.qcow2', + ARCH => 'x86_64', + BACKEND => 'qemu', + BUILD => '1234', + BUILD_SLE => '1234', + MACHINE => '64bit', + PATCH => 1, + UPGRADE => 1, + ISO => 'SLE-15-SP1-Installer-DVD-64bit-Build1234-Media1.iso', + WORKER_CLASS => 'qemu_x86_64', + VERSION => '15-SP1', + FLAVOR => 'Installer-DVD', + ADDONURL_SDK => 'ftp://openqa.suse.de/SLE-15-SP1-SDK-POOL-x86_64-Build1234-Media1/', + DEPENDENCY_RESOLVER_FLAG => 1, + DESKTOP => 'textmode', + DEV_IMAGE => 1, + HDDSIZEGB => 50, + INSTALLONLY => 1, + PATTERNS => 'base,minimal', + PUBLISH_PFLASH_VARS => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed-uefi-vars.qcow2', + SEPARATE_HOME => 0, + BUILD_HA => '1234', + BUILD_SES => '1234', + WORKAROUND_MODULES => 'base,desktop,serverapp,script,sdk', + }; + is_deeply($settings, $match_settings, "Settings replaced"); +}; + +subtest circular_reference => sub { + my $circular_settings = { + BUILD_SDK => '%BUILD_HA%', + ISO_MAXSIZE => '4700372992', + HDD_1 => 'SLES-%VERSION%-%ARCH%-%BUILD_HA%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2', + PUBLISH_HDD_1 => 'SLES-%VERSION%-%ARCH%-%BUILD_HA%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2', + ANOTHER_JOB => 'SLES-%VERSION%-%ARCH%-%BUILD_HA%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2', + ARCH => 'x86_64', + BACKEND => 'qemu', + BUILD => '%BUILD_HA%', + BUILD_HA => '%BUILD%', + VERSION => '15-SP1', + MACHINE => '64bit', + }; + my $str = OpenQA::Settings::expand_placeholders($circular_settings); + like($str, qr/The key (\w+) is a circular reference, value is %\w+%/, "circular reference exit successfully"); +}; + +done_testing; diff --git a/t/api/04-jobs.t b/t/api/04-jobs.t index 9c2d463bdf18..cf60fe67b9ac 100644 --- a/t/api/04-jobs.t +++ b/t/api/04-jobs.t @@ -702,12 +702,12 @@ subtest 'Expand specified Machine, Testsuit, Product variables' => sub { my $products = $t->app->schema->resultset('Products'); $products->create( { - version => 'Tumbleweed', + version => '15-SP1', name => '', - distri => 'opensuse', + distri => 'sle', arch => 'x86_64', description => '', - flavor => 'DVD', + flavor => 'Installer-DVD', settings => [ {key => 'BUILD_SDK', value => '%BUILD%'}, {key => 'BETA', value => '1'}, @@ -715,6 +715,18 @@ subtest 'Expand specified Machine, Testsuit, Product variables' => sub { {key => 'BUILD_HA', value => '%BUILD%'}, {key => 'BUILD_SES', value => '%BUILD%'}, {key => 'SHUTDOWN_NEEDS_AUTH', value => '1'}, + { + key => 'HDD_1', + value => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2' + }, + { + key => 'PUBLISH_HDD_1', + value => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2' + }, + { + key => 'ANOTHER_JOB', + value => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2' + }, ], }); my $testsuites = $t->app->schema->resultset('TestSuites'); @@ -731,30 +743,36 @@ subtest 'Expand specified Machine, Testsuit, Product variables' => sub { ], }); - delete $jobs_post_params{_GROUP_ID}; - delete $jobs_post_params{WORKER_CLASS}; - $jobs_post_params{_GROUP} = 'opensuse'; - $jobs_post_params{ARCH} = 'x86_64'; - $jobs_post_params{TEST} = 'autoupgrade'; + my %new_jobs_post_params = ( + iso => 'SLE-%VERSION%-%FLAVOR%-%MACHINE%-Build%BUILD%-Media1.iso', + DISTRI => 'sle', + VERSION => '15-SP1', + FLAVOR => 'Installer-DVD', + ARCH => 'x86_64', + TEST => 'autoupgrade', + MACHINE => '64bit', + BUILD => '1234', + _GROUP => 'opensuse', + ); - $post = $t->post_ok('/api/v1/jobs', form => \%jobs_post_params)->status_is(200); + $post = $t->post_ok('/api/v1/jobs', form => \%new_jobs_post_params)->status_is(200); my $result = $jobs->find($post->tx->res->json->{id})->settings_hash; delete $result->{NAME}; is_deeply( $result, { 'QEMUCPU' => 'qemu64', - 'VERSION' => 'Tumbleweed', - 'DISTRI' => 'opensuse', + 'VERSION' => '15-SP1', + 'DISTRI' => 'sle', 'MACHINE' => '64bit', - 'FLAVOR' => 'DVD', + 'FLAVOR' => 'Installer-DVD', 'ARCH' => 'x86_64', 'BUILD' => '1234', 'ISO_MAXSIZE' => '4700372992', 'INSTALLONLY' => 1, 'WORKER_CLASS' => 'qemu_x86_64', 'DESKTOP' => 'gnome', - 'ISO' => 'openSUSE-Tumbleweed-DVD-x86_64-Current.iso', + 'ISO' => 'SLE-15-SP1-Installer-DVD-64bit-Build1234-Media1.iso', 'BUILD_HA' => '1234', 'TEST' => 'autoupgrade', 'BETA' => 1, @@ -763,11 +781,72 @@ subtest 'Expand specified Machine, Testsuit, Product variables' => sub { 'SHUTDOWN_NEEDS_AUTH' => 1, 'PATCH' => 1, 'UPGRADE' => 1, + 'PUBLISH_HDD_1' => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed.qcow2', + 'ANOTHER_JOB' => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed.qcow2', + 'HDD_1' => 'SLES-15-SP1-x86_64-1234@64bit-minimal_with_sdk1234_installed.qcow2', }, 'Job post method expand specified MACHINE, PRODUCT, TESTSUIT variable', ); }; +subtest 'circular reference settings' => sub { + my $products = $t->app->schema->resultset('Products'); + $products->create( + { + version => '12-SP5', + name => '', + distri => 'sle', + arch => 'x86_64', + description => '', + flavor => 'Installer-DVD', + settings => [ + {key => 'BUILD_SDK', value => '%BUILD_HA%'}, + {key => 'BETA', value => '1'}, + {key => 'ISO_MAXSIZE', value => '4700372992'}, + {key => 'BUILD_HA', value => '%BUILD%'}, + {key => 'BUILD_SES', value => '%BUILD%'}, + {key => 'SHUTDOWN_NEEDS_AUTH', value => '1'}, + { + key => 'PUBLISH_HDD_1', + value => 'SLES-%VERSION%-%ARCH%-%BUILD%@%MACHINE%-minimal_with_sdk%BUILD_SDK%_installed.qcow2' + }, + ], + }); + my $testsuites = $t->app->schema->resultset('TestSuites'); + $testsuites->create( + { + name => 'circular', + description => '', + settings => [ + {key => 'DESKTOP', value => 'gnome'}, + {key => 'INSTALLONLY', value => '1'}, + {key => 'MACHINE', value => '64bit'}, + {key => 'PATCH', value => '1'}, + {key => 'UPGRADE', value => '1'}, + ], + }); + + my %new_jobs_post_params = ( + iso => 'SLE-%VERSION%-%FLAVOR%-%MACHINE%-Build%BUILD%-Media1.iso', + DISTRI => 'sle', + VERSION => '12-SP5', + FLAVOR => 'Installer-DVD', + ARCH => 'x86_64', + TEST => 'circular', + MACHINE => '64bit', + BUILD => '%BUILD_HA%', + _GROUP => 'opensuse', + ); + + $t->post_ok('/api/v1/jobs', form => \%new_jobs_post_params)->status_is(400); + like( + $t->tx->res->json->{error}, + qr/The key (\w+) is a circular reference, value is %\w+%/, + 'circular reference exit successfully' + ); +}; + + subtest 'error on insufficient params' => sub { $t->post_ok('/api/v1/jobs', form => {})->status_is(400); };