Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid logging user-input related errors in YAML to server log #4520

Merged
merged 8 commits into from
Feb 24, 2022

Conversation

Martchus
Copy link
Contributor

  • Distinguish between errors caused by user-input (1) and unhandled
    errors (2)
  • Report only (1) to the user and log only (2) to the server log
  • Treat constraint violations as user-input related as they are most likely
    caused by the input as well
  • See https://progress.opensuse.org/issues/106880

@perlpunk
Copy link
Contributor

The CI problems in the fullstack / unstable tests are likely caused by https://progress.opensuse.org/issues/107002 , I commented there

@okurz
Copy link
Member

okurz commented Feb 22, 2022

Typo in commit message s/unnecassary/unnecessary/

@Martchus
Copy link
Contributor Author

@perlpunk Indeed. At least now we have the video, see https://progress.opensuse.org/issues/106912.

@Martchus Martchus marked this pull request as ready for review February 22, 2022 14:04
@kalikiana
Copy link
Member

So this is failing in unstable:

Retry 1 of 5 …
[14:10:29] t/05-scheduler-full.t .. 3/? 
    #   Failed test 'Allocated maximum number of jobs that could have been allocated'
    #   at t/05-scheduler-full.t line 225.
    #          got: '9'
    #     expected: '10'
    # Looks like you failed 1 test of 1.
[14:10:29] t/05-scheduler-full.t .. 4/? 

And subsequent runs fail due to issues with retries, see https://progress.opensuse.org/issues/107002 but I think this PR doesn't have to block on that.

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I don't see a test case where an unexpected error happens and is logged, (but we also didn't have that before). Do you think it makes sense to add one?
Other than that all good.

@okurz
Copy link
Member

okurz commented Feb 23, 2022

@Mergifyio rebase

@perlpunk
Copy link
Contributor

You could rebase now as the unrelated problems with the unstable tests should be fixed.

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2022

rebase

✅ Branch has been successfully rebased

@okurz
Copy link
Member

okurz commented Feb 23, 2022

@perlpunk I just asked mergify to do that. It's just weird that it looks like "kraih" would have rebased.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #4520 (b788044) into master (c4eb0b8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4520   +/-   ##
=======================================
  Coverage   97.97%   97.97%           
=======================================
  Files         374      374           
  Lines       34109    34144   +35     
=======================================
+ Hits        33417    33453   +36     
+ Misses        692      691    -1     
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/Table.pm 97.43% <ø> (-0.03%) ⬇️
lib/OpenQA/WebAPI/Plugin/YAML.pm 100.00% <ø> (ø)
lib/OpenQA/Schema/Result/JobGroups.pm 99.45% <100.00%> (+<0.01%) ⬆️
lib/OpenQA/Schema/ResultSet/JobTemplates.pm 100.00% <100.00%> (ø)
lib/OpenQA/WebAPI/Controller/API/V1/JobTemplate.pm 93.88% <100.00%> (+0.59%) ⬆️
t/api/08-jobtemplates.t 98.34% <100.00%> (+0.16%) ⬆️

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 c4eb0b8...b788044. Read the comment docs.

okurz
okurz previously requested changes Feb 23, 2022
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov/patch failed. As this PR is non-critical I recommend to increase patch coverage as part of this work here.

@Martchus
Copy link
Contributor Author

Ok, so I need to cover:

        # Push the exception to the list of errors without the trailing new line
        push @$user_errors, substr($_, 0, -1);

and

    if (@server_errors) {
        push @$user_errors, 'Internal server error occurred';
        $self->app->log->error(@server_errors);

@Martchus Martchus dismissed okurz’s stale review February 24, 2022 14:08

The test passes now as coverage has been added.

@mergify mergify bot merged commit 7601114 into os-autoinst:master Feb 24, 2022
@Martchus Martchus deleted the yaml-errors branch February 24, 2022 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants