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.

Sign up for GitHub

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

Jump to bottom

Fix typo for default of sig #9495

Merged
merged 1 commit into from
Feb 11, 2025
Merged

Fix typo for default of sig #9495

merged 1 commit into from
Feb 11, 2025

Conversation

daveisfera
Copy link
Contributor

Fix a typo so the default of sig will match the uses of the function

@codecov Codecov
Copy link

codecov bot commented Jan 15, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.24%. Comparing base (57ab2a6) to head (fc8d4be).
Report is 9 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #9495   +/-   ##
=======================================
  Coverage   78.24%   78.24%           
=======================================
  Files         153      153           
  Lines       19040    19040           
  Branches     2520     2520           
=======================================
  Hits        14898    14898           
+ Misses       3856     3854    -2     
- Partials      286      288    +2
Flag Coverage Δ
unittests 78.22% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@auvipy auvipy requested review from Nusnus and auvipy January 15, 2025 07:10
Nusnus
Nusnus reviewed Feb 9, 2025
View reviewed changes
Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

Was there an issue / bug that needed this change?

@Nusnus
Copy link
Member

Nusnus commented Feb 9, 2025

Was there an issue / bug that needed this change?

Is this related?
#9505

@daveisfera
Copy link
Contributor Author

Was there an issue / bug that needed this change?

Is this related? #9505

I noticed it while looking into that issue, but it wasn't impacting anything I noticed because all existing callers I could see specify that value, so it was just a fix for completeness.

auvipy
auvipy requested changes Feb 10, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

some verification test possible?

@daveisfera
Copy link
Contributor Author

daveisfera commented Feb 11, 2025 •
edited
Loading

some verification test possible?

All of the callers of this function are in this same file and you can see that they all have SIGXXXX for the value passed to sig so the default of TERM is incorrect and should either be changed to SIGTERM or removed

celery/celery/apps/worker.py

Line 422 in fc8d4be

_shutdown_handler, sig='SIGTERM', how='Cold', callback=on_cold_shutdown, exitcode=EX_FAILURE,

@auvipy auvipy added this to the 5.5.0 milestone Feb 11, 2025
@auvipy auvipy merged commit cc9e96d into celery:main Feb 11, 2025
70 checks passed
@Nusnus
Copy link
Member

Nusnus commented Feb 11, 2025

Thank you for the fix!

@auvipy auvipy added the Worker label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nusnus Nusnus Nusnus left review comments

@auvipy auvipy auvipy approved these changes

Assignees
No one assigned
Labels
PR Type: Bugfix Worker
Projects
None yet
Milestone
5.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@daveisfera @Nusnus @auvipy