Migrate public endpoint Clear Task Instances to FastAPI #44220
Conversation
boring-cyborg
bot
added
area:API
omkar-foss
added
legacy api
omkar-foss
requested review from
bbovenzi,
pierrejeambrun,
ryanahamilton,
jscheffl and
ephraimbuddy
as code owners
|
Resolving conflicts from recent PR merges into |
omkar-foss
force-pushed
the
aip-84/tis/post-clear-tis
branch
from
c54811d
to
f670a0a
Compare
This is done, PR synced with |
|
All checks passed, PR ready to review ✅ |
There was a problem hiding this comment.
Looking good!
A few suggestions (only #44220 (comment) requires change), we can merge after :)
| @pytest.fixture(scope="module") | ||
| def dagbag(): | ||
| from airflow.models import DagBag | ||
|
|
||
| dagbag_instance = DagBag(include_examples=True, read_dags_from_db=False) | ||
| dagbag_instance.sync_to_db() | ||
| return dagbag_instance |
There was a problem hiding this comment.
If you put the fixture at the conftest.py level, do you mind also using it in other tests, and replace.
Maybe test_dag in test_dag_sources.py can reuse this fixture:
@pytest.fixture
def test_dag(dagbag):
return dagbag.dags[TEST_DAG_ID]
There was a problem hiding this comment.
Yes this works, but if you don't mind, shall I pick this up in a separate PR? Have some other cleanup to do in tests so can do it together if that's okay.
tests/api_fastapi/core_api/routes/public/test_task_instances.py
Outdated
Show resolved
Hide resolved
tests/api_fastapi/core_api/routes/public/test_task_instances.py
Outdated
Show resolved
Hide resolved
omkar-foss
force-pushed
the
aip-84/tis/post-clear-tis
branch
from
8cd608c
to
b50bd96
Compare
Thanks @pierrejeambrun, I've made the changes as per your suggestions and resolved the corresponding conversations. Also rebased with |
| class TaskInstanceReferenceCollectionResponse(BaseModel): | ||
| """Task Instance Reference collection serializer for responses.""" | ||
|
|
||
| task_instances: list[TaskInstanceReferenceResponse] |
There was a problem hiding this comment.
For consistency with other Collection Datamodel, can you add total_entries: int as well please. (and in the response as well)
It's not here in the legacy one but I think it should be added to stay consistent with the rest of the API.
| task_id: str | ||
| dag_run_id: str = Field(validation_alias="run_id") | ||
| dag_id: str | ||
| logical_date: datetime |
There was a problem hiding this comment.
Again, I'm not sure we want or need logical_date to be here.
closes: #43751
related: #42370
This migrates the Clear Task Instances API from
api_connexiontoapi_fastapi.