-
Notifications
You must be signed in to change notification settings - Fork 112
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
Add 'all_tasks_dispatched' to TaskGroup #682
Conversation
Attached issue: https://pulp.plan.io/issues/6591 |
e21ebed
to
0a46921
Compare
migrations.AddField( | ||
model_name='taskgroup', | ||
name='all_tasks_dispatched', | ||
field=models.BooleanField(default=False), |
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.
Since the default is "false" we need to go back through and change all of the existing ones to True.
The code looks great. I think since we expose this field to end users, we should probably have a changelog entry for end users too. |
CHANGES/6591.feature
Outdated
@@ -0,0 +1 @@ | |||
Task groups now have an 'all_tasks_dispatched' field which denotes that no more tasks will spawn as part of this group. |
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.
nitpick: can this have a newline char at the end? I think it will make the changelog look inconsistent otherwise.
CHANGES/plugin_api/6591.feature
Outdated
@@ -0,0 +1 @@ | |||
TaskGroups now have an 'all_tasks_dispatched' field that can be used to notify systems that no further tasks will be dispatched for a TaskGroup. Plugin writers should call ".finish()" on all TaskGroups created once they are done using them to set this field. |
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.
nitpick: can this have a newline char at the end I think it will make the changleog look inconsistent otherwise.
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.
Can this be wrapped to 100 chars please?
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.
Can plugin writer docs be added for this? I think a TaskGroups section under the 'Tasks' part of the plugin writer guide would be a good place for them https://docs.pulpproject.org/plugins/plugin-writer/concepts/index.html#tasks
If I remember correctly, the task groups are not documented. Regardless I don't think we should continue down the road of adding plugin features without docs.
@@ -92,7 +94,11 @@ this is accomplished. | |||
# Since tasks are asynchronous, we return a 202 | |||
return OperationPostponedResponse(result, request) | |||
|
|||
See :class:`~pulpcore.plugin.tasking.enqueue_with_reservation` for more details. | |||
If a "task_group" is provided as an optional keyword argument, then the deployed task will be |
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.
Can we make this section linkable as task_groups
and link directly to it?
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'm not sure what you mean exactly
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 mean can above this paragraph something like this be added? I'm not 100% if RST allows it is why I'm a bit unsure.
.. _dispatching-task-groups:
or maybe
.. _task-groups:
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.
well, I formatted it this way because that is the name of the kwarg for enqueue_with_reservation
. I could put the link somewhere near here though I guess?
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 something linkable would be good. This is an optional thing, the main thing is the changelog for me.
7146727
to
ea1693c
Compare
CHANGES/plugin_api/6591.feature
Outdated
@@ -0,0 +1 @@ | |||
TaskGroups now have an 'all_tasks_dispatched' field that can be used to notify systems that no further tasks will be dispatched for a TaskGroup. Plugin writers should call ".finish()" on all TaskGroups created once they are done using them to set this field. |
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.
Can this be line wrapped like all pulp docs are (to 100 chars).
|
||
def finish(self): | ||
""" | ||
Set 'all_tasks_dispatched' to True so that API users can know that there are no |
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.
Can this docstring get a single line summary as its top line to be PEP 257 compliant
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.
Thanks for adding the docs. Please merge after fixing the line wrapping.
closes: #6591
https://pulp.plan.io/issues/6591