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

exit celery with non zero exit value if failing #6602

Merged
merged 1 commit into from
Jan 19, 2021

Conversation

tned73
Copy link
Contributor

@tned73 tned73 commented Jan 18, 2021

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

There is an issue with version 5.x for the celery command. If the command is failing, the exit value is still 0. This PR will fix this and exits with the intended exit value.

NOTE: All patches should be made against master, not a maintenance branch like
3.1, 2.5, etc. That is unless the bug is already fixed in master, but not in
that version series.

If it fixes a bug or resolves a feature request,
be sure to link to that issue via (Fixes #6601) for example.
-->

@lgtm-com
Copy link

lgtm-com bot commented Jan 18, 2021

This pull request fixes 2 alerts when merging 2f5758f into 855d551 - view on LGTM.com

fixed alerts:

  • 1 for Non-exception in 'except' clause
  • 1 for Module is imported with 'import' and 'import from'

Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

We don't need a specialized exception.
We can simply use ctx.fail().

@thedrow thedrow self-assigned this Jan 18, 2021
@tned73
Copy link
Contributor Author

tned73 commented Jan 18, 2021

If I look at ctx.fail() it has a message parameter and not a exit value parameter. Using this will solve a part of the issue. It will probably exit with value 1 and not the 69 if the celery inspect ping fails as version 4.x does.
for my purposes it is not an issue.

please advice on direction to solve this issue

@thedrow
Copy link
Member

thedrow commented Jan 18, 2021

Huh.
That API is very surprising.

I think we're good to go on this one.

Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Looks good. should we merge this as is or need unit test?

@thedrow
Copy link
Member

thedrow commented Jan 19, 2021

There are no unit tests for the CLI at the time being, unfortunately.
I don't have time to write them, currently.

@thedrow thedrow merged commit 2551162 into celery:master Jan 19, 2021
@thedrow thedrow modified the milestones: 5.0.6, 5.1.0 Feb 24, 2021
@thedrow thedrow added this to Done in Celery 5.1.0 Feb 24, 2021
jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Celery 5.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

Celery command version 5.x if failing exits with a 0 value
3 participants