Skip to content

AIP-84 Migrate backfill API to fast api#43496

Merged
dstandish merged 5 commits intoapache:mainfrom
astronomer:migrate-backfills-rest-api-to-fast-api
Nov 4, 2024
Merged

AIP-84 Migrate backfill API to fast api#43496
dstandish merged 5 commits intoapache:mainfrom
astronomer:migrate-backfills-rest-api-to-fast-api

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

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

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@jscheffl jscheffl Awaiting requested review from jscheffl

@ashb ashb Awaiting requested review from ashb

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

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

No open 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