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 Migrating GET ASSETS Legacy API to fastAPI #43783

Merged
merged 16 commits into from
Nov 12, 2024

Conversation

amoghrajesh
Copy link
Contributor

@amoghrajesh amoghrajesh commented Nov 7, 2024

related: #42370
Migrating the connexion API for GET ASSETS to fastAPI.

Testing performed:

  1. Created 2 pairs of DAGS each linking to different assets
    image

  2. API responses using legacy and fastAPI endpoints (one example with all params used)

Legacy API:
image

Fast API:
image

Swagger Spec:
image

image


^ 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 area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Nov 7, 2024
@pierrejeambrun pierrejeambrun added legacy api Whether legacy API changes should be allowed in PR AIP-84 Modern Rest API labels Nov 7, 2024
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@amoghrajesh
Copy link
Contributor Author

Okay seems like the dag_ids filter doesn't work yet. FIxing it

@pierrejeambrun pierrejeambrun changed the title AIP 84: Migrating GET ASSETS Legacy API to fastAPI AIP-84 Migrating GET ASSETS Legacy API to fastAPI Nov 8, 2024
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.

Looking good overall, thanks.

Need rebasing to solve conflicts, small nitpicks.

airflow/api_fastapi/core_api/routes/public/assets.py Outdated Show resolved Hide resolved
airflow/api_fastapi/common/parameters.py Show resolved Hide resolved
airflow/api_fastapi/common/parameters.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/assets.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_assets.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_assets.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_assets.py Outdated Show resolved Hide resolved
@amoghrajesh
Copy link
Contributor Author

With the latest commits, most of the things should be taken care of.
@pierrejeambrun and @ephraimbuddy addressed your comments

@pierrejeambrun
Copy link
Member

Thanks, don't hesitate to resolve conversation that you have adressed. I'm out of office until tuesday. (Bank holiday in france)

Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

Looks good thanks ! 🎉

@pierrejeambrun pierrejeambrun merged commit 3217576 into apache:main Nov 12, 2024
56 checks passed
@pierrejeambrun pierrejeambrun deleted the AIP84-get-asset-to-fastapi branch November 12, 2024 16:04
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
* AIP-84: Migrating GET Assets to fastAPI

* matching response to legacy

* Adding unit tests - part 1

* Update airflow/api_fastapi/common/parameters.py

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

* fixing the dag_ids filter

* fixing the dag_ids filter

* Adding unit tests - part 2

* fixing unit tests & updating parameter type

* review comments pierre

* fixing last commit

* fixing unit tests

---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-84 Modern Rest API area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. legacy api Whether legacy API changes should be allowed in PR
Projects
Development

Successfully merging this pull request may close these issues.

5 participants