Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2822254141 Right. Let's close this one then. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi closed pull request #47821: AIP-38 :: created new dag warning banner added to dag details page URL: https://github.com/apache/airflow/pull/47821 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
aritra24 commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2821988752 @bbovenzi it made it in as part of #49236 I couldn't push to this branch so I'd opened a new branch off of this one -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2821980554 Didn't make it for 3.0 but let's get it for 3.0.1. Can @tyrellcurry or @aritra24 rebase please? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
pierrejeambrun commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2800947560 @aritra24 I think it just need a little bit of adjustment and it would be great to push this forward. Unfortunately I don't think you'll be able to push directly to this branch, but feel free to branch out from @tyrellcurry and commit the few adjustements needed, if we have an RC3 this might even make it to 3.0 release. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
aritra24 commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2789639732 Hey @tyrellcurry were you able to get around to this? If you think you might be busy I can take it forward to closure? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
pierrejeambrun commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r2000563084
##
airflow/ui/src/pages/Dag/Dag.tsx:
##
@@ -51,6 +55,10 @@ export const Dag = () => {
dagId,
});
+ const { data: warningData, isLoading: isWarningLoading } =
useDagWarningServiceListDagWarnings({
+dagId,
+ });
+
Review Comment:
Maybe you should pass down the `dagId` to `DetailsLayout` and let the
warnings be fetched there.
I don't think there is a reason for fetching the warnings here, we will not
display them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2742289413 > > @bbovenzi I'm happy to rework this to that new notification style. Where would we want it? Are you saying have the icon next to the name in list view? Or have the icon next to the name once you've opened the DAG in the DAG Details view? > > If you don't mind sending me a quick screenshot for placement, that would be super helpful! > > Yes, although actually, let's keep your idea for now for the sake of time. And just put the warnings in an accordion so they can be collapsed. Hey @bbovenzi, what do you think of this: When `warnings` or `errors` are present, it will show this expandable accordion:  When you expand, it looks like this:  And if there are many warnings/errors you can scroll through them, otherwise they would be cut-off in the UI:  Let me know what you think -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
pierrejeambrun commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2747775211 Great to hear @tyrellcurry, thanks for the update :) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2746886073 Quick update: got a modal version of this in the works, going to need a little more time. Will try to get a commit for review by mid week. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2744248430 Sorry about the rebase chaos earlier @bbovenzi & @pierrejeambrun 😅. I did a reset with a cherry-pick, should be good to go now. I'll apply your suggestions with a modal pop up that also changes the icon/color and text based on warnings and/or errors present. Should have something ready by end of this weekend -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi commented on PR #47821:
URL: https://github.com/apache/airflow/pull/47821#issuecomment-2743802575
First, looks like we need to do a big rebase.
Second, I like the icon but I think it should be the trigger for a modal.
Thats not great how much it pushed the page content down. Let's try to be more
descriptive than "Issues" at least give a "{X} Warnings"?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2735104757 > @bbovenzi I'm happy to rework this to that new notification style. Where would we want it? Are you saying have the icon next to the name in list view? Or have the icon next to the name once you've opened the DAG in the DAG Details view? > > If you don't mind sending me a quick screenshot for placement, that would be super helpful! Yes, although actually, let's keep your idea for now for the sake of time. And just put the warnings in an accordion so they can be collapsed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2735103360 > @bbovenzi I'm happy to rework this to that new notification style. Where would we want it? Are you saying have the icon next to the name in list view? Or have the icon next to the name once you've opened the DAG in the DAG Details view? > > If you don't mind sending me a quick screenshot for placement, that would be super helpful! This list view already has an import error icon next. That's where the screenshot was from. Next to the name once youved open the Dag. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2735093529 > > Also, just realized that those warnings are not part of the 'scrollable' area. So if we get too many of them I think it will just push out of the screen the rest of the details pane without having the ability to scroll down. > > Let's match how our list of Dag Import Errors works: https://private-user-images.githubusercontent.com/4600967/424096060-bacdefca-3839-4f2e-aaab-b136d9fa1df7.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDIzMTg2NjYsIm5iZiI6MTc0MjMxODM2NiwicGF0aCI6Ii80NjAwOTY3LzQyNDA5NjA2MC1iYWNkZWZjYS0zODM5LTRmMmUtYWFhYi1iMTM2ZDlmYTFkZjcucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDMxOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAzMThUMTcxOTI2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9YWJlZmE4ZWExZGY2ZTI1YzVhZGQ5YWRhZDlhMzk4ZDVkZTYxMjdiZDA0OThhOGU1Y2Q1ODg0ZjE4MmJmZTA1YyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.dmWY1-yuoXagVvuACjfB-IurAExpxRKyD50T0SM0BgY";> > > Instead of a banner, we show warning or error icons next to the dag_display_name. And clicking on it should open a modal with more info. Especially with warnings, sometimes a user doesnt want it to disrupt the whole details panel @bbovenzi I'm happy to rework this to that new notification style. Where would we want it? Are you saying have the icon next to the name in list view? Or have the icon next to the name once you've opened the DAG in the DAG Details view? If you don't mind sending me a quick screenshot for placement, that would be super helpful! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
bbovenzi commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2734024899 > Also, just realized that those warnings are not part of the 'scrollable' area. So if we get too many of them I think it will just push out of the screen the rest of the details pane without having the ability to scroll down. Let's match how our list of Dag Import Errors works: https://github.com/user-attachments/assets/bacdefca-3839-4f2e-aaab-b136d9fa1df7"; /> Instead of a banner, we show warning or error icons next to the dag_display_name. And clicking on it should open a modal with more info. Especially with warnings, sometimes a user doesnt want it to disrupt the whole details panel -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
pierrejeambrun commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r2000560286
##
airflow/ui/src/pages/Dag/Dag.tsx:
##
@@ -75,8 +83,9 @@ export const Dag = () => {
{
dagId,
});
+ const { data: warningData, isLoading: isWarningLoading } =
useDagWarningServiceListDagWarnings({
+dagId,
+ });
+
Review Comment:
Maybe you should pass down the `dagId` to `DetailsLayout` and the warning be
fetched there.
I don't think there is a reason for fetching the warnings there, we will not
display them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997664541
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -89,7 +101,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
{children}
-
+
+
+{warning?.dag_warnings.map((warningItem) => (
+
Review Comment:
Ah right, thanks for catching that. I saw our linting rules does not allow
us to use index anywhere in the key, so how do you feel about timestamp? eg.
`asset_name_uri-2025-03-16T16:32:41.965952Z`
Since it's down to the microsecond, we should be okay for uniqueness.
It's using the `timestamp` property. This is me assuming it updates relative
to each warning generated.
I'll do a push with this change to review
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997664541
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -89,7 +101,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
{children}
-
+
+
+{warning?.dag_warnings.map((warningItem) => (
+
Review Comment:
Ah right, thanks for catching that. I saw our linting rules does not allow
us to use index anywhere in the key, so how do you feel about timestamp? eg.
`asset_name_uri-2025-03-16T16:32:41.965952Z`
Since it's down to the microsecond, we should be okay for uniqueness.
It's using the `timestamp` property. This is me assuming it updates relative
to each warning generated in the array.
I'll do a push with this change to review
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997664541
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -89,7 +101,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
{children}
-
+
+
+{warning?.dag_warnings.map((warningItem) => (
+
Review Comment:
Ah right, thanks for catching that. I saw our linting rules does not allow
us to use index anywhere in the key, so how do you feel about timestamp? eg.
`asset_name_uri-2025-03-16T16:32:41.965952Z`
Since it's down to the microsecond, we should be okay for uniqueness.
I'll do a push with this change to review
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
aritra24 commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997659098
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -89,7 +101,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
{children}
-
+
+
+{warning?.dag_warnings.map((warningItem) => (
+
Review Comment:
The key is required to be
[unique](https://react.dev/learn/rendering-lists#rules-of-keys) between peers
which is used during rerendering
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997657489
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -39,14 +40,25 @@ import { Grid } from "./Grid";
import { NavTabs } from "./NavTabs";
import { PanelButtons } from "./PanelButtons";
+type DagWarningItem = {
+ dag_id: string;
+ message: string;
+};
+
+type DagWarningsResponse = {
+ dag_warnings: Array;
+ total_entries: number;
+};
+
type Props = {
readonly dag?: DAGResponse;
readonly error?: unknown;
readonly isLoading?: boolean;
readonly tabs: Array<{ icon: ReactNode; label: string; value: string }>;
+ readonly warning?: DagWarningsResponse;
Review Comment:
@aritra24 thanks for catching this, I was looking for that type and couldn't
find 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
tyrellcurry commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997654767
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -89,7 +101,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
{children}
-
+
+
+{warning?.dag_warnings.map((warningItem) => (
+
Review Comment:
@aritra24 sorry I am not quite sure what you're asking, could you rephrase?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
aritra24 commented on code in PR #47821:
URL: https://github.com/apache/airflow/pull/47821#discussion_r1997641362
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -89,7 +101,12 @@ export const DetailsLayout = ({ children, error, isLoading,
tabs }: Props) => {
{children}
-
+
+
+{warning?.dag_warnings.map((warningItem) => (
+
Review Comment:
Since all the warnings come from the same dag I presume this would be the
same for all alerts thus throwing an error?
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -39,14 +40,25 @@ import { Grid } from "./Grid";
import { NavTabs } from "./NavTabs";
import { PanelButtons } from "./PanelButtons";
+type DagWarningItem = {
+ dag_id: string;
+ message: string;
+};
+
+type DagWarningsResponse = {
+ dag_warnings: Array;
+ total_entries: number;
+};
+
type Props = {
readonly dag?: DAGResponse;
readonly error?: unknown;
readonly isLoading?: boolean;
readonly tabs: Array<{ icon: ReactNode; label: string; value: string }>;
+ readonly warning?: DagWarningsResponse;
Review Comment:
This can be of the type `DAGWarningCollectionResponse` which is the response
type of `useDagWarningServiceListDagWarnings`
##
airflow/ui/src/layouts/Details/DetailsLayout.tsx:
##
@@ -39,14 +40,25 @@ import { Grid } from "./Grid";
import { NavTabs } from "./NavTabs";
import { PanelButtons } from "./PanelButtons";
+type DagWarningItem = {
+ dag_id: string;
+ message: string;
+};
+
+type DagWarningsResponse = {
+ dag_warnings: Array;
+ total_entries: number;
+};
+
Review Comment:
I don't this this needs to be defined.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] AIP-38 :: created new dag warning banner added to dag details page [airflow]
boring-cyborg[bot] commented on PR #47821: URL: https://github.com/apache/airflow/pull/47821#issuecomment-2727210165 Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-pre-commit-hooks) will help you with that. - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations. - Be patient and persistent. It might take some time to get a review or get the final approval from Committers. - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack. - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
