Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (dabccf0) 87.42% compared to head (11c772b) 87.44%.
Report is 8 commits behind head on main.

@@            Coverage Diff             @@
##             main    #8462      +/-   ##
==========================================
+ Coverage   87.42%   87.44%   +0.01%     
==========================================
  Files         148      148              
  Lines       18488    18488              
  Branches     3155     3155              
==========================================
+ Hits        16164    16166       +2     
+ Misses       2034     2033       -1     
+ Partials      290      289       -1     

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

approving on principle. but are we sure we don not have any other edge case?

Is there a particular edge you're concerned about?

As I mentioned, if I understand the tests correctly, there is a fairly glaring shortcoming: there's no test that the --skip-checks option to the cli utility feeds the expected value into CELERY_SKIP_CHECKS in the environment. In fact, there's no test at all that the --skip-checks option doesn't entirely crash the utility (which it, in fact, did prior to this patch).

There's also something between a bug and a lack of documentation about the CELERY_SKIP_CHECKS variable. The documentation for it says, generically, "Provide a default value..." without any mention of how the value is interpreted. Any truthy value (i.e., non-empty string) is interpreted as "skip the initial checks." I hesitated to use the value 'true' here because that falsely suggests that setting CELERY_SKIP_CHECKS=false might disable it. (IMO a check similar to the strtobool() utility in the serializer code would be ideal, but a quick scan of the code base found at least one other place where the simpler truthiness check was applied to an environment variable used as a flag.)

I'd be happy to capture these as tickets, but imo they're substantive enough that I didn't want to start addressing them as a drive-by. Someone more involved in the project needs to think about them.

should we at least correct some of the doc issues in this PR? I am OK to merge this soon

Sure. I think the Provide a default for --skip-checks is coming from sphinx-click and I don't see a way to adjust that. Instead, I've added text to the --skip-checks help that should make it clear.

I'm going to separately push a minor tweak to the CLI documentation to point out that the environment variables all need a CELERY_ prefix. (edit: done, it's #8469)

I think that should eliminate the lint and improve the coverage.

Tests appear to be failing, but it's not clear to me why. If there's something specific to be done please let me know but I'm at the limit of my familiarity with the project and my time available to debug.