You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "imrichardwu (via GitHub)" <gi...@apache.org> on 2026/04/08 20:50:14 UTC
[PR] Add name and description fields to deadline alerts [airflow]
imrichardwu opened a new pull request, #64926:
URL: https://github.com/apache/airflow/pull/64926
Adds name and description fields to DeadlineAlert, exposed through the API
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to deadline alerts [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3073134711
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Are you using this? Because if we do not adjust permissions associated to this, this won't be useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3075062867
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Just checking, but we should remove it for this pr?
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Just confirming, but we should remove it for this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3075062867
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Just wondering, is the reason for doing it later to prevent bloat, in case I do not end up working on the deadline page task?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #64926:
URL: https://github.com/apache/airflow/pull/64926
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3081705208
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
Seems odd to drop, but I won't block over it. As you said, it's new and should be safe and we can always add it later if users ask for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4240749759
> I'm not sure I follow. If the user wants to set a description, why not let them? Let's say we work for Big Company. The person in Marketing who is using the deadline may not be the dev/admin person who wrote the code for the deadline. Having a description seems reasonable to me.
Having interval + reference and name should provide enough detail about the deadline without the ui feeling bloated. Because I did add description before and it felt like there was too much info and a bit confusing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077386056
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I see, thanks for clarifying that.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
If it didnt make it in a release, I am ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name and description fields to deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3059139303
##########
providers/common/ai/src/airflow/providers/common/ai/plugins/www/__init__.py:
##########
Review Comment:
came from the pre commit hook. I'm not too sure why
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077382158
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I added this in aprevious pr. No one should have used the api yet, so it should be ok. I initially decided to add a description because I thought about what @ferruzzi was thinking, but after talking to @bbovenzi in my ui pr, we decided to drop it. If I made any mistake in my judgement @bbovenzi please correct me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3085247688
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
It's because if we add this it requires code change in providers (auth managers) it's better to have all that in it's own PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3090240161
##########
airflow-core/src/airflow/serialization/encoders.py:
##########
@@ -211,6 +211,7 @@ def encode_deadline_alert(d: DeadlineAlert | SerializedDeadlineAlert) -> dict[st
from airflow.sdk.serde import serialize
return {
+ "name": getattr(d, "name", None),
Review Comment:
Nit: I think this works how you have it (presumably you've tested this manually?) but based on the other values in this dict,, I'm pretty sure this can/should be
```suggestion
"name": d.name,
```
Also, not for this PR but I suppose we could have used the DeadlineAlertFields.NAME, etc constants for the keys here like we did in the decoder to make sure they stay in sync. But that's not your problem for this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077382158
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I added this in aprevious pr. No one should have used the api yet, so it should be ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to deadline alerts [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3066490404
##########
airflow-core/src/airflow/models/serialized_dag.py:
##########
@@ -577,8 +578,17 @@ def write_dag(
if deadline_uuid_mapping is not None:
# All deadlines matched — reuse the UUIDs to preserve hash.
- # Clear the mapping since the alert rows already exist in the DB;
- # no need to delete and recreate identical records.
+ # Update name in case it changed (it doesn't affect the definition
+ # match or the hash, so existing rows won't be recreated, but it
+ # must stay current in the DB).
+ for uuid_str, deadline_data in deadline_uuid_mapping.items():
+ session.execute(
+ update(DeadlineAlertModel)
+ .where(DeadlineAlertModel.id == UUID(uuid_str))
+ .values(
+ name=deadline_data.get(DeadlineAlertFields.NAME),
+ )
+ )
dag.data["dag"]["deadline"] = existing_deadline_uuids
Review Comment:
This block executes `UPDATE deadline_alert SET name=...` even when the serialized DAG hash is unchanged, but `write_dag()` can still return `False` later (meaning "not written"). That makes the return value misleading and can hide that a DB write occurred; consider tracking whether any name updates were applied and returning `True` (or adjusting the control flow/docstring accordingly).
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +50,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
reference_type: str = Field(validation_alias=AliasPath("reference", "reference_type"))
interval: float = Field(description="Interval in seconds between deadline evaluations.")
Review Comment:
PR description says the API will expose both `name` and `description` on `DeadlineAlert`, but this change removes `description` from the UI API response models (and corresponding OpenAPI). Either update the PR description to match the actual behavior, or reintroduce `description` in the response models/OpenAPI if it should remain supported.
##########
airflow-core/src/airflow/models/serialized_dag.py:
##########
@@ -577,8 +578,17 @@ def write_dag(
if deadline_uuid_mapping is not None:
# All deadlines matched — reuse the UUIDs to preserve hash.
- # Clear the mapping since the alert rows already exist in the DB;
- # no need to delete and recreate identical records.
+ # Update name in case it changed (it doesn't affect the definition
+ # match or the hash, so existing rows won't be recreated, but it
+ # must stay current in the DB).
+ for uuid_str, deadline_data in deadline_uuid_mapping.items():
+ session.execute(
+ update(DeadlineAlertModel)
+ .where(DeadlineAlertModel.id == UUID(uuid_str))
+ .values(
+ name=deadline_data.get(DeadlineAlertFields.NAME),
+ )
+ )
Review Comment:
New behavior: when deadline UUIDs are reused, `write_dag()` now updates `DeadlineAlertModel.name` even if the serialized DAG hash doesn't change. There doesn't appear to be a unit test covering this path (e.g., name changes while reference/interval/callback stay the same), so a regression test should be added to ensure the DB row is updated and the returned value/side-effects are as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3090312924
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I commented below as well, but I'll let it go. I'm not totally sold, but we can always add it back in if the users want it. Maybe a good Name is enough and nobody will miss it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4282696267
Nice, congrats
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3073625568
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Yes this should only be included once we have a deadlines page in the browse tab
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4239657485
Why are we removing description? I think other than that it looks fine from my side
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4280907958
### Backport failed to create: v3-2-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/24667586192'> Run details </a>
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>❌</td>
<td>v3-2-test</td>
<td><a href="https://github.com/apache/airflow/commit/e9d106667c7fe98f77c1312bb082c010b472c388"><img src='https://img.shields.io/badge/Commit-e9d1066-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
cherry_picker e9d1066 v3-2-test
```
This should apply the commit to the v3-2-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.
After you have resolved the conflicts, you can continue the backport process by running:
```bash
cherry_picker --continue
```
If you don't have cherry-picker installed, see the [installation guide](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#how-to-backport-pr-with-cherry-picker-cli).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4289415028
Manual backport https://github.com/apache/airflow/pull/65601
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077353450
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
Why are we dropping this? This will qualify as a breaking change for clients consuming this rest API since it was sent across in Airflow <= 3.2 API response?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3075062867
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Just wondering, is it better to do it later, to prevent bloat, let's say if I didn't end up doing the deadline page task?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077382158
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I added this in aprevious pr. No one should have used the api yet, so it should be ok. I initially decided to add a description because I thought about what @ferruzzi was thinking, but after talking to @bbovenzi in my ui pr, we decided to drop it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077393588
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
If 3.2.1 hasn't been released yet it should be good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3077382158
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I added this in aprevious pr. No one should have used the api yet, so it should be ok. I initially decided to add a description because I thought the same thing on what @ferruzzi was saying above,_ but after talking to @bbovenzi in my ui pr, we decided to drop it. If I made any mistake in my judgement @bbovenzi please correct me.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
I added this in aprevious pr. No one should have used the api yet, so it should be ok. I initially decided to add a description because I thought the same thing on what @ferruzzi was saying above, but after talking to @bbovenzi in my ui pr, we decided to drop it. If I made any mistake in my judgement @bbovenzi please correct me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4245885787
@ferruzzi @amoghrajesh I told Richard that we could ignore descriptions in favor of a good name + a human readable interval+reference string. But since 3.2.1 is about to be released and there could still be a reason for a description then I am ok with putting description back. My apologies for adding extra confusion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3081265483
##########
airflow-core/src/airflow/api_fastapi/common/types.py:
##########
@@ -120,6 +120,7 @@ class MenuItem(Enum):
CONFIG = "Config"
CONNECTIONS = "Connections"
DAGS = "Dags"
+ DEADLINES = "Deadlines"
Review Comment:
Yeah, its better to keep PRs narrowly scoped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name and description fields to deadline alerts [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3059102489
##########
providers/common/ai/src/airflow/providers/common/ai/plugins/www/__init__.py:
##########
Review Comment:
Why is this here?
##########
airflow-core/src/airflow/ui/public/i18n/locales/en/browse.json:
##########
Review Comment:
These i18n json changes shouldn't be in this PR.
##########
airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts:
##########
Review Comment:
We don't have a Deadlines browse tab in this PR so we shouldn't make this change either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4240636778
I'm not sure I follow. If the user wants to set a description, why not let them? Let's say we work for Big Company. The person in Marketing who is using the deadline may not be the dev/admin person who wrote the code for the deadline. Having a description seems reasonable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "imrichardwu (via GitHub)" <gi...@apache.org>.
imrichardwu commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4239754533
> Why are we removing description? I think other than that it looks fine from my side
We are removing description because we decided that we should not rely on description in telling the users what the deadline and deadline alerts are.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #64926:
URL: https://github.com/apache/airflow/pull/64926#discussion_r3085264718
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/deadline.py:
##########
@@ -53,7 +51,6 @@ class DeadlineAlertResponse(BaseModel):
id: UUID
name: str | None = None
- description: str | None = None
Review Comment:
Also deadlines API is still private `/ui` so no breaking change there. This leave us time for it to stabilize before we actually move it to public API. (It was too early to freeze the API and be like "this is the definite deadline API" cause things were still in construction)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add name fields to SDK deadline alerts [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on PR #64926:
URL: https://github.com/apache/airflow/pull/64926#issuecomment-4256668219
Alight, let's give it a try. Worst case, we can always add a Description field back later if users ask for it, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org