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.

Sign up for GitHub

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

Jump to bottom

Impl: signature in periodic task #361

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Impl: signature in periodic task #361

wants to merge 21 commits into from

Conversation

SunnyCapt
Copy link

@SunnyCapt SunnyCapt commented Sep 10, 2020 •
edited
Loading

changelog:

  • added support for task signature (by dill lib)
  • added support for task callback signature (by dill lib)
  • added support for performing an action before starting a periodic task (app.conf.call_before_run_periodic_task)

close: #360
close: #403

@SunnyCapt SunnyCapt changed the title Signature in periodic task Impl: signature in periodic task Sep 10, 2020
@auvipy auvipy self-requested a review September 12, 2020 08:34
auvipy
auvipy requested changes Oct 26, 2020
View reviewed changes
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.

check the unit test failures please

@SunnyCapt SunnyCapt closed this Oct 26, 2020
@SunnyCapt SunnyCapt reopened this Oct 26, 2020
@arnau126 arnau126 mentioned this pull request Nov 2, 2020
Merged
@auvipy
Copy link
Member

auvipy commented Dec 4, 2020

can you rebase please?

auvipy
auvipy reviewed Mar 4, 2021
View reviewed changes
django_celery_beat/utils.py
logger = get_logger(__name__)


def generate_keys(
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why we need rsa key?

Copy link
Author

@SunnyCapt SunnyCapt Mar 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it's need to sign serialized objects so as not to execute any python code that may appear in the database (execute only signed serilized code which can be signed and added only by main proc)

Copy link
Author

Choose a reason for hiding this comment

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

https://davidhamann.de/2020/04/05/exploiting-python-pickle/

Copy link
Member

Choose a reason for hiding this comment

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

why noy json serializer? it's default in celery

Copy link
Member

Choose a reason for hiding this comment

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

JSON serializer should be the default task serializer in the integration packages as well

Copy link
Author

@SunnyCapt SunnyCapt Apr 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

okay, I'll rewrite (I used dill because it was faster and easier to implement)

Copy link
Member

Choose a reason for hiding this comment

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

dill is ok for pickle, but pickle is not the default serializer. so rewriting this with json would be great

@auvipy auvipy self-assigned this Mar 4, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 21, 2021

This pull request introduces 1 alert when merging bd74d99 into 7416e22 - view on LGTM.com

new alerts:

  • 1 for Unused import

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auvipy auvipy auvipy requested changes

Assignees

@auvipy auvipy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performing an action before starting a periodic task Signature in periodic task
2 participants
@SunnyCapt @auvipy