AIP-84: Migrate get_log endpoint #44238
Conversation
utkarsharma2
requested review from
bbovenzi,
pierrejeambrun,
ryanahamilton,
jscheffl and
ephraimbuddy
as code owners
utkarsharma2
force-pushed
the
migrate_log_endpoint
branch
from
5d29904
to
6996c71
Compare
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
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
tests/api_fastapi/core_api/routes/public/test_task_instances.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Pierre Jeambrun <[email protected]>
Co-authored-by: Pierre Jeambrun <[email protected]>
Co-authored-by: Pierre Jeambrun <[email protected]>
Co-authored-by: Pierre Jeambrun <[email protected]>
Co-authored-by: Kalyan R <[email protected]>
| dag_id: str, | ||
| dag_run_id: str, | ||
| task_id: str, | ||
| task_try_number: int, |
There was a problem hiding this comment.
| task_try_number: int, | |
| task_try_number: NonNegativeInt, |
There was a problem hiding this comment.
we can import this from pydantic
from pydantic import NonNegativeInt
There was a problem hiding this comment.
This will automatically raise 422. We don't need to separately check if its less than 0
There was a problem hiding this comment.
try_number: int = 1
Let's add a default try number value. and just try_number is sufficient. There is no need for the task_ prefix
There was a problem hiding this comment.
I forgot that try_number can be >0. In that case, we should use PositiveInt
| if task_try_number <= 0: | ||
| raise HTTPException(status.HTTP_400_BAD_REQUEST, "task_try_number must be a positive integer") |
There was a problem hiding this comment.
| if task_try_number <= 0: | |
| raise HTTPException(status.HTTP_400_BAD_REQUEST, "task_try_number must be a positive integer") |
There was a problem hiding this comment.
Minor nitpick, after addressing @rawwar suggestions, should be good to merge.
|
|
||
| @task_instances_log_router.get( | ||
| "/{task_id}/logs/{task_try_number}", | ||
| responses=create_openapi_http_exception_doc([status.HTTP_404_NOT_FOUND]), |
There was a problem hiding this comment.
we are missing the documentation for text/plain mimetype.
Related: #42370