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

Allow users to write dag_id and task_id in their national characters, added display name for dag / task (v2) #38446

Merged
merged 26 commits into from
Mar 27, 2024
Merged

Allow users to write dag_id and task_id in their national characters, added display name for dag / task (v2) #38446

merged 26 commits into from
Mar 27, 2024

Conversation

jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Mar 25, 2024 •
edited
Loading

This PR tries to continue the work from [ @aleksandr-shel @xgao1023 @uranusjr ] in PR #35320. It merges with current main. On top of this I fixed the bugs in pytest, some glitches in UI.

I'd LOVE to get this into 2.9.0 and as there were a couple of reviews already on the previous PR I hope it is not as compex as it looks. A lot of code is also due to the fact that I try to migrate all examples over to "nicer names". Sneak preview how it could look like after this PR:
image

What is open to be merged?

  • Review of course
  • Migrate the second half of example DAGs

closes: #22073
related: #32520, #28183

@boring-cyborg boring-cyborg bot added the provider:cncf-kubernetes Kubernetes provider related issues label Mar 25, 2024
@jscheffl jscheffl changed the title Fix 22073 Allow users to write dag_id and task_id in their national characters, added display name for dag / task (v2) Mar 25, 2024
@jscheffl jscheffl added the area:UI Related to UI/UX. For Frontend Developers. label Mar 25, 2024
@jscheffl jscheffl mentioned this pull request Mar 25, 2024
Closed
potiuk
potiuk approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I reviewed the backend code and looked at the UI. Not sure if all the places in UI that should be updated, are but the backend implementation looks sound to me. I think some other pairs of eyes shold also look at that though.

@potiuk
Copy link
Member

potiuk commented Mar 25, 2024

The failing MINSQL ALchemy test here is fixed in #38445

@ephraimbuddy ephraimbuddy added this to the Airflow 2.9.0 milestone Mar 25, 2024
ephraimbuddy
ephraimbuddy reviewed Mar 25, 2024
View reviewed changes
Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

I don't think we should modify a lot of example dags to add task_display_name on them

@ephraimbuddy ephraimbuddy mentioned this pull request Mar 25, 2024
Merged
@jscheffl
Copy link
Contributor Author

I don't think we should modify a lot of example dags to add task_display_name on them

Okay, then before going into a debate I'll revert the adjustments in the example DAGs for most cases and leave this for future cleanup.
I#l like to consolidate a lot of examples though. I believe we have many many and maybe (after release of 2.9) it is time to think about consolidating them.

@uranusjr
Copy link
Member

We should add a couple of sections in the documentation about the fields. Not sure where tbh.

@jscheffl
Copy link
Contributor Author

We should add a couple of sections in the documentation about the fields. Not sure where tbh.

How can I interpret your "Should"?
a) Must be before approval+merge
b) Okay to merge but do it before release
c) would be nice sooner or later

@jscheffl
Copy link
Contributor Author

I hope (&pray) that I catched all errors now and pipeline turns green...

@potiuk
Copy link
Member

potiuk commented Mar 26, 2024

Looks good

@uranusjr
Copy link
Member

Docs can be added in a separate PR, but I would prefer to have it for the final release. (Don’t need to hit alphas though, docs don’t break things.)

@jscheffl jscheffl requested a review from uranusjr March 27, 2024 00:21
@ephraimbuddy ephraimbuddy merged commit 8c44bcb into apache:main Mar 27, 2024
47 checks passed
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Mar 27, 2024
@cmarteepants cmarteepants mentioned this pull request Mar 28, 2024
Closed
2 tasks
@jscheffl jscheffl mentioned this pull request Mar 29, 2024
Merged
mathiaHT pushed a commit to mathiaHT/airflow that referenced this pull request Apr 4, 2024
@jscheffl @xgao1023
@uranusjr @aleksandr-shel
Allow users to write dag_id and task_id in their national characters,…
4e77fe8 
… added display name for dag / task (v2) (apache#38446)

* Add display name in DAGs and tasks

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Co-authored-by: Aleksandr Shelukheev <shelukheev@gmail.com>

* Fix frontend issues

* Fix pytests

* Add DAG display tool tips

* Implement national language char display example oon two DAGs

* Review nit from other PR

* Fix WWW pytest on HTML code

* Review feedback, make task_display_name a property, not a field in mapped operator

* Review feedback, rename internal fields

* Small nit, optimize sorting on DAG home page with display name

* Review feedback

* Fix pytests in API plus review feedback on API

* Review feedback, extend schema, update apispecs and fix breadcrumb navigation

* nit, update page titles as well

* Fix pytests for extended API model

* Ensure mapped operator also provides a display string

* Ensure display field is only serialized if different from task id

* fix pytest for API

* Review feedback from TP

* Move labels down to base and mapped operator

* Move labels up to abstract operator

* Only one decorator is needed

---------

Co-authored-by: Vincent Gao <xgao1023@gmail.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Co-authored-by: Aleksandr Shelukheev <shelukheev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ephraimbuddy ephraimbuddy ephraimbuddy left review comments

@bbovenzi bbovenzi bbovenzi approved these changes

@uranusjr uranusjr uranusjr approved these changes

@potiuk potiuk potiuk approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@pierrejeambrun pierrejeambrun Awaiting requested review from pierrejeambrun pierrejeambrun is a code owner

@kaxil kaxil Awaiting requested review from kaxil

@bolkedebruin bolkedebruin Awaiting requested review from bolkedebruin bolkedebruin is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

Assignees
No one assigned
Labels
area:UI Related to UI/UX. For Frontend Developers. provider:cncf-kubernetes Kubernetes provider related issues type:new-feature Changelog: New Features
Projects
None yet
Milestone
Airflow 2.9.0
Development

Successfully merging this pull request may close these issues.

Separate dag/task UI display name from dag / task id
6 participants
@jscheffl @potiuk @uranusjr @ephraimbuddy @bbovenzi @xgao1023