-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Refactor tasks into decorators #4666
Refactor tasks into decorators #4666
Conversation
Pylint doesn't like bounded tasks
step = UpdateDocsTaskStep(task=self) | ||
return step.run(*args, **kwargs) | ||
@app.task(bind=True, max_retries=5, default_retry_delay=7 * 60) | ||
def update_docs_task(self, project_id, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pylint doesn't like binded task, the self parameter gets injected by the decorator.
tasks.SyncRepositoryTask().run( | ||
version.pk, | ||
) | ||
tasks.sync_repository_task(version.pk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that calling directly to run is the same as executing the task on the same host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this should be equal
I think this is ready, we can refactor more: classes aren't needed here, we can refactor those to be just functions. Let me know if you want me to do that here, or maybe we can do it in another PR. Also, I tested with some builds locally (also some commands), it works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another nice clean refactor 👍
I did a quick scan on our commercial hosting code, and I don't think we rely on any of the classes here. This probably won't break things, but we'll need some QA here as well.
tasks.SyncRepositoryTask().run( | ||
version.pk, | ||
) | ||
tasks.sync_repository_task(version.pk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this should be equal
Looks like need to fix some linter errors |
Yeah i think in part due to my resolve. Seems to be okay now though |
Error fixed, it was because of #4653 |
Closes #3973
This will also helps with #3984 later