Skip to content

AIP-84: Migrating GET Dataset events for DAG runs api to fastAPI - #43874

Merged
pierrejeambrun merged 29 commits into
apache:mainfrom
astronomer:AIP84-get-dataset-events-dagrun
Nov 15, 2024
Merged

AIP-84: Migrating GET Dataset events for DAG runs api to fastAPI#43874
pierrejeambrun merged 29 commits into
apache:mainfrom
astronomer:AIP84-get-dataset-events-dagrun

Conversation

@amoghrajesh

Copy link
Copy Markdown
Contributor

related #42370

Depends on #43783

Migrating the GET Dataset events for DAG runs to fastAPI

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 11, 2024
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

Only the last 3 commits are relevant here

@pierrejeambrunpierrejeambrun added the AIP-84 Modern Rest API label Nov 12, 2024
@pierrejeambrun

pierrejeambrun commented Nov 12, 2024

Copy link
Copy Markdown
Member

The base branch has been merged, branch needs rebasing.

Also you can add the legacy api tag to allow the CI to build.

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

Nice just a couple of comments.

I'll do a final in depth review when the branch is rebased, it will make the review easier to only have the 'definitive' diff and not also the one from the base branch.

Comment threadairflow/api_fastapi/core_api/routes/public/dag_run.py Outdated
Comment threadairflow/api_fastapi/core_api/datamodels/assets.py Outdated
@amoghrajeshamoghrajesh added the legacy api Whether legacy API changes should be allowed in PR label Nov 13, 2024
@amoghrajeshamoghrajesh self-assigned this Nov 13, 2024

@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, a few suggestions

Comment threadairflow/api_fastapi/core_api/datamodels/assets.py Outdated
Comment threadairflow/api_fastapi/core_api/datamodels/assets.py Outdated
Comment threadairflow/api_fastapi/core_api/datamodels/assets.py Outdated

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

One small suggestion following the change of convention based on Kaxil's PR that was just merged yesterday.

Otherwise looking good and ready to merge.

Thanks

Comment threadairflow/api_fastapi/core_api/routes/public/dag_run.py Outdated
Comment threadtests/api_fastapi/core_api/routes/public/test_dag_run.py
@amoghrajesh

Copy link
Copy Markdown
ContributorAuthor

OK i got a green CI. Is this one good to merge @pierrejeambrun?

Comment threadtests/api_fastapi/core_api/routes/public/test_dag_run.py

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

Needs rebasing, small need and ready to merge.

Comment threadtests/api_fastapi/core_api/routes/public/test_dag_run.py Outdated
@pierrejeambrun

Copy link
Copy Markdown
Member

Thanks 🎉

@pierrejeambrun
pierrejeambrun merged commit 0f14f66 into apache:mainNov 15, 2024
@pierrejeambrun
pierrejeambrun deleted the AIP84-get-dataset-events-dagrun branch November 15, 2024 16:18
kandharvishnu pushed a commit to kandharvishnu/airflow that referenced this pull request Nov 19, 2024
…che#43874)
* 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
* AIP-84: Migrating GET Dataset events for DAG runs to fastAPI
* adding test cases
* adding test cases
* review comments pierre
* fixing unit tests
* review comments pierre
* review comments and fixing a test
* review comments on ut
---------
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

3 participants

@amoghrajesh@pierrejeambrun@rawwar