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

AIP-84 Migrate public xcom endpoint get entry to fastapi #43521

Merged

Conversation

michaeljs-c
Copy link
Contributor

@michaeljs-c michaeljs-c commented Oct 30, 2024

closes #42978


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

@michaeljs-c michaeljs-c marked this pull request as ready for review October 30, 2024 18:26
@michaeljs-c michaeljs-c force-pushed the migrate-xcom-getentry-to-fastapi branch from b107d68 to 37e36a4 Compare October 31, 2024 23:15
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. Overall looking nice, a few minor suggestions.

We are missing the mark_fastapi_migration_done, check �17_adding_api_endpoints.rst for more information.

airflow/api_fastapi/core_api/routes/public/xcom.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/xcom.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_xcom.py Outdated Show resolved Hide resolved
@michaeljs-c michaeljs-c force-pushed the migrate-xcom-getentry-to-fastapi branch from a48ad67 to 73a95e3 Compare November 6, 2024 22:57
@michaeljs-c
Copy link
Contributor Author

Thank you for the PR. Overall looking nice, a few minor suggestions.

We are missing the mark_fastapi_migration_done, check �17_adding_api_endpoints.rst for more information.

Thanks for the comments. I've updated the PR. It looks like adding the migration decorator caused CI to fail due to changes in the legacy API requiring a legacy tag?

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Minor change in how we define routes. TLDR we should not use async.

More info here #43797

I just pushed a commit to remove the async so we can merge.

Thanks again for your contribution. 🎉

airflow/api_fastapi/core_api/routes/public/xcom.py Outdated Show resolved Hide resolved
@pierrejeambrun pierrejeambrun added the legacy api Whether legacy API changes should be allowed in PR label Nov 8, 2024
@pierrejeambrun pierrejeambrun reopened this Nov 8, 2024
@pierrejeambrun
Copy link
Member

@michaeljs-c Can you please rebase and solve conflicts, we are good to merge after that :)

@michaeljs-c michaeljs-c force-pushed the migrate-xcom-getentry-to-fastapi branch from 3a48142 to 0b2b68f Compare November 8, 2024 22:06
@michaeljs-c
Copy link
Contributor Author

@michaeljs-c Can you please rebase and solve conflicts, we are good to merge after that :)

All done. Thanks @pierrejeambrun!

@pierrejeambrun pierrejeambrun merged commit 5bb9221 into apache:main Nov 12, 2024
56 checks passed
@pierrejeambrun pierrejeambrun added the AIP-84 Modern Rest API label Nov 12, 2024
@pierrejeambrun pierrejeambrun changed the title Migrate public xcom endpoint get entry to fastapi AIP-84 Migrate public xcom endpoint get entry to fastapi Nov 12, 2024
sunank200 pushed a commit to astronomer/airflow that referenced this pull request Nov 12, 2024
* Migrate public xcom get entry to fastapi

* Update date parse call

* add migration decorator, update params and test

* Minor adjustment

* Fix import and api spec

---------

Co-authored-by: pierrejeambrun <[email protected]>
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
* Migrate public xcom get entry to fastapi

* Update date parse call

* add migration decorator, update params and test

* Minor adjustment

* Fix import and api spec

---------

Co-authored-by: pierrejeambrun <[email protected]>
@pierrejeambrun pierrejeambrun added this to the Airflow 3.0.0 milestone Nov 26, 2024
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
* Migrate public xcom get entry to fastapi

* Update date parse call

* add migration decorator, update params and test

* Minor adjustment

* Fix import and api spec

---------

Co-authored-by: pierrejeambrun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-84 Modern Rest API legacy api Whether legacy API changes should be allowed in PR
Projects
Development

Successfully merging this pull request may close these issues.

AIP-84 Migrate XCom get entry public endpoint to FastAPI
2 participants