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

Fixes #8224 Fixes worker command optimizations flag #8312

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Fixes #8224 Fixes worker command optimizations flag #8312

merged 1 commit into from
Jun 15, 2023

Conversation

stuart-bradley
Copy link
Contributor

@stuart-bradley stuart-bradley commented Jun 13, 2023

Description

This PR fixes #8224 wherein the optimizations flag of the worker command lost its double -, resulting in unintended breaking behaviour in 5.*.

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.

did you check the celery 5.0.0 release notes and migration guide?

@stuart-bradley
Copy link
Contributor Author

stuart-bradley commented Jun 13, 2023

@auvipy I did, this change is not referenced. There is reference to the CLI changes, but I don't think this change is covered by it: https://docs.celeryq.dev/en/v5.0.0/whatsnew-5.0.html#step-1-adjust-your-command-line-invocation

@auvipy auvipy requested a review from thedrow June 13, 2023 09:20
@auvipy
Copy link
Member

auvipy commented Jun 13, 2023

Celery 5.0 introduces a new CLI implementation which isn’t completely backwards compatible.

The global options can no longer be positioned after the sub-command. Instead, they must be positioned as an option for the celery command like so:

@auvipy
Copy link
Member

auvipy commented Jun 13, 2023

@Nusnus it would be great if you could take a feedback from Omer

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e540a8d) 87.16% compared to head (b5947b3) 87.16%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8312   +/-   ##
=======================================
  Coverage   87.16%   87.16%           
=======================================
  Files         148      148           
  Lines       18469    18469           
  Branches     3148     3148           
=======================================
  Hits        16098    16098           
  Misses       2092     2092           
  Partials      279      279           
Flag Coverage Δ
unittests 87.12% <100.00%> (ø)

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

Impacted Files Coverage Δ
celery/bin/worker.py 57.81% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Nusnus Nusnus self-requested a review June 13, 2023 21:19
@Nusnus
Copy link
Member

Nusnus commented Jun 13, 2023

@auvipy

@Nusnus it would be great if you could take a feedback from Omer

He's OOO so to speak for some time and available only for critical/urgent issues so I prefer to not reach him with that at the moment and help myself instead.

So I've reviewed the changes myself, and it makes sense to me.
This definitely looks like a typo, and the fix looks correct.

Thank you @stuart-bradley!
Added my approval.

@Nusnus Nusnus added this to the 5.3.x milestone Jun 13, 2023
@@ -166,8 +166,8 @@ def detach(path, argv, logfile=None, pidfile=None, uid=None,
type=LOG_LEVEL,
help_group="Worker Options",
help="Logging level.")
@click.option('optimization',
'-O',
@click.option('-O',
Copy link
Member

Choose a reason for hiding this comment

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

-O before or after? just for double check

Copy link
Member

Choose a reason for hiding this comment

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

Before :)

Copy link
Member

@Nusnus Nusnus Jun 14, 2023

Choose a reason for hiding this comment

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

See code above/below, the pattern is consistent being first -O, then --optimization

@auvipy auvipy merged commit 3f965eb into celery:main Jun 15, 2023
29 checks passed
@stuart-bradley stuart-bradley deleted the fix_8224_optimizations_flag_for_worker_command branch June 15, 2023 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why has the CLI worker command flag optimizations changed from --optimizations in 5.*?
3 participants