Validate parameters passed to job group API routes #2812

kalikiana · 2020-03-03T22:18:07Z

See also #2781 for validation in JobTemplate.

See: poo#64075

okurz

nice :)

lib/OpenQA/WebAPI/Plugin/Helpers.pm

t/api/10-jobgroups.t

okurz · 2020-03-10T08:33:58Z

+1

UI test failed with:

[08:25:33] t/ui/13-admin.t ............................ 11/? 
        #   Failed test 'size edited'
        #   at t/ui/13-admin.t line 471.
        #          got: '1000'
        #     expected: ''
        # Looks like you failed 1 test of 10.

    #   Failed test 'edit some properties'
    #   at t/ui/13-admin.t line 472.
    # Looks like you failed 1 test of 4.
[08:25:33] t/ui/13-admin.t ............................ 12/? 
#   Failed test 'job property editor'
#   at t/ui/13-admin.t line 473.
    # products: {}\nscenarios: {} # additional comment\n
[08:25:33] t/ui/13-admin.t ............................ 15/? # Looks like you failed 1 test of 15.
[08:25:33] t/ui/13-admin.t ............................ Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/15 subtests

kalikiana · 2020-03-16T08:56:15Z

+1

UI test failed with:

[08:25:33] t/ui/13-admin.t ............................ 11/? 
        #   Failed test 'size edited'
        #   at t/ui/13-admin.t line 471.
        #          got: '1000'
        #     expected: ''
        # Looks like you failed 1 test of 10.

    #   Failed test 'edit some properties'
    #   at t/ui/13-admin.t line 472.
    # Looks like you failed 1 test of 4.
[08:25:33] t/ui/13-admin.t ............................ 12/? 
#   Failed test 'job property editor'
#   at t/ui/13-admin.t line 473.
    # products: {}\nscenarios: {} # additional comment\n
[08:25:33] t/ui/13-admin.t ............................ 15/? # Looks like you failed 1 test of 15.
[08:25:33] t/ui/13-admin.t ............................ Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/15 subtests

After spending considerable time debugging this, my finding is this: $self-validation->param consistently returns no value if the value is indeed '' as can be the case with size_limit_gb regardless of errors or even without any regex defined. So I changed it back to $self->param and added a XXX comment.

kraih · 2020-03-16T11:58:12Z

I've proposed an upstream change that might help with this. mojolicious/mojo#1482

Martchus

I guess it is best if @kraih give a review because he knows best how the validator API is supposed to be used.

t/api/10-jobgroups.t

codecov · 2020-03-16T15:48:42Z

Codecov Report

Merging #2812 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2812      +/-   ##
==========================================
+ Coverage   93.27%   93.29%   +0.02%     
==========================================
  Files         189      189              
  Lines       11917    11912       -5     
==========================================
- Hits        11115    11113       -2     
+ Misses        802      799       -3

Impacted Files	Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/JobGroup.pm	`98.27% <100%> (+0.25%)`	⬆️
lib/OpenQA/Schema/Result/JobModules.pm	`93.9% <0%> (-0.36%)`	⬇️
lib/OpenQA/Schema/Result/Jobs.pm	`97.03% <0%> (-0.02%)`	⬇️
lib/OpenQA/WebAPI/Plugin/ObsRsync.pm	`99.17% <0%> (-0.01%)`	⬇️
lib/OpenQA/Worker/Job.pm	`74.59% <0%> (+0.11%)`	⬆️

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 b26b51b...4495ec4. Read the comment docs.

t/api/10-jobgroups.t

Martchus

Let's see whether the tests pass.

Martchus · 2020-03-31T11:13:32Z

t/api/10-jobgroups.t

+        {keep_important_logs_in_days    => '4 days'},
+        {keep_results_in_days           => '30 days'},
+        {keep_important_results_in_days => '1 days'},
+        {default_priority               => 'inherit'},


It would actually make sense to allow this so one can revert to the inherited value. But it looks like that is really not supported so far so I'm fine considering this invalid at this point.

