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

Adding celery scaffold to the project. #36

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

fuzzball81
Copy link
Member

Adding celery_scaffold for use with flask based projects. It provides a way to configure celery for use, using the flask configuration files. It also provides a celery_app and a flask_app that can be used in your project.

A base_scaffold was pulled out due to the celery worker assuming any 'app' attribute is of type Celery. The original app_scaffold is available to provide backward compatibility. It leverages the new base_scaffold and sets the 'app' attribute to flask_app to ensure existing use cases are handled.

@fuzzball81
Copy link
Member Author

fuzzball81 commented Mar 28, 2024

The changes to tox and the GitHub action are due to an issue in import lib-metadata in python 3.7 see python/importlib_metadata#411 for more details. If we no longer need 3.7 support we could just remove that from tox and the action and everything should just work.

@jguiditta
Copy link
Member

The changes to tox and the GitHub action are due to an issue in import lib-metadata in python 3.7 see python/importlib_metadata#411 for more details. If we no longer need 3.7 support we could just remove that from tox and the action and everything should just work.

I would advocate removing 3.7 support so we don't have to make changes to work around it. That version is EOL, and I already removed it elsewhere. Happy to make a patch for that if it would help.

@fuzzball81
Copy link
Member Author

The changes to tox and the GitHub action are due to an issue in import lib-metadata in python 3.7 see python/importlib_metadata#411 for more details. If we no longer need 3.7 support we could just remove that from tox and the action and everything should just work.

I would advocate removing 3.7 support so we don't have to make changes to work around it. That version is EOL, and I already removed it elsewhere. Happy to make a patch for that if it would help.

Go for it, I am happy to rebase my patch on that. We may want another patch to add 3.12 since that is the default in Fedora now.

Copy link
Member

@jguiditta jguiditta left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. I am hoping the python version stuff will be able to be removed once my patch merges, and I have a few thoughts, largely around documentation.

setup.cfg Outdated Show resolved Hide resolved
src/flask_container_scaffold/celery_scaffold.py Outdated Show resolved Hide resolved
src/flask_container_scaffold/app_scaffold.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@fuzzball81 fuzzball81 force-pushed the celery-integration branch 5 times, most recently from 4f13411 to 9482e98 Compare April 2, 2024 11:34
Copy link
Member

@jguiditta jguiditta left a comment

Choose a reason for hiding this comment

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

Super minor item or two, otherwise, looking good to me!

README.md Outdated Show resolved Hide resolved
src/flask_container_scaffold/app_scaffold.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@fuzzball81 fuzzball81 force-pushed the celery-integration branch 2 times, most recently from 3ea4232 to d947d91 Compare April 3, 2024 21:47
Copy link
Member

@jguiditta jguiditta left a comment

Choose a reason for hiding this comment

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

Couple minor points left over, but I think this is just about ready.

# Add the celery app as an extension to the flask app so it can be easily
# accessed if a flask application factory pattern is used.
# see https://flask.palletsprojects.com/en/2.3.x/patterns/celery/ for details.
self.flask_app.extensions["celery"] = self.celery_app
Copy link
Member

Choose a reason for hiding this comment

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

This is a good improvement if it helps when the factory pattern is used, as I know a number of our applications do this currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

This provides a lot of flexibility, if you want to access the celery app from the flask app you can, or you can use the class attribute. This doesn't force a single implementation pattern, which I like.

src/flask_container_scaffold/celery_scaffold.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
jpichon
jpichon previously approved these changes Apr 5, 2024
Copy link
Contributor

@jpichon jpichon left a comment

Choose a reason for hiding this comment

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

(praise) Really enjoying the README updates including link to relevant docs and installation/usage examples, really helpful!

It's really cool to see how nicely this works out. I'm approving although I'd like to see the rewording improvement suggested by Jay implemented if possible, as it also tripped me up on first read. Thanks for the patch!

README.md Outdated Show resolved Hide resolved
@@ -28,6 +28,9 @@ install_requires =
toolchest

[options.extras_require]
celery =
celery
Copy link
Contributor

Choose a reason for hiding this comment

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

(praise) I really like how self-contained the additions are, here and in the Pipfile, install instructions, etc. It works really neatly.

src/flask_container_scaffold/app_scaffold.py Show resolved Hide resolved
Adding celery_scaffold for use with flask based projects. It provides a
way to configure celery for use, using the flask configuration files. It
also provides a celery_app and a flask_app that can be used in your
project.

A base_scaffold was pulled out due to the celery worker assuming any 'app'
attribute is of type Celery. The original app_scaffold is available to
provide backward compatibility. It leverages the new base_scaffold and
sets the 'app' attribute to flask_app to ensure existing use cases are
handled.

Signed-off-by: Jason Joyce <[email protected]>
@jguiditta jguiditta enabled auto-merge (rebase) April 5, 2024 17:11
@jguiditta jguiditta merged commit 71ccbe0 into release-depot:main Apr 5, 2024
7 checks passed
@fuzzball81 fuzzball81 deleted the celery-integration branch April 8, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants