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

Force tasks with no files as failed #92

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Force tasks with no files as failed #92

merged 2 commits into from
Nov 11, 2024

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Nov 8, 2024

Fix #90

@benoit74 benoit74 self-assigned this Nov 8, 2024
@benoit74 benoit74 requested a review from rgaudin November 8, 2024 14:53
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

Attention: Patch coverage is 61.11111% with 7 lines in your changes missing coverage. Please review.

Project coverage is 46.11%. Comparing base (6eaea92) to head (0941055).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
api/src/zimitfrontend/routes/hook.py 0.00% 5 Missing ⚠️
api/src/zimitfrontend/routes/utils.py 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
- Coverage   46.35%   46.11%   -0.25%     
==========================================
  Files          10       10              
  Lines         384      386       +2     
  Branches       43       44       +1     
==========================================
  Hits          178      178              
- Misses        205      206       +1     
- Partials        1        2       +1     

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

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Haven't checked the template to find out what happens exactly but this with current line 133 is a mistake

return MailToSend(status=SUCCESS, target=target, subject=subject, body=body)

@benoit74
Copy link
Collaborator Author

benoit74 commented Nov 8, 2024

I don't get why? Because status=SUCCESS? This status is the hook return status, and this should be SUCCESS (and we should rename it for clarity ^^)

@rgaudin
Copy link
Member

rgaudin commented Nov 9, 2024

Please do so in this PR. This function used to return only on failures and only reach this point when everything's fine. Now that we get to this point with a failed task, we need to be more careful. We don't want to have to look at the templates to find this out

@benoit74
Copy link
Collaborator Author

This function used to return only on failures and only reach this point when everything's fine.

Nope, this function already reached this point when task status was among "requested", "succeeded", "failed" or "canceled". See line

if task.status not in ("requested", "succeeded", "failed", "canceled"):

But anyway, will fix this in this PR, better now than never

@benoit74 benoit74 requested a review from rgaudin November 11, 2024 09:26
@benoit74 benoit74 merged commit c1241e2 into main Nov 11, 2024
7 checks passed
@benoit74 benoit74 deleted the force_fail_status branch November 11, 2024 10:49
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.

Download email missing download links
2 participants