Yes, and since we support that with job templates one could be forgiven for trying to use that. So I picked the invalid value here as well to highlight that it is not supported.

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

t/api/10-jobgroups.t

See also os-autoinst#2781 for validation in JobTemplate. See: https://progress.opensuse.org/issues/64075

kalikiana self-assigned this Mar 3, 2020

okurz reviewed Mar 4, 2020

View reviewed changes

lib/OpenQA/WebAPI/Plugin/Helpers.pm Outdated Show resolved Hide resolved

t/api/10-jobgroups.t Outdated Show resolved Hide resolved

t/api/10-jobgroups.t Outdated Show resolved Hide resolved

kalikiana force-pushed the job_groups_validation_docs branch from 9737218 to d3c73ef Compare March 9, 2020 08:09

kalikiana force-pushed the job_groups_validation_docs branch from d3c73ef to 6b7276e Compare March 16, 2020 08:53

kalikiana marked this pull request as ready for review March 16, 2020 08:56

kalikiana mentioned this pull request Mar 16, 2020

Change validation to consider the empty string a valid value mojolicious/mojo#1482

Merged

Martchus reviewed Mar 16, 2020

View reviewed changes

t/api/10-jobgroups.t Outdated Show resolved Hide resolved

kalikiana force-pushed the job_groups_validation_docs branch from 6b7276e to 2ef134a Compare March 16, 2020 14:33

kalikiana force-pushed the job_groups_validation_docs branch from 2ef134a to 7b31212 Compare March 30, 2020 13:57

kalikiana changed the title ~~Validation and docs for JobGroup create and update~~ Validate parameters passed to job group API routes Mar 30, 2020

kalikiana force-pushed the job_groups_validation_docs branch from 7b31212 to bae48ac Compare March 30, 2020 16:57

okurz reviewed Mar 31, 2020

View reviewed changes

t/api/10-jobgroups.t Outdated Show resolved Hide resolved

kalikiana force-pushed the job_groups_validation_docs branch from bae48ac to 4d7c8ec Compare March 31, 2020 10:07

Martchus approved these changes Mar 31, 2020

View reviewed changes

kraih requested changes Mar 31, 2020

View reviewed changes

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

t/api/10-jobgroups.t Outdated Show resolved Hide resolved

okurz approved these changes Mar 31, 2020

View reviewed changes

Validate parameters passed to job group API routes

4495ec4

See also os-autoinst#2781 for validation in JobTemplate. See: https://progress.opensuse.org/issues/64075

kalikiana force-pushed the job_groups_validation_docs branch from 4d7c8ec to 4495ec4 Compare March 31, 2020 13:16

kraih approved these changes Mar 31, 2020

View reviewed changes

Martchus merged commit 21e083f into os-autoinst:master Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate parameters passed to job group API routes #2812

Validate parameters passed to job group API routes #2812

kalikiana commented Mar 3, 2020

okurz left a comment

okurz commented Mar 10, 2020

kalikiana commented Mar 16, 2020

kraih commented Mar 16, 2020

Martchus left a comment

codecov bot commented Mar 16, 2020 •

edited

Loading

Martchus left a comment

Martchus Mar 31, 2020 •

edited

Loading

kalikiana Mar 31, 2020

Validate parameters passed to job group API routes #2812

Validate parameters passed to job group API routes #2812

Conversation

kalikiana commented Mar 3, 2020

okurz left a comment

Choose a reason for hiding this comment

okurz commented Mar 10, 2020

kalikiana commented Mar 16, 2020

kraih commented Mar 16, 2020

Martchus left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2020 • edited Loading

Codecov Report

Martchus left a comment

Choose a reason for hiding this comment

Martchus Mar 31, 2020 • edited Loading

Choose a reason for hiding this comment

kalikiana Mar 31, 2020

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2020 •

edited

Loading

Martchus Mar 31, 2020 •

edited

Loading