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

Add map_index parameter to extra links API #46107

Merged
merged 4 commits into from
Feb 3, 2025
Merged

Add map_index parameter to extra links API #46107

merged 4 commits into from
Feb 3, 2025

Conversation

shubhamraj-git
Copy link
Contributor

related: #45614 (This PR is for 3.0)

image

Try here after building: http://localhost:29091/docs#/Extra%20Links/get_extra_links

^ 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.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Jan 27, 2025
@shubhamraj-git shubhamraj-git mentioned this pull request Jan 27, 2025
Merged
@eladkal
Copy link
Contributor

eladkal commented Jan 27, 2025

Thanks for this!
I have to say that from my point of view this is a bug fix not a feature. Given #43757
I think we should backport it to 2.10 - this is really something that causes pain for users on 2.10 (including myself)

@shubhamraj-git
Copy link
Contributor Author

Sure, also we have to add tests regarding this. This PR don't do that.

pierrejeambrun
pierrejeambrun approved these changes Jan 27, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Feel free to open another PR directly against v2-10-test test branch.

Sure, also we have to add tests regarding this. This PR don't do that.

Yes ideally we want a small test before merging. Nothing to complicated but just to ensure things are working as expected.

jscheffl
jscheffl approved these changes Jan 27, 2025
View reviewed changes
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.

Yeah some test would be great, else LGTM!
And I second that a PR against v2-10-test as back-port (logical) to 2.10 would be good. But a direct backport with automation is not possible as it would need to be added against the old Connexion framework...

@shubhamraj-git
Copy link
Contributor Author

@jscheffl @pierrejeambrun Yes, I will be adding tests soon.
Will take care of backport PR, It's a separate fix altogether.

@tirkarthi
Copy link
Contributor

@shubhamraj-git Any reason to close this PR without merge?

@shubhamraj-git
Copy link
Contributor Author

This is just to prevent accidental merge till the tests are added, I will reopen in a while.

@eladkal eladkal reopened this Jan 29, 2025
eladkal
eladkal requested changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Temporary request changes to avoid accidental merge

@shubhamraj-git shubhamraj-git force-pushed the add_map_index_for_extra_links branch from 557b03a to 2ab6cda Compare January 29, 2025 16:33
pierrejeambrun
pierrejeambrun reviewed Jan 30, 2025
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Task SDK does not support extra links yet.

You can mark the test as xfail and add a comment:

# TODO: TaskSDK need to fix this
# Extra links should work for mapped operator

@pierrejeambrun
Copy link
Member

cc: @ashb

@shubhamraj-git
Copy link
Contributor Author

Task SDK does not support extra links yet.

You can mark the test as xfail and add a comment:

# TODO: TaskSDK need to fix this
# Extra links should work for mapped operator

Do we still backport this fix? Or wait for Task SDK to support extra links
cc: @eladkal @jscheffl @pierrejeambrun

@eladkal
Copy link
Contributor

eladkal commented Jan 30, 2025 •
edited
Loading

Task SDK does not support extra links yet.
You can mark the test as xfail and add a comment:

# TODO: TaskSDK need to fix this
# Extra links should work for mapped operator

Do we still backport this fix? Or wait for Task SDK to support extra links cc: @eladkal @jscheffl @pierrejeambrun

#43757 is a bug on 2.10 so we can raise PR to fix the issue directly on 2.10 test branch. This is not related to task SDK (in terms of we can fix it today on 2.10 and have a seperated fix for airflow 3 on main branch later on)

@shubhamraj-git
Copy link
Contributor Author

@eladkal raised PR for 2.10 #46337

@eladkal
Copy link
Contributor

eladkal commented Feb 3, 2025

If no further comments I'm happy to merge

@shubhamraj-git shubhamraj-git mentioned this pull request Feb 3, 2025
Closed
2 tasks
@shubhamraj-git
Copy link
Contributor Author

I think we can merge this, the fix needed on the task SDK can be taken later on.
Raised a issue in case someone can pickup early #46363
Tests are marked as xfail till then. we need to fix that post the fix for above issue gets merge.

tirkarthi
tirkarthi approved these changes Feb 3, 2025
View reviewed changes
Copy link
Contributor

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

LGTM. I can rebase the changes after merge for UI in #46074 . Thanks @shubhamraj-git

@pierrejeambrun pierrejeambrun merged commit 56fdc20 into apache:main Feb 3, 2025
91 checks passed
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
@shubhamraj-git @niklasr22
Add map_index parameter to extra links API (apache#46107)
22822cd 
* add map_index

* Add tests for extra links with mapped task

* Mark Xfail the test_should_respond_200_mapped_task_instance
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
@shubhamraj-git @ambika-garg
Add map_index parameter to extra links API (apache#46107)
5b271d5 
* add map_index

* Add tests for extra links with mapped task

* Mark Xfail the test_should_respond_200_mapped_task_instance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tirkarthi tirkarthi tirkarthi approved these changes

@bbovenzi bbovenzi bbovenzi approved these changes

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@eladkal eladkal eladkal approved these changes

@jscheffl jscheffl jscheffl approved these changes

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

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

Assignees
No one assigned
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
AIP-84 MODERN REST API
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shubhamraj-git @eladkal @tirkarthi @pierrejeambrun @bbovenzi @jscheffl