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

AIP-84 Migrate backfill API to fast api #43496

Merged
merged 5 commits into from
Nov 4, 2024
Merged

AIP-84 Migrate backfill API to fast api #43496

merged 5 commits into from
Nov 4, 2024

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Oct 29, 2024 •
edited by pierrejeambrun
Loading

Related to #42370

That is all.

@dstandish dstandish force-pushed the migrate-backfills-rest-api-to-fast-api branch from 788867c to e0bf1c6 Compare October 30, 2024 23:55
@dstandish dstandish marked this pull request as ready for review October 31, 2024 04:19
@dstandish dstandish changed the title WIP Migrate backfill API to fast api Migrate backfill API to fast api Oct 31, 2024
@pierrejeambrun pierrejeambrun added the legacy api Whether legacy API changes should be allowed in PR label Oct 31, 2024
pierrejeambrun
pierrejeambrun reviewed Oct 31, 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 great overall.

A few minor suggestions

airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/routes/public/backfills.py Outdated Show resolved Hide resolved
airflow/api_fastapi/core_api/serializers/backfills.py Outdated Show resolved Hide resolved
tests/api_fastapi/core_api/routes/public/test_backfills.py Outdated Show resolved Hide resolved
@pierrejeambrun pierrejeambrun added legacy ui Whether legacy UI change should be allowed in PR AIP-84 Modern Rest API labels Oct 31, 2024
@pierrejeambrun pierrejeambrun changed the title Migrate backfill API to fast api AIP-84 Migrate backfill API to fast api Oct 31, 2024
@pierrejeambrun pierrejeambrun mentioned this pull request Oct 31, 2024
Closed
1 task
@dstandish dstandish force-pushed the migrate-backfills-rest-api-to-fast-api branch from 022dc44 to 4fe5b77 Compare October 31, 2024 19:38
@dstandish dstandish force-pushed the migrate-backfills-rest-api-to-fast-api branch from b45a704 to 1f24643 Compare October 31, 2024 21:53
pierrejeambrun
pierrejeambrun approved these changes Nov 4, 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 Daniel

@dstandish dstandish merged commit 30147ff into apache:main Nov 4, 2024
52 checks passed
@dstandish dstandish deleted the migrate-backfills-rest-api-to-fast-api branch November 4, 2024 17:06
@potiuk
Copy link
Member

potiuk commented Nov 4, 2024

Ah...

FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py::TestListBackfills::test_should_respond_200_with_granular_dag_access - assert 404 == 200
 +  where 404 = <WrapperTestResponse streamed [404 NOT FOUND]>.status_code
FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py::TestGetBackfill::test_should_respond_200_with_granular_dag_access - assert 404 == 200
 +  where 404 = <WrapperTestResponse streamed [404 NOT FOUND]>.status_code
FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py::TestCreateBackfill::test_create_backfill - assert 404 == 200
 +  where 404 = <WrapperTestResponse streamed [404 NOT FOUND]>.status_code
FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py::TestPauseBackfill::test_should_respond_200_with_granular_dag_access - assert 404 == 200
 +  where 404 = <WrapperTestResponse streamed [404 NOT FOUND]>.status_code
FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py::TestCancelBackfill::test_should_respond_200_with_granular_dag_access - assert 404 == 200
 +  where 404 = <WrapperTestResponse streamed [404 NOT FOUND]>.status_code

Will faili now in main - see https://github.com/apache/airflow/actions/runs/11669732900/job/32492756690?pr=43556#step:7:4733

I guess those tests should never be in FAB in the first place ? Maybe we should move them to API @dstandish @pierrejeambrun

@potiuk
Copy link
Member

potiuk commented Nov 4, 2024

It's nice to see with all those changes how sometimes mixed things were :D

@bugraoz93 bugraoz93 mentioned this pull request Nov 4, 2024
Closed
@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 5, 2024 •
edited
Loading

Yep, we missed that FAB provider tests were relying on these legacy endpoints. Indeed those should not be there in the first place, Daniel opened quickly a PR to fix it, sorry for the disruption.

ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
@dstandish @ellisms
AIP-84 Migrate backfill API to fast api (apache#43496)
b0de7ca
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

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

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

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

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

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

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

Assignees

@dstandish dstandish

Labels
AIP-84 Modern Rest API legacy api Whether legacy API changes should be allowed in PR legacy ui Whether legacy UI change should be allowed in PR
Projects
AIP-84 MODERN REST API
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dstandish @potiuk @pierrejeambrun