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

Add group_display_name attribute to allow specifying a custom display name in the UI for TaskGroup #45264

Merged

Conversation

hardeybisey
Copy link
Contributor

@hardeybisey hardeybisey commented Dec 28, 2024

closes: #44569


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@hardeybisey hardeybisey marked this pull request as draft December 28, 2024 16:40
@hardeybisey hardeybisey marked this pull request as ready for review December 28, 2024 19:03
@eladkal eladkal added this to the Airflow 3.0.0 milestone Dec 29, 2024
@eladkal eladkal added the type:new-feature Changelog: New Features label Dec 29, 2024
@hardeybisey hardeybisey force-pushed the accept_group_display_name_in_taskgroup branch from 2d7a6e8 to 85e2bb3 Compare December 29, 2024 11:10
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I always wanted to make this but the big shift to Airflow 3 distracted me from making this...

Once check that we need to discuss on attribute model...

... and besides this, would it make sense to consider adding UI changes in the same PR as well? Else the group display name would have no effect other than just adding an attribute.

@jscheffl jscheffl requested a review from uranusjr December 31, 2024 13:16
@hardeybisey
Copy link
Contributor Author

hardeybisey commented Jan 4, 2025

Thanks for the contribution! I always wanted to make this but the big shift to Airflow 3 distracted me from making this...

Once check that we need to discuss on attribute model...

... and besides this, would it make sense to consider adding UI changes in the same PR as well? Else the group display name would have no effect other than just adding an attribute.

Thank you for taking the time to review this and for your thoughtful feedback!

To clarify, when you mention the display name having no effect, are you suggesting it doesn’t currently reflect in the UI? In this implementation, when the group display name is provided, it is displayed in the UI as expected as shown below.

image

Regarding the UI changes, I’d be happy to extend this PR to also include a new Group Display Name key as part of the task group details on this page if that is what you are suggesting.
image

Please let me know the direction you’d like me to proceed. Thank you!

@jscheffl
Copy link
Contributor

jscheffl commented Jan 4, 2025

Thanks for the contribution! I always wanted to make this but the big shift to Airflow 3 distracted me from making this...
Once check that we need to discuss on attribute model...
... and besides this, would it make sense to consider adding UI changes in the same PR as well? Else the group display name would have no effect other than just adding an attribute.

Thank you for taking the time to review this and for your thoughtful feedback!

To clarify, when you mention the display name having no effect, are you suggesting it doesn’t currently reflect in the UI? In this implementation, when the group display name is provided, it is displayed in the UI as expected as shown below.
image

Regarding the UI changes, I’d be happy to extend this PR to also include a new Group Display Name key as part of the task group details on this page if that is what you are suggesting. image

Please let me know the direction you’d like me to proceed. Thank you!

Oh, did not notice that UI changed w/o touching the code. Cool.
I assume on current (legacy) UI no changes are needed. But if you like... the "new" UI is under construction and if you want to contribute there... any contribution is welcome! https://github.com/orgs/apache/projects/400

@hardeybisey
Copy link
Contributor Author

Thanks for the contribution! I always wanted to make this but the big shift to Airflow 3 distracted me from making this...
Once check that we need to discuss on attribute model...
... and besides this, would it make sense to consider adding UI changes in the same PR as well? Else the group display name would have no effect other than just adding an attribute.

Thank you for taking the time to review this and for your thoughtful feedback!
To clarify, when you mention the display name having no effect, are you suggesting it doesn’t currently reflect in the UI? In this implementation, when the group display name is provided, it is displayed in the UI as expected as shown below.
image
Regarding the UI changes, I’d be happy to extend this PR to also include a new Group Display Name key as part of the task group details on this page if that is what you are suggesting. image
Please let me know the direction you’d like me to proceed. Thank you!

Oh, did not notice that UI changed w/o touching the code. Cool. I assume on current (legacy) UI no changes are needed. But if you like... the "new" UI is under construction and if you want to contribute there... any contribution is welcome! https://github.com/orgs/apache/projects/400

Thanks for the feedback! Just to confirm, does this mean the PR is ready to go and meets the current requirements?

Also, I’m happy to contribute to other areas that are not UI-related, as I’m not an expert with frontend libraries or code 😄

@hardeybisey hardeybisey force-pushed the accept_group_display_name_in_taskgroup branch from 85e2bb3 to b1b3d1b Compare January 4, 2025 14:48
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Then from my view... good to be merged.

@jscheffl jscheffl merged commit 3a9a032 into apache:main Jan 4, 2025
91 checks passed
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
…ay name in the UI for TaskGroup (apache#45264)

* Add group_display_name attribute to allow specifying a custom display name in the UI for TaskGroup

* Add new tests and update old test to suppport the changes made.
agupta01 pushed a commit to agupta01/airflow that referenced this pull request Jan 6, 2025
…ay name in the UI for TaskGroup (apache#45264)

* Add group_display_name attribute to allow specifying a custom display name in the UI for TaskGroup

* Add new tests and update old test to suppport the changes made.
HariGS-DB pushed a commit to HariGS-DB/airflow that referenced this pull request Jan 16, 2025
…ay name in the UI for TaskGroup (apache#45264)

* Add group_display_name attribute to allow specifying a custom display name in the UI for TaskGroup

* Add new tests and update old test to suppport the changes made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Give the possibility to define a display name for task_group
4 participants