Expand Machine and Medium Type settings when creating job with 'jobs post' #2083

Amrysliu · 2019-05-28T07:42:15Z

When users created jobs using 'client jobs post', the jobs settings did
not expand Machine and Medium Type settings. This PR is used to support
to expand settings from Machine and Medium Type when creating jobs with
'jobs post'.

See: https://progress.opensuse.org/issues/20464
Related: https://progress.opensuse.org/issues/51479

Martchus · 2019-05-28T09:00:08Z

You need to find out whether the failing CI test is related:

./t/api/11-bugs.t                       (Wstat: 256 Tests: 31 Failed: 1)
  Failed test:  28
  Non-zero exit status: 1

codecov · 2019-05-28T10:05:55Z

Codecov Report

Merging #2083 into master will increase coverage by 0.03%.
The diff coverage is 94.91%.

@@            Coverage Diff             @@
##           master    #2083      +/-   ##
==========================================
+ Coverage   85.04%   85.07%   +0.03%     
==========================================
  Files         159      160       +1     
  Lines       10596    10638      +42     
==========================================
+ Hits         9011     9050      +39     
- Misses       1585     1588       +3

Impacted Files	Coverage Δ
lib/OpenQA/ExpandPlaceholder.pm	`100% <100%> (ø)`
lib/OpenQA/Schema/Result/ScheduledProducts.pm	`97.15% <100%> (-0.09%)`	⬇️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm	`86.64% <90.9%> (+0.43%)`	⬆️
lib/OpenQA/Worker/WebUIConnection.pm	`77.48% <0%> (-2.62%)`	⬇️
lib/OpenQA/Worker.pm	`82.33% <0%> (-0.71%)`	⬇️
lib/OpenQA/Worker/Settings.pm	`97.56% <0%> (+17.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e00d396...9cb48b8. Read the comment docs.

lib/OpenQA/Settings.pm

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm

lib/OpenQA/Settings.pm

t/40-settings.t

lib/OpenQA/Settings.pm

Martchus

And since you've asked for general improvements of the algorithm: I also don't know a better way to detect cycles other then remembering visited elements. I'd only use a hash to store and check for them.

lib/OpenQA/Settings.pm

okurz

please see inline comments. Also, I think we should extend the documentation a bit? I can offer my help but so far unfortunately I am not yet sure what exactly the new behaviour is supposed to support

lib/OpenQA/Utils.pm

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm

okurz · 2019-07-02T05:33:51Z

t/api/04-jobs.t

@@ -693,6 +693,76 @@ subtest 'TEST is only mandatory parameter' => sub {
    $t->json_is('/job/settings/DISTRI'  => undef);
 };

+subtest 'Expand specified Machine, Testsuit, Product variables' => sub {


typo "Testsuit"

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm

t/40-settings.t

Amrysliu · 2019-07-02T10:40:03Z

@Martchus @kraih @okurz Please help me review this PR, if you have time. Thanks.

okurz

see unresolved inline comments

perlpunk · 2019-07-03T10:31:21Z

lib/OpenQA/Settings.pm

+                my $key_in_value = $1;
+                last unless defined $settings->{$key_in_value};
+
+                return "The key $key_in_value contains a circular reference, its value is " . $settings->{$key_in_value}


I'm a fan of exceptions.
Returning a string and then relying on the calling code is usually less maintainable.
In lib/OpenQA/WebAPI/Controller/API/V1/Job.pm for example there is already a try/catch block. So I would just die instead of returning a string.

perlpunk · 2019-07-03T10:32:25Z

lib/OpenQA/Settings.pm

+sub expand_placeholders {
+    my ($settings) = @_;
+
+    my $error_string;


I can't see that this variable is used anywhere.

Thanks, I will remove it.

perlpunk · 2019-07-03T10:36:17Z

lib/OpenQA/Settings.pm

+use warnings;
+
+# replace %NAME% with $settings{NAME}
+sub expand_placeholders {


May I suggest an alternative to the expand_placeholders subroutine?

sub expand_placeholders { my ($settings) = @_; for my $value (sort values %$settings) { next unless defined $value; my %seen; $value =~ s/%(\w+)%/expand_placeholder($settings, $1, \%seen)/ge; } return 1; } sub expand_placeholder { my ($settings, $key, $global_seen) = @_; unless (exists $settings->{ $key }) { return ''; } my %seen = %$global_seen; # prevent circular replacement if ($seen{ $key }++) { die sprintf "The key %s contains a circular reference, its value is %s", $key, $settings->{ $key }; } my $value = $settings->{ $key }; $value =~ s/%(\w+)%/expand_placeholder($settings, $1, \%seen)/ge; return $value; }

I prefer throwing exceptions too. But i don't think we have an exception class in openQA yet to filter which exceptions to show the user.

@perlpunk If I may give a suggestion, start your github code section comments with the word "perl" immediately after the three backticks as I just did with your comment for cool syntax highlighting :)

@okurz done, thanks :)
edit: oh, somehow I didn't see that you already updated it. github didn't warn me...

@perlpunk Thanks for your suggestion. It looks better. I would like to ask your opinion @perlpunk @kraih @okurz Do I need to change return to die. If I do not need to do that, I prefer to keep current method expand_placeholders.

okurz · 2019-07-05T07:24:21Z

If you don't mind in your PR description and also git commit could you please describe what the important change or benefit is? Also there is a typo "testsuit" in one commit message, should be "test suite" instead

Amrysliu · 2019-07-05T10:57:12Z

@Martchus @kraih @okurz @perlpunk Please help me review this PR if you have time. Thanks.
There are some major changes in this commit:

Change to Settings.pm to ExpandPlaceholder.pm. Because Settings.pm is used in @Martchus branch.
Change the replacing value algorithm from while loop to a recursive function.
Fix typo.

@okurz I modify the commit message. Wrote the related tickets in it. I am wondering if it is enough to describe this PR's aim.

Martchus · 2019-07-05T15:39:55Z

Just for the record: The change 1. is not required. Those are different Settings.pm files.

okurz · 2019-07-06T09:38:05Z

Yes, the reference to the ticket is fine. But please rewrite the subject line of the PR and the commit message to point out what the important change of behaviour is. For example the openSUSE package's changelog is generated from the commit messages. The changelog should in an obvious way describe what is important for the user. "Rewrite code …" is what we do with every commit in general and not very exciting to read for users ;)

lib/OpenQA/ExpandPlaceholder.pm

lib/OpenQA/Schema/Result/ScheduledProducts.pm

t/api/04-jobs.t

t/40-expand_placeholders.t

lib/OpenQA/ExpandPlaceholder.pm

Amrysliu · 2019-07-12T10:32:10Z

@Martchus Thanks for your comments. I already modified the code. If you have time please have a review. Thanks

Martchus

Sorry for nitpicking on variable name but it really helps to understand the code better (especially in a language without static types).

lib/OpenQA/Schema/Result/ScheduledProducts.pm

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm

Amrysliu · 2019-07-15T06:02:55Z

Sorry for nitpicking on variable name but it really helps to understand the code better (especially in a language without static types).

Please feel free to give me suggestions. All names you suggested are more meaningful and I am a pleasure to modify them.

Martchus

There are outstanding changes for the other reviews but the issues I mentioned have been addressed.

post' When users created jobs using 'client jobs post', the jobs settings did not expand Machine and Medium Type settings. This PR is used to support to expand settings from Machine and Medium Type when creating jobs with 'jobs post'. See: https://progress.opensuse.org/issues/20464 Related: https://progress.opensuse.org/issues/51479

perlpunk · 2019-07-16T10:16:41Z

t/40-expand_placeholders.t

+
+BEGIN {
+    unshift @INC, 'lib';
+}


I know this BEGIN block is used in many other tests.
Just a note that this was the way to do it in older perls, but the modern way is to use

use FindBin; use lib "$FindBin::Bin/../lib";

okurz · 2019-07-16T08:53:30Z

lib/OpenQA/ExpandPlaceholder.pm

+        eval { $value =~ s/%(\w+)%/_expand_placeholder($settings, $1, \%visited_top_level_placeholders)/eg; };
+        if ($@) {
+            return "Error: $@";
+        }


@perlpunk I think you suggested the simple loop but without the eval/if, can you comment on this? I don't see why a call to an internal method should be wrapped by eval

I think we had a discussion about exceptions in the chat.
Using exceptions consequently and catching them at the upmost level possible would be preferable, but we decided that we make this improvement later.
Still, I think _expand_placeholder should die instead of returning a string, because handling this would be more work here, so we need that eval.

Last weekend i made a new exception handling mechanism for Mojolicious. That might come in handy for this stuff in the future.

Yes, in the future :) we should not yet rely on this new version

@kraih @okurz So do I need to change the code to return a string?

Not sure myself now. @kraih @perlpunk should @Amrysliu change something here or keep as is?

IMHO keep it as it is

All errors are presented to the user, right? I'm always very sceptical of doing that, even in cases like this were there is not much risk of leaking information the user shouldn't have. Maybe just to be sure make the die statements throw arrayrefs and rethrow non-arrayref exceptions? Maybe that's the pattern we should encourage until we have real exception objects.

eval { ... }; if (my $err = $@) { if ($err eq 'ARRAY') { return "Error: @$err" } else { die $err } } sub _expand_placeholder { ... die ["The key $key contains a circular reference, its value is $settings->{$key}."]; ... }

Think this would be setting a good example.

kraih

The exception handling could still be better, but i don't think it's required at this time.

commit 7a26251 Merge: 6a8f9a5 9cb48b8 Author: LiuXiaojing <xiaojing.liu@suse.com> AuthorDate: Tue Jul 23 16:46:19 2019 +0800 Commit: GitHub <noreply@github.com> CommitDate: Tue Jul 23 16:46:19 2019 +0800 Merge pull request #2083 from Amrysliu/redone_job_setting Expand Machine and Medium Type settings when creating job with 'jobs post'

Amrysliu closed this May 28, 2019

Amrysliu reopened this May 28, 2019

os-autoinst deleted a comment from codecov bot May 28, 2019

Martchus requested changes May 28, 2019

View reviewed changes

Amrysliu force-pushed the redone_job_setting branch 2 times, most recently from 01cdbbb to 075a220 Compare May 30, 2019 02:56

Martchus approved these changes Jun 3, 2019

View reviewed changes

kraih requested changes Jun 6, 2019

View reviewed changes

lib/OpenQA/Settings.pm Outdated Show resolved Hide resolved

Amrysliu force-pushed the redone_job_setting branch 2 times, most recently from 8666a6b to 41784b3 Compare June 11, 2019 01:56

Martchus requested changes Jun 11, 2019

View reviewed changes

lib/OpenQA/Settings.pm Outdated Show resolved Hide resolved

lib/OpenQA/Settings.pm Outdated Show resolved Hide resolved

Amrysliu force-pushed the redone_job_setting branch from 41784b3 to a45f17b Compare June 14, 2019 03:09

okurz reviewed Jul 2, 2019

View reviewed changes

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm Outdated Show resolved Hide resolved

t/40-settings.t Outdated Show resolved Hide resolved

Amrysliu force-pushed the redone_job_setting branch from a45f17b to 6f9b838 Compare July 2, 2019 10:37

okurz requested changes Jul 2, 2019

View reviewed changes

perlpunk reviewed Jul 3, 2019

View reviewed changes

Amrysliu force-pushed the redone_job_setting branch 2 times, most recently from c0f2ccd to f2ac486 Compare July 5, 2019 10:55

Amrysliu force-pushed the redone_job_setting branch from f2ac486 to 030ce69 Compare July 8, 2019 03:08

Amrysliu changed the title ~~Rewrite code for job settings expansion~~ Expand Machine and Medium Type settings when creating job with 'jobs post' Jul 8, 2019

Amrysliu requested review from Martchus, kraih and okurz July 11, 2019 06:44

Martchus requested changes Jul 11, 2019

View reviewed changes

Martchus reviewed Jul 11, 2019

View reviewed changes

lib/OpenQA/ExpandPlaceholder.pm Outdated Show resolved Hide resolved

Martchus reviewed Jul 11, 2019

View reviewed changes

lib/OpenQA/ExpandPlaceholder.pm Outdated Show resolved Hide resolved

Martchus reviewed Jul 11, 2019

View reviewed changes

lib/OpenQA/ExpandPlaceholder.pm Outdated Show resolved Hide resolved

Amrysliu force-pushed the redone_job_setting branch 2 times, most recently from 41a3e90 to 5289954 Compare July 12, 2019 10:22

Martchus requested changes Jul 12, 2019

View reviewed changes

lib/OpenQA/Schema/Result/ScheduledProducts.pm Show resolved Hide resolved

lib/OpenQA/WebAPI/Controller/API/V1/Job.pm Outdated Show resolved Hide resolved

Amrysliu force-pushed the redone_job_setting branch from 5289954 to 026fa0f Compare July 15, 2019 05:46

Martchus approved these changes Jul 15, 2019

View reviewed changes

Amrysliu force-pushed the redone_job_setting branch from 026fa0f to 9cb48b8 Compare July 16, 2019 01:50

perlpunk approved these changes Jul 16, 2019

View reviewed changes

okurz reviewed Jul 16, 2019

View reviewed changes

okurz approved these changes Jul 19, 2019

View reviewed changes

kraih approved these changes Jul 19, 2019

View reviewed changes

Amrysliu merged commit 7a26251 into os-autoinst:master Jul 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand Machine and Medium Type settings when creating job with 'jobs post' #2083

Expand Machine and Medium Type settings when creating job with 'jobs post' #2083

Amrysliu commented May 28, 2019 •

edited

Loading

Martchus commented May 28, 2019

codecov bot commented May 28, 2019 •

edited

Loading

Martchus left a comment

okurz left a comment

okurz Jul 2, 2019

Amrysliu commented Jul 2, 2019

okurz left a comment

perlpunk Jul 3, 2019

perlpunk Jul 3, 2019

Amrysliu Jul 4, 2019

perlpunk Jul 3, 2019 •

edited by okurz

Loading

kraih Jul 3, 2019

okurz Jul 3, 2019

perlpunk Jul 3, 2019 •

edited

Loading

Amrysliu Jul 4, 2019

okurz commented Jul 5, 2019

Amrysliu commented Jul 5, 2019 •

edited

Loading

Martchus commented Jul 5, 2019

okurz commented Jul 6, 2019

Amrysliu commented Jul 12, 2019

Martchus left a comment

Amrysliu commented Jul 15, 2019

Martchus left a comment •

edited

Loading

perlpunk Jul 16, 2019

okurz Jul 16, 2019

perlpunk Jul 16, 2019 •

edited

Loading

kraih Jul 16, 2019

okurz Jul 16, 2019

Amrysliu Jul 17, 2019

okurz Jul 17, 2019

perlpunk Jul 17, 2019

kraih Jul 19, 2019 •

edited

Loading

kraih left a comment

Expand Machine and Medium Type settings when creating job with 'jobs post' #2083

Expand Machine and Medium Type settings when creating job with 'jobs post' #2083

Conversation

Amrysliu commented May 28, 2019 • edited Loading

Martchus commented May 28, 2019

codecov bot commented May 28, 2019 • edited Loading

Codecov Report

Martchus left a comment

Choose a reason for hiding this comment

okurz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amrysliu commented Jul 2, 2019

okurz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlpunk Jul 3, 2019 • edited by okurz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlpunk Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okurz commented Jul 5, 2019

Amrysliu commented Jul 5, 2019 • edited Loading

Martchus commented Jul 5, 2019

okurz commented Jul 6, 2019

Amrysliu commented Jul 12, 2019

Martchus left a comment

Choose a reason for hiding this comment

Amrysliu commented Jul 15, 2019

Martchus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perlpunk Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraih Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

kraih left a comment

Choose a reason for hiding this comment

Amrysliu commented May 28, 2019 •

edited

Loading

codecov bot commented May 28, 2019 •

edited

Loading

perlpunk Jul 3, 2019 •

edited by okurz

Loading

perlpunk Jul 3, 2019 •

edited

Loading

Amrysliu commented Jul 5, 2019 •

edited

Loading

Martchus left a comment •

edited

Loading

perlpunk Jul 16, 2019 •

edited

Loading

kraih Jul 19, 2019 •

edited

Loading