-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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.
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
Migrate public endpoint Patch Task Instance to FastAPI #44223
base: main
Are you sure you want to change the base?
Conversation
All checks passed, PR is ready for review ✅ |
@pierrejeambrun I'm adding the Set Task Instance Note functionality to this (Patch Task Instance) API as you mentioned here. Will update this PR in a while. |
addcfe1
to
f6c3f24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to include the update_mask
as we do for other patch endpoints. (The state part would actually be the set_task_instance_state
of your other PR)
if len(note) > 1000: | ||
raise ValueError("Note length should not exceed 1000 characters.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exists natively in pydantic Field. We can use that instead of coding a check manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared the link above.
request: Request, | ||
body: PatchTaskInstanceBody, | ||
session: Annotated[Session, Depends(get_session)], | ||
map_index: int = -1, |
There was a problem hiding this 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 update mask.
You can take a look at others (not all of them are reworked yet), but patch_dag_run
might be close to what you want.
Also, if mask
is provided, None
values should not be ignored but set to None
.
|
||
dry_run: bool = True | ||
new_state: str | None = None | ||
note: str | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.pydantic.dev/latest/api/types/#pydantic.types.StringConstraints
note: str | None = None | |
note: Annotated[str, StringConstraints(max_length=1000)] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that one.
Thanks for your input @Kludex :)
if len(note) > 1000: | ||
raise ValueError("Note length should not exceed 1000 characters.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared the link above.
closes: #43753 and #43754
related: #42370
This migrates the Patch Task Instance API from
api_connexion
toapi_fastapi
.