Skip to content

Fixes #8224 Fixes worker command optimizations flag#8312

Merged
auvipy merged 1 commit intocelery:mainfrom
stuart-bradley:fix_8224_optimizations_flag_for_worker_command
Jun 15, 2023
Merged

Fixes #8224 Fixes worker command optimizations flag#8312
auvipy merged 1 commit intocelery:mainfrom
stuart-bradley:fix_8224_optimizations_flag_for_worker_command

Conversation

@stuart-bradley
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

auvipy commented Jun 13, 2023

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

@codecov
Copy link
Copy Markdown

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
Copy Markdown
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
help="Logging level.")
@click.option('optimization',
'-O',
@click.option('-O',
Copy link
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Before :)

Copy link
Copy Markdown
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
@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