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

PART 2: WebAPI::Controller::Running: Remove uncovered close_connection handler - After #5755 #4898

Closed
wants to merge 1 commit into from

Conversation

okurz
Copy link
Member

@okurz okurz commented Nov 15, 2022

my @st = stat $logfile;
return $close_connection->() unless @st && $st[1] == $ino && $st[3] > 0 && $st[7] >= $size;
die "Zero tolerance for any shenanigans with the logfile, such as truncation, rotation, etc."
Copy link
Contributor

@Martchus Martchus Nov 15, 2022

Choose a reason for hiding this comment

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

We should likely close the connection here as the code previously did. Otherwise we'd leave the client waiting for further data until a timeout occurs. Placing a die here seems generally wrong as it would likely just be caught by Mojolicious' handler. The message has also not enough context to be useful in the server-side logs (where it would end up). If we wanted better error handling, then an error should likely go to the client in form of a bad return code.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I agree. All CI tests passing show that we are missing test coverage here

@okurz okurz marked this pull request as draft November 15, 2022 12:35
@okurz okurz changed the title WebAPI::Controller::Running: Remove uncovered close_connection handler PART 2: WebAPI::Controller::Running: Remove uncovered close_connection handler - After #4961 Dec 19, 2022
@stale stale bot added the stale label Mar 25, 2023
@os-autoinst os-autoinst deleted a comment from stale bot Jul 11, 2024
@stale stale bot removed the stale label Jul 11, 2024
@os-autoinst os-autoinst deleted a comment from mergify bot Jul 11, 2024
@okurz okurz changed the title PART 2: WebAPI::Controller::Running: Remove uncovered close_connection handler - After #4961 WebAPI::Controller::Running: Remove uncovered close_connection handler Jul 11, 2024
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 98.43%. Comparing base (63bc4d0) to head (f838979).
Report is 49 commits behind head on master.

Files Patch % Lines
lib/OpenQA/Shared/Controller/Running.pm 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4898      +/-   ##
==========================================
- Coverage   98.43%   98.43%   -0.01%     
==========================================
  Files         393      393              
  Lines       38594    38593       -1     
==========================================
- Hits        37991    37990       -1     
  Misses        603      603              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@okurz okurz changed the title WebAPI::Controller::Running: Remove uncovered close_connection handler PART 2: WebAPI::Controller::Running: Remove uncovered close_connection handler - After #5755 Jul 11, 2024
@os-autoinst os-autoinst deleted a comment from mergify bot Jul 23, 2024
@okurz okurz closed this Jul 23, 2024
@okurz okurz deleted the enhance/signatures8 branch July 23, 2024 11:22
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.

2 participants