Skip to content

AIP 84: Migrate GET one ASSET legacy API to fast API - #43825

Merged
amoghrajesh merged 27 commits into
apache:mainfrom
astronomer:AIP84-get-one-asset-to-fastapi
Nov 14, 2024
Merged

AIP 84: Migrate GET one ASSET legacy API to fast API#43825
amoghrajesh merged 27 commits into
apache:mainfrom
astronomer:AIP84-get-one-asset-to-fastapi

Conversation

@amoghrajesh

@amoghrajeshamoghrajesh commented Nov 8, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

NOTE TO REVIEWERS
Only last 2 commits are relevant

Depends on #43783
related: #42370

Migrating the connexion API for GET one ASSETS to fastAPI.

Testing performed:

Same setup as #43783

API responses:

Legacy:
image

FastAPI:
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-cyborgboring-cyborgBot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Nov 8, 2024
amoghrajesh
amoghrajesh commented Nov 8, 2024
View reviewed changes

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@pierrejeambrun a note for you

Comment threadairflow/api_fastapi/core_api/routes/public/assets.py
@amoghrajeshamoghrajesh mentioned this pull request Nov 8, 2024
Closed
1 task
kaxil
kaxil reviewed Nov 8, 2024
View reviewed changes
Comment threadairflow/api_fastapi/core_api/serializers/assets.py
pierrejeambrun
pierrejeambrun reviewed Nov 8, 2024
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few common comments from the previous PR that this work is based off.

Otherwise looks good, I'll do a final in depth review once the previous PR is merged.

@rawwarrawwar added the legacy api Whether legacy API changes should be allowed in PR label Nov 9, 2024
@amoghrajesh

amoghrajesh commented Nov 13, 2024

Copy link
Copy Markdown
ContributorAuthor

@pierrejeambrun the PR has been rebased now. Only has the relevant changes

pierrejeambrun
pierrejeambrun reviewed Nov 13, 2024
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

Comment threadairflow/api_fastapi/core_api/routes/public/assets.py Outdated
Comment threadairflow/api_fastapi/core_api/routes/public/assets.py Outdated
pierrejeambrun
pierrejeambrun reviewed Nov 13, 2024
View reviewed changes
Comment threadairflow/api_fastapi/core_api/routes/public/assets.py Outdated
pierrejeambrun
pierrejeambrun reviewed Nov 13, 2024
View reviewed changes
Comment threadtests/api_fastapi/core_api/routes/public/test_assets.py
pierrejeambrun
pierrejeambrun approved these changes Nov 13, 2024
View reviewed changes

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good.

One more nit on the tests and we can merge

@amoghrajesh

amoghrajesh commented Nov 14, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review @pierrejeambrun! Handled the review comment for tests. Merging it

@amoghrajesh
amoghrajesh merged commit c84d356 into apache:mainNov 14, 2024
@amoghrajesh
amoghrajesh deleted the AIP84-get-one-asset-to-fastapi branch November 14, 2024 04:05
amoghrajesh added a commit to astronomer/airflow that referenced this pull request Nov 14, 2024
@amoghrajesh
AIP 84: Migrate GET one ASSET legacy API to fast API (apache#43825)
556766e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaxilkaxilkaxil left review comments
@pierrejeambrunpierrejeambrunpierrejeambrun approved these changes
@bbovenzibbovenziAwaiting requested review from bbovenzi
@ryanahamiltonryanahamiltonAwaiting requested review from ryanahamilton
@jscheffljschefflAwaiting requested review from jscheffl
@ephraimbuddyephraimbuddyAwaiting requested review from ephraimbuddy

Assignees

@amoghrajeshamoghrajesh

Labels

area:APIAirflow's REST/HTTP APIarea:UIRelated to UI/UX. For Frontend Developers.legacy apiWhether legacy API changes should be allowed in PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amoghrajesh@kaxil@pierrejeambrun@rawwar