-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add Required Actions tab in group task page
#54322
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
Conversation
Required Actions tab in group task page
jscheffl
left a comment
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.
Looks good in my view!
pierrejeambrun
left a comment
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.
Really nice.
Unfortunately we will need some backend change I think before we can implement this feature. More details in my comment bellow.
| const tabs = [ | ||
| { icon: <MdOutlineTask />, label: translate("tabs.taskInstances"), value: "" }, | ||
| { icon: <FiUser />, label: translate("tabs.requiredActions"), value: "required_actions" }, | ||
| ]; |
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.
You don't need to create the full list to then filter it again.
Example:
...(Boolean(taskId)
? []
: [
{
accessorKey: "task_id",
enableSorting: true,
header: translate("common:taskId"),
meta: {
skeletonWidth: 10,
},
},
]),
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.
Yes, it does make the code more clear and more efficient. I would update it.
| { | ||
| dagId, | ||
| dagRunId: runId, | ||
| taskIdPattern: groupId, |
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.
Using taskIdPattern to retrieve 'group related' HILT will not work in every case. While this is true that task from a same group tend to share the same prefix, it also mean that any task outside the group that also maches the pattern will get returned wrongly in here and displayed in the group actions.
For instance, based on your example, I added a TI, outside all groups, and since the name matches the 'validation' prefix, this will end up like this in the UI
validation_single_task = HITLBranchOperator(
task_id="validation_single_task",
subject="This is the validation for the single task",
options=["1", "2", "3"],
)
We need to add a backend filter on group_id I think. To be sure to only retrieve 'group' related TIs.
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 clear example that makes sense to me. Let me mark this as draft to wait the backend update.
|
Close since it seems no needed currently. |
Why
How
tested with this example
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.