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

Migrate the public endpoint Get DAG Stats to FastAPI #43255

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Migrate the public endpoint Get DAG Stats to FastAPI #43255

merged 3 commits into from
Nov 5, 2024

Conversation

omkar-foss
Copy link
Collaborator

closes: #42877
related: #42370

This migrates the Get DAG Stats API from api_connexion to api_fastapi.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Oct 22, 2024
@omkar-foss omkar-foss force-pushed the aip-84/dags/get-dag-stats branch 3 times, most recently from 7519fd1 to a3a80ec Compare October 23, 2024 10:03
@omkar-foss omkar-foss marked this pull request as ready for review October 23, 2024 19:30
pierrejeambrun
pierrejeambrun reviewed Oct 25, 2024
View reviewed changes
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. A few suggestions and we should be good to merge.

tests/api_fastapi/core_api/routes/public/test_dag_stats.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_stats.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_dag_stats.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_stats.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_stats.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_stats.py Outdated Show resolved Hide resolved
@omkar-foss
Copy link
Collaborator Author

@pierrejeambrun All comments resolved, please have a look when possible. Thank you!

pierrejeambrun
pierrejeambrun reviewed Oct 30, 2024
View reviewed changes
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.

We are missing the @mark_fastapi_migration_done decorator

airflow/api_fastapi/common/db/dags.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/dag_stats.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_dag_stats.py Outdated Show resolved Hide resolved
@omkar-foss omkar-foss added the legacy api Whether legacy API changes should be allowed in PR label Nov 4, 2024
@omkar-foss
Copy link
Collaborator Author

We are missing the @mark_fastapi_migration_done decorator

Done, added it. And have added legacy api label to PR.

@omkar-foss omkar-foss closed this Nov 4, 2024
@omkar-foss omkar-foss reopened this Nov 4, 2024
@omkar-foss
Copy link
Collaborator Author

omkar-foss commented Nov 4, 2024 •
edited
Loading

@pierrejeambrun all conversations and conflicts resolved, PR ready to review ✅

Also kindly see this thread, let me know if any changes needed there: #43255 (comment)

pierrejeambrun
pierrejeambrun approved these changes Nov 5, 2024
View reviewed changes
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, thanks

@pierrejeambrun
Copy link
Member

Need rebasing but can be merged after that.

@omkar-foss
Copy link
Collaborator Author

Need rebasing but can be merged after that.

@pierrejeambrun done, PR rebased and synced with main ✅

@pierrejeambrun
Copy link
Member

Thanks @omkar-foss

@pierrejeambrun pierrejeambrun merged commit 69c1f92 into apache:main Nov 5, 2024
56 checks passed
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
@omkar-foss @ellisms
Migrate the public endpoint Get DAG Stats to FastAPI (apache#43255)
85bc5f0 
* Migrate public endpoint Get DAG Stats to FastAPI, with main resynced

* Re-run static checks

* Add newlines to separate entities
@omkar-foss omkar-foss deleted the aip-84/dags/get-dag-stats branch November 14, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@jscheffl jscheffl jscheffl approved these changes

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

@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. legacy api Whether legacy API changes should be allowed in PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AIP-84 Migrate the public endpoint Get DAG Stats to FastAPI
3 participants
@omkar-foss @pierrejeambrun @jscheffl