You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/03/09 16:03:46 UTC
[GitHub] [airflow] bbovenzi opened a new pull request #22123: Add details drawer to Grid View
bbovenzi opened a new pull request #22123:
URL: https://github.com/apache/airflow/pull/22123
Add a details drawer to the right side of the grid view to quickly see details and actions for the DAG / Dag Run / or Task Instance.
Closes #19938
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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
[GitHub] [airflow] uranusjr commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1080512305
I added a commit to fix the KeyError. There are still a couple of failures relating to argument count that need fixing, I’ll leave those to you.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r829364328
##########
File path: airflow/www/static/js/dag.js
##########
@@ -227,22 +227,6 @@ export function callModal(t, d, extraLinks, tryNumbers, sd, drID) {
}
}
-export function callModalDag(dag) {
Review comment:
We only called this in the grid view, so we can fully replace 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
[GitHub] [airflow] norm commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084273120
I can't approve except in words, but I haven't found any further flaws in this, code or UI.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837064176
##########
File path: airflow/www/static/js/tree/ResetRoot.jsx
##########
@@ -0,0 +1,55 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Button, Link } from '@chakra-ui/react';
+
+import { getMetaValue } from '../utils';
+
+const dagId = getMetaValue('dag_id');
+const numRuns = getMetaValue('num_runs');
+const urlRoot = getMetaValue('root');
+const baseDate = getMetaValue('base_date');
+
+const ResetRoot = () => {
+ let rootLink;
+ if (urlRoot) {
+ const params = new URLSearchParams();
+ if (numRuns) params.set('num_runs', numRuns);
+ if (baseDate) params.set('base_date', baseDate);
+ rootLink = `/dags/${dagId}/grid?${params.toString()}`;
Review comment:
We can't hard code the URL like this - airflow can be rooted/deployed to a location other than `/`.
In other places we put the URL in a meta tag from the jinja templates for this purpose.
##########
File path: airflow/www/static/js/tree/api/useClearTask.js
##########
@@ -0,0 +1,77 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import axios from 'axios';
+import { useToast } from '@chakra-ui/react';
+import { useMutation, useQueryClient } from 'react-query';
+import { getMetaValue } from '../../utils';
+import { useAutoRefresh } from '../providers/autorefresh';
+
+export default function useClearTask({
+ dagId, runId, taskId, executionDate,
+}) {
+ const queryClient = useQueryClient();
+ const toast = useToast();
+ const { startRefresh } = useAutoRefresh();
+
+ return useMutation(
+ ['clearTask', dagId, runId, taskId],
+ ({
+ past, future, upstream, downstream, recursive, failed, confirmed,
+ }) => {
+ const csrfToken = getMetaValue('csrf_token');
+ const params = new URLSearchParams({
+ csrf_token: csrfToken,
+ dag_id: dagId,
+ dag_run_id: runId,
+ task_id: taskId,
+ confirmed,
+ execution_date: executionDate,
+ past,
+ future,
+ upstream,
+ downstream,
+ recursive,
+ only_failed: failed,
+ }).toString();
+
+ return axios.post('/clear', params, {
Review comment:
Same comment here about hard-coding URLs
##########
File path: airflow/www/static/js/tree/api/index.js
##########
@@ -0,0 +1,57 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import axios from 'axios';
+import camelcaseKeys from 'camelcase-keys';
+
+import useDag from './useDag';
+import useTasks from './useTasks';
+import useClearRun from './useClearRun';
+import useQueueRun from './useQueueRun';
+import useMarkFailedRun from './useMarkFailedRun';
+import useMarkSuccessRun from './useMarkSuccessRun';
+import useRunTask from './useRunTask';
+import useClearTask from './useClearTask';
+import useMarkFailedTask from './useMarkFailedTask';
+import useMarkSuccessTask from './useMarkSuccessTask';
+import useExtraLinks from './useExtraLinks';
+import useConfirmMarkTask from './useConfirmMarkTask';
+import useTreeData from './useTreeData';
+
+axios.interceptors.response.use(
+ (res) => (res.data ? camelcaseKeys(res.data, { deep: true }) : res),
+);
+
+axios.defaults.headers.common.Accept = 'application/json';
+
+export {
+ useDag,
Review comment:
Why the `use` prefix on all these functions/nodules? It looks slightly odd 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
[GitHub] [airflow] uranusjr commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r833881233
##########
File path: airflow/www/views.py
##########
@@ -1718,6 +1730,7 @@ def run(self, session=None):
dag_run_id = request.form.get('dag_run_id')
map_index = request.args.get('map_index', -1, type=int)
origin = get_safe_url(request.form.get('origin'))
+ return_as_json = request.form.get('return_as_json') == "true"
Review comment:
I wonder if it’s better to use HTTP header for this. We could use `request.content_type` (automatically return JSON for `application/json` and redirect otherwise), or the conventional `X-Requested-With`.
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1079137769
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r832597881
##########
File path: airflow/www/static/js/tree/details/content/taskInstance/taskActions/Clear.jsx
##########
@@ -0,0 +1,127 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Button,
+ Flex,
+ ButtonGroup,
+ useDisclosure,
+} from '@chakra-ui/react';
+
+import ActionButton from './ActionButton';
+import ConfirmDialog from '../../ConfirmDialog';
+import { useClearTask, useConfirmClearTask } from '../../../../api';
+
+const Run = ({
+ dagId,
+ runId,
+ taskId,
+ executionDate,
+}) => {
+ const [affectedTasks, setAffectedTasks] = useState([]);
+
+ // Options check/unchecked
+ const { isOpen: past, onToggle: onTogglePast } = useDisclosure();
Review comment:
Fair enough. Updated to `useState(false)`
--
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
[GitHub] [airflow] eladkal edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1073341799
Played with it a bit in local env this is **really really good!**
Have a few suggestions/bugs **all are minor**:
1. [Suggestion] Can we have the ability to easily copy to clipboard?
To give example of what i had in mind:
2. [Tweak] Need to fix the name to Grid also in:
3. [Bug] When changing Timezone there is a weird "jump" of values :
Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?
4. [Bug] When opening the panel a fresh task (state None) when the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
The values in the panel should be refreshed when with the auto-refresh property.
(Edit: noticed also the status left as non-status though it's Success and probably duration is also not updated.. so I guess the whole panel isn't being updated with the auto refresh functionality)
5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?
6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.
7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and vise versia. The tool tip is redundant any anyway isn't 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
[GitHub] [airflow] uranusjr commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r834883768
##########
File path: airflow/www/views.py
##########
@@ -527,6 +527,21 @@ def get_task_stats_from_query(qry):
return data
+def redirect_or_json(origin, return_as_json, msg, status=""):
Review comment:
I think we can remove `return_as_json` entirely and check `request.headers['Accept'] == 'application/json'` in this function directly
--
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
[GitHub] [airflow] eladkal commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1074291785
Great. Since I'm not familiar with the UI side of the code I can't do a proper code review but I can confirm that functionality wise it work 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
[GitHub] [airflow] potiuk commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084678112
> Apparently I _can_ approve it :)
Of course. and you are encouraged to. It does not change the mergability status though if you are not commiter :)
--
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
[GitHub] [airflow] ashb commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1071466305
> 2. I think in the confirm pop-ups we should default the confirm button
What did the current confirm page default to?
--
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
[GitHub] [airflow] ryanahamilton commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r832308970
##########
File path: airflow/www/static/js/tree/StatusBox.jsx
##########
@@ -24,55 +24,76 @@ import { isEqual } from 'lodash';
import {
Box,
Tooltip,
+ useTheme,
} from '@chakra-ui/react';
-import { callModal } from '../dag';
import InstanceTooltip from './InstanceTooltip';
+import { useContainerRef } from './providers/containerRef';
+import { useSelection } from './providers/selection';
export const boxSize = 10;
export const boxSizePx = `${boxSize}px`;
+export const SimpleStatus = ({ state, ...rest }) => (
+ <Box
+ width={boxSizePx}
+ height={boxSizePx}
+ backgroundColor={stateColors[state] || 'white'}
+ borderRadius="2px"
+ borderWidth={state ? 0 : 1}
+ {...rest}
+ />
+);
+
const StatusBox = ({
- group, instance, containerRef, extraLinks = [],
+ group, instance,
}) => {
- const {
- executionDate, taskId, tryNumber = 0, operator, runId, mapIndex,
- } = instance;
- const onClick = () => executionDate && callModal(taskId, executionDate, extraLinks, tryNumber, operator === 'SubDagOperator', runId, mapIndex);
+ const containerRef = useContainerRef();
+ const { selected, onSelect } = useSelection();
+ const { runId, taskId } = instance;
+ const { colors } = useTheme();
+ const hoverBlue = `${colors.blue[100]}50`;
// Fetch the corresponding column element and set its background color when hovering
const onMouseEnter = () => {
- [...containerRef.current.getElementsByClassName(`js-${runId}`)]
- .forEach((e) => { e.style.backgroundColor = 'rgba(113, 128, 150, 0.1)'; });
+ if (selected.runId !== runId) {
+ [...containerRef.current.getElementsByClassName(`js-${runId}`)]
+ .forEach((e) => { e.style.backgroundColor = hoverBlue; });
+ }
};
const onMouseLeave = () => {
[...containerRef.current.getElementsByClassName(`js-${runId}`)]
.forEach((e) => { e.style.backgroundColor = null; });
};
+ const onClick = () => {
+ onMouseLeave();
+ onSelect({ taskId, runId });
+ };
+
return (
- <Tooltip
- label={<InstanceTooltip instance={instance} group={group} />}
- fontSize="md"
- portalProps={{ containerRef }}
- hasArrow
- placement="top"
- openDelay={400}
- >
- <Box
- width={boxSizePx}
- height={boxSizePx}
- backgroundColor={stateColors[instance.state] || 'white'}
- borderRadius="2px"
- borderWidth={instance.state ? 0 : 1}
- onMouseEnter={onMouseEnter}
- onMouseLeave={onMouseLeave}
- onClick={onClick}
- zIndex={1}
- cursor={!group.children && 'pointer'}
- data-testid="task-instance"
- />
- </Tooltip>
+ <>
Review comment:
Why the added fragment? Doesn't appear necessary given `<Tooltip />` is the only child.
##########
File path: airflow/www/static/js/tree/details/content/Dag.jsx
##########
@@ -0,0 +1,137 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* global moment */
+
+import React from 'react';
+import {
+ Table,
+ Tbody,
+ Tr,
+ Td,
+ Tag,
+ Text,
+ Code,
+ Link,
+ Box,
+ Button,
+ Flex,
+} from '@chakra-ui/react';
+
+import { formatDuration } from '../../../datetime_utils';
+import { getMetaValue } from '../../../utils';
+import { useDag, useTasks } from '../../api';
+import Time from '../../Time';
+
+const dagId = getMetaValue('dag_id');
+
+const Dag = () => {
+ const { data: dag } = useDag(dagId);
+ const { data: taskData } = useTasks(dagId);
+ if (!dag || !taskData) return null;
+ const { tasks = [], totalEntries = '' } = taskData;
+ const {
+ description, tags, fileloc, owners, catchup, startDate, timezone, dagRunTimeout,
+ } = dag;
+
+ // Build a key/value object of operator counts, the name is hidden inside of t.classRef.className
+ const operators = {};
+ tasks.forEach((t) => {
+ if (!operators[t.classRef.className]) {
+ operators[t.classRef.className] = 1;
+ } else {
+ operators[t.classRef.className] += 1;
+ }
+ });
+
+ return (
+ <Box>
Review comment:
Is this `Box` doing anything or could it be a fragment instead?
##########
File path: airflow/www/static/js/tree/details/content/dagRun/ClearRun.jsx
##########
@@ -0,0 +1,65 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import { Button, useDisclosure } from '@chakra-ui/react';
+
+import { useClearRun } from '../../../api';
+import ConfirmDialog from '../ConfirmDialog';
+
+const ClearRun = ({ dagId, runId }) => {
+ const [affectedTasks, setAffectedTasks] = useState([]);
+ const { isOpen, onOpen, onClose } = useDisclosure();
+ const { mutateAsync: onClear, isLoading } = useClearRun(dagId, runId);
+
+ const onClick = async () => {
+ try {
+ const data = await onClear({ confirmed: false });
+ setAffectedTasks(data);
+ onOpen();
+ } catch (e) {
+ console.error(e);
+ }
+ };
+
+ const onConfirm = async () => {
+ try {
+ await onClear({ confirmed: true });
+ setAffectedTasks([]);
+ onClose();
+ } catch (e) {
+ console.error(e);
+ }
+ };
+
+ return (
+ <>
+ <Button onClick={onClick} isLoading={isLoading}>Clear existing tasks</Button>
+ <ConfirmDialog
+ isOpen={isOpen}
+ onClose={onClose}
+ onConfirm={onConfirm}
+ description="Here's the list of task instances you are about to clear:"
Review comment:
I didn't see this copy in the PR GIF, but even without the context, it seems like the copy could be simplified:
```suggestion
description="Task instances you are about to clear:"
```
##########
File path: airflow/www/static/js/tree/details/content/taskInstance/ExtraLinks.jsx
##########
@@ -0,0 +1,64 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import {
+ Button,
+ Flex,
+ Link,
+ Divider,
+} from '@chakra-ui/react';
+
+import { useExtraLinks } from '../../../api';
+
+const ExtraLinks = ({
+ dagId,
+ taskId,
+ executionDate,
+ extraLinks = [],
+}) => {
+ const { data: links = [] } = useExtraLinks({
+ dagId, taskId, executionDate, extraLinks,
+ });
+
+ if (!links.length) return null;
+ const checkIfExternal = (url) => /^(?:[a-z]+:)?\/\//.test(url);
Review comment:
Nit, the function naming could make it more explicit that returns a boolean
```suggestion
const isExternal = (url) => /^(?:[a-z]+:)?\/\//.test(url);
```
##########
File path: airflow/www/static/js/tree/details/content/taskInstance/taskActions/Clear.jsx
##########
@@ -0,0 +1,127 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import {
+ Button,
+ Flex,
+ ButtonGroup,
+ useDisclosure,
+} from '@chakra-ui/react';
+
+import ActionButton from './ActionButton';
+import ConfirmDialog from '../../ConfirmDialog';
+import { useClearTask, useConfirmClearTask } from '../../../../api';
+
+const Run = ({
+ dagId,
+ runId,
+ taskId,
+ executionDate,
+}) => {
+ const [affectedTasks, setAffectedTasks] = useState([]);
+
+ // Options check/unchecked
+ const { isOpen: past, onToggle: onTogglePast } = useDisclosure();
Review comment:
Seems a little unorthodox to use `useDisclosure` to manage these boolean state values, no?
##########
File path: airflow/www/static/js/tree/StatusBox.jsx
##########
@@ -24,55 +24,76 @@ import { isEqual } from 'lodash';
import {
Box,
Tooltip,
+ useTheme,
} from '@chakra-ui/react';
-import { callModal } from '../dag';
import InstanceTooltip from './InstanceTooltip';
+import { useContainerRef } from './providers/containerRef';
+import { useSelection } from './providers/selection';
export const boxSize = 10;
export const boxSizePx = `${boxSize}px`;
+export const SimpleStatus = ({ state, ...rest }) => (
+ <Box
+ width={boxSizePx}
+ height={boxSizePx}
+ backgroundColor={stateColors[state] || 'white'}
+ borderRadius="2px"
+ borderWidth={state ? 0 : 1}
+ {...rest}
+ />
+);
+
const StatusBox = ({
- group, instance, containerRef, extraLinks = [],
+ group, instance,
}) => {
- const {
- executionDate, taskId, tryNumber = 0, operator, runId, mapIndex,
- } = instance;
- const onClick = () => executionDate && callModal(taskId, executionDate, extraLinks, tryNumber, operator === 'SubDagOperator', runId, mapIndex);
+ const containerRef = useContainerRef();
+ const { selected, onSelect } = useSelection();
+ const { runId, taskId } = instance;
+ const { colors } = useTheme();
+ const hoverBlue = `${colors.blue[100]}50`;
Review comment:
What's this translating to? Is it just suffixing the `50` to the hex value? e.g. `#BEE3F850`?
##########
File path: airflow/www/static/js/tree/details/content/dagRun/index.jsx
##########
@@ -0,0 +1,140 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import {
+ Flex,
+ Text,
+ Box,
+ Button,
+ Link,
+ Divider,
+} from '@chakra-ui/react';
+import { MdPlayArrow, MdOutlineAccountTree } from 'react-icons/md';
+
+import { SimpleStatus } from '../../../StatusBox';
+import { formatDuration } from '../../../../datetime_utils';
Review comment:
Not for this PR, but we might want to add some path aliasing so we can avoid all of the relative path imports.
##########
File path: airflow/www/static/js/tree/details/content/dagRun/MarkFailedRun.jsx
##########
@@ -0,0 +1,61 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import { Button, useDisclosure } from '@chakra-ui/react';
+
+import { useMarkFailedRun } from '../../../api';
+import ConfirmDialog from '../ConfirmDialog';
+
+const MarkFailedRun = ({ dagId, runId }) => {
+ const [affectedTasks, setAffectedTasks] = useState([]);
+ const { isOpen, onOpen, onClose } = useDisclosure();
+ const { mutateAsync: markFailed, isLoading } = useMarkFailedRun(dagId, runId);
+
+ const onClick = async () => {
+ try {
+ const data = await markFailed({ confirmed: false });
+ setAffectedTasks(data);
+ onOpen();
+ } catch (error) {
+ console.error(error);
+ }
+ };
+
+ const onConfirm = () => {
+ markFailed({ confirmed: true });
+ setAffectedTasks([]);
+ onClose();
+ };
+
+ return (
+ <>
+ <Button onClick={onClick} colorScheme="red" isLoading={isLoading}>Mark Failed</Button>
+ <ConfirmDialog
+ isOpen={isOpen}
+ onClose={onClose}
+ onConfirm={onConfirm}
+ description="Here's the list of task instances you are about to mark as failed:"
Review comment:
Similar copy suggestion here
```suggestion
description="Task instances you are about to mark as failed:"
```
##########
File path: airflow/www/static/js/tree/details/Header.jsx
##########
@@ -0,0 +1,95 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import {
+ Breadcrumb,
+ BreadcrumbItem,
+ BreadcrumbLink,
+ Box,
+ Heading,
+} from '@chakra-ui/react';
+import { MdPlayArrow } from 'react-icons/md';
+
+import { getMetaValue } from '../../utils';
+import { useSelection } from '../providers/selection';
+import Time from '../Time';
+import useTreeData from '../useTreeData';
+
+const dagId = getMetaValue('dag_id');
+
+const LabelValue = ({ label, value }) => (
+ <Box position="relative">
+ <Heading as="h5" size="sm" color="gray.300" position="absolute" top="-12px">{label}</Heading>
+ <Heading as="h3" size="md">{value}</Heading>
+ </Box>
+);
+
+const Header = () => {
+ const { data: { dagRuns = [] } } = useTreeData();
+ const { selected: { taskId, runId, task }, onSelect, clearSelection } = useSelection();
+ const dagRun = dagRuns.find((r) => r.runId === runId);
+
+ let runLabel;
+ if (dagRun) {
+ if (runId.includes('manual__') || runId.includes('scheduled__') || runId.includes('backfill__')) {
+ runLabel = (<Time dateTime={dagRun.dataIntervalEnd} />);
+ } else {
+ runLabel = runId;
+ }
+ if (dagRun.runType === 'manual') {
+ runLabel = (
+ <>
+ <MdPlayArrow style={{ display: 'inline' }} />
+ {runLabel}
+ </>
+ );
+ }
+ }
+
+ const isMapped = task && task.isMapped;
+ const lastIndex = taskId ? taskId.lastIndexOf('.') : null;
+ const taskName = lastIndex ? taskId.substring(lastIndex + 1) : taskId;
+
+ return (
+ <Breadcrumb color="gray.300">
+ <BreadcrumbItem isCurrentPage={!runId && !taskId} mt="15px">
Review comment:
Could the `mt="15px"` on each of these `BreadcrumbItem` be moved up to the parent `Breadcrumb`? Could we just use Chakra spacing instead? e.g. `mt={4}`
##########
File path: airflow/www/static/js/tree/details/content/dagRun/QueueRun.jsx
##########
@@ -0,0 +1,74 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, { useState } from 'react';
+import { Button, useDisclosure } from '@chakra-ui/react';
+
+import { useQueueRun } from '../../../api';
+import ConfirmDialog from '../ConfirmDialog';
+
+const QueueRun = ({ dagId, runId }) => {
+ const [affectedTasks, setAffectedTasks] = useState([]);
+ const { isOpen, onOpen, onClose } = useDisclosure();
+ const { mutateAsync: onQueue, isLoading } = useQueueRun(dagId, runId);
+
+ // Get what the changes will be and show it in a modal
+ const onClick = async () => {
+ try {
+ const data = await onQueue({ confirmed: false });
+ setAffectedTasks(data);
+ onOpen();
+ } catch (e) {
+ console.error(e);
+ }
+ };
+
+ // Confirm changes
+ const onConfirm = async () => {
+ try {
+ await onQueue({ confirmed: true });
+ setAffectedTasks([]);
+ onClose();
+ } catch (e) {
+ console.error(e);
+ }
+ };
+
+ return (
+ <>
+ <Button
+ onClick={onClick}
+ isLoading={isLoading}
+ ml="5px"
+ title="Queue up new tasks to make the DAG run up-to-date with any DAG file changes."
+ >
+ Queue up new tasks
+ </Button>
+ <ConfirmDialog
+ isOpen={isOpen}
+ onClose={onClose}
+ onConfirm={onConfirm}
+ description="Here's the list of task instances you are about to queue:"
Review comment:
Similar copy suggestion here
```suggestion
description="Task instances you are about to queue:"
```
--
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
[GitHub] [airflow] bbovenzi commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1074191905
Thanks for all the testing @eladkal!
1. I can do that in a later PR
2. Already fixed in a different PR
3. The whole DAG details page will change pretty soon, so I won't fix for now
4. I think this should be a later PR. It should totally happen, but to do correctly I think we need to redo how `tree_data` works in `views.py`. I will absolutely do it before 2.3.0, but I think making that change to this PR will make it even harder than it already is to review all of the code changes.
5. Added a "Reset Root" button if you filter upstream
6. Updated all the "More Details" to specifiy which details we are linking to
7. That title was backwards and now redundant. Removed.
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r833890461
##########
File path: airflow/www/views.py
##########
@@ -1961,24 +1976,53 @@ def _clear_dag_tis(
dry_run=True,
)
except AirflowException as ex:
- flash(str(ex), 'error')
- return redirect(origin)
+ return redirect_or_json(origin, return_as_json, msg=str(ex), status="error")
if not tis:
- flash("No task instances to clear", 'error')
- response = redirect(origin)
+ msg = "No task instances to clear"
+ return redirect_or_json(origin, return_as_json, msg, status="error")
else:
details = "\n".join(str(t) for t in tis)
response = self.render_template(
'airflow/confirm.html',
endpoint=None,
- message="Here's the list of task instances you are about to clear:",
+ message="Task instances you are about to clear:",
details=details,
)
return response
+ def _clear_dag_run(self, dag, start_date, end_date, recursive=False, confirmed=False, only_failed=False):
+ if confirmed:
+ count = dag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=recursive,
+ include_parentdag=recursive,
+ only_failed=only_failed,
+ )
+ return {'status': 'success', 'message': f"{count} task instances have been cleared"}
+
+ try:
+ tis = dag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=recursive,
+ include_parentdag=recursive,
+ only_failed=only_failed,
+ dry_run=True,
+ )
+ except AirflowException as ex:
+ return {'status': 'error', 'message': str(ex)}
+
+ if not tis:
+ return {'status': 'error', 'message': "No task instances to clear"}
+ else:
+ details = [str(t) for t in tis]
+
+ return htmlsafe_json_dumps(details, separators=(',', ':'))
Review comment:
Is this the same as `_clear_dag_tis` but without `flash` and with different return values? We can probably combine them if that’s the case.
--
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
[GitHub] [airflow] eladkal commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1073341799
Played with it a bit in local env this is **really really good!**
Have a few suggestions/bugs **all are minor**:
1. [Suggestion] Can we have the ability to easily copy to clipboard?
To give example of what i had in mind:
2. [Bug] Need to fix the name to Grid also in:
3. [Bug] When changing Timezone there is a weird "jump" of values :
Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?
4. [Bug] When opening the panel a fresh task (state None) when the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
The values in the panel should be refreshed when with the auto-refresh property.
5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?
6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.
7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and viseveria. The tool tip is redundant any anyway isn't 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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837699337
##########
File path: airflow/www/static/js/tree/ResetRoot.jsx
##########
@@ -0,0 +1,55 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React from 'react';
+import { Button, Link } from '@chakra-ui/react';
+
+import { getMetaValue } from '../utils';
+
+const dagId = getMetaValue('dag_id');
+const numRuns = getMetaValue('num_runs');
+const urlRoot = getMetaValue('root');
+const baseDate = getMetaValue('base_date');
+
+const ResetRoot = () => {
+ let rootLink;
+ if (urlRoot) {
+ const params = new URLSearchParams();
+ if (numRuns) params.set('num_runs', numRuns);
+ if (baseDate) params.set('base_date', baseDate);
+ rootLink = `/dags/${dagId}/grid?${params.toString()}`;
Review comment:
All urls in react should be updated now.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r829364328
##########
File path: airflow/www/static/js/dag.js
##########
@@ -227,22 +227,6 @@ export function callModal(t, d, extraLinks, tryNumbers, sd, drID) {
}
}
-export function callModalDag(dag) {
Review comment:
We only called this in the grid view, so we can fully replace it
##########
File path: airflow/www/static/js/tree/InstanceTooltip.jsx
##########
@@ -78,86 +76,27 @@ const InstanceTooltip = ({
});
}
- const taskIdTitle = isGroup ? 'Task Group Id: ' : 'Task Id: ';
-
return (
- <Box fontSize="12px" py="4px">
+ <Box fontSize="12px" py="2px">
{group.tooltip && (
<Text>{group.tooltip}</Text>
)}
<Text>
- <Text as="strong">Status:</Text>
+ {isGroup ? 'Overall ' : ''}
+ Status:
{' '}
{state || 'no status'}
</Text>
- {isGroup && (
- <>
- <br />
- <Text as="strong">Group Summary</Text>
- {groupSummary}
- </>
- )}
- {group.isMapped && (
- <>
- <br />
- <Text as="strong">
- {mappedStates.length}
- {' '}
- {mappedStates.length === 1 ? 'Task ' : 'Tasks '}
- Mapped
- </Text>
- {mapSummary}
- </>
- )}
- <br />
- <Text>
- {taskIdTitle}
- {taskId}
- </Text>
- <Text whiteSpace="nowrap">
- Run Id:
- {' '}
- {runId}
- </Text>
- {operator && (
- <Text>
- Operator:
- {' '}
- {operator}
- </Text>
- )}
- <Text>
- Duration:
- {' '}
- {formatDuration(duration || getDuration(startDate, endDate))}
- </Text>
- <br />
- <Text as="strong">UTC</Text>
- <Text>
- Started:
- {' '}
- {startDate && formatDateTime(moment.utc(startDate))}
- </Text>
- <Text>
- Ended:
- {' '}
- {endDate && formatDateTime(moment.utc(endDate))}
- </Text>
- <br />
- <Text as="strong">
- Local:
- {' '}
- {moment().format('Z')}
- </Text>
+ {isGroup && groupSummary}
<Text>
Started:
{' '}
{startDate && formatDateTime(startDate)}
</Text>
<Text>
- Ended:
+ Duration:
{' '}
- {endDate && formatDateTime(endDate)}
+ {formatDuration(duration || getDuration(startDate, endDate))}
Review comment:
`endDate` is in the details but not in the tooltip.
When in progress it will use `Date.now()` instead of enddate
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -375,70 +380,6 @@ <h4>Task Actions</h4>
</div>
</div>
</div>
- <!-- Modal for DAG run -->
Review comment:
This is now all in React instead.
--
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
[GitHub] [airflow] bbovenzi commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1071470388
> > 2. I think in the confirm pop-ups we should default the confirm button
>
> What did the current confirm page default to?
It highlighted "Cancel" by default.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r829392229
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -375,70 +380,6 @@ <h4>Task Actions</h4>
</div>
</div>
</div>
- <!-- Modal for DAG run -->
Review comment:
This is now all in React instead.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r832451709
##########
File path: airflow/www/static/js/tree/details/content/dagRun/index.jsx
##########
@@ -0,0 +1,140 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import {
+ Flex,
+ Text,
+ Box,
+ Button,
+ Link,
+ Divider,
+} from '@chakra-ui/react';
+import { MdPlayArrow, MdOutlineAccountTree } from 'react-icons/md';
+
+import { SimpleStatus } from '../../../StatusBox';
+import { formatDuration } from '../../../../datetime_utils';
Review comment:
Absolutely. It pained me to write all of these relative path imports.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r834581281
##########
File path: airflow/www/views.py
##########
@@ -1718,6 +1730,7 @@ def run(self, session=None):
dag_run_id = request.form.get('dag_run_id')
map_index = request.args.get('map_index', -1, type=int)
origin = get_safe_url(request.form.get('origin'))
+ return_as_json = request.form.get('return_as_json') == "true"
Review comment:
Good call. I added a check for `request.headers['Accept'] == 'application/json'` instead of a param
--
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
[GitHub] [airflow] eladkal edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1073341799
Played with it a bit in local env this is **really really good!**
Have a few suggestions/bugs **all are minor**:
1. [Suggestion] Can we have the ability to easily copy to clipboard?
To give example of what i had in mind:
2. [Tweak] Need to fix the name to Grid also in:
3. [Bug] When changing Timezone there is a weird "jump" of values :
Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?
4. [Bug] When opening the panel a fresh task (state None) when the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
The values in the panel should be refreshed when with the auto-refresh property.
5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?
6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.
7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and viseveria. The tool tip is redundant any anyway isn't 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
[GitHub] [airflow] ashb commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1071466305
> 2. I think in the confirm pop-ups we should default the confirm button
What did the current confirm page default to?
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r835360152
##########
File path: airflow/www/views.py
##########
@@ -527,6 +527,21 @@ def get_task_stats_from_query(qry):
return data
+def redirect_or_json(origin, return_as_json, msg, status=""):
Review comment:
TIL
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837581757
##########
File path: airflow/www/static/js/tree/api/index.js
##########
@@ -0,0 +1,57 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import axios from 'axios';
+import camelcaseKeys from 'camelcase-keys';
+
+import useDag from './useDag';
+import useTasks from './useTasks';
+import useClearRun from './useClearRun';
+import useQueueRun from './useQueueRun';
+import useMarkFailedRun from './useMarkFailedRun';
+import useMarkSuccessRun from './useMarkSuccessRun';
+import useRunTask from './useRunTask';
+import useClearTask from './useClearTask';
+import useMarkFailedTask from './useMarkFailedTask';
+import useMarkSuccessTask from './useMarkSuccessTask';
+import useExtraLinks from './useExtraLinks';
+import useConfirmMarkTask from './useConfirmMarkTask';
+import useTreeData from './useTreeData';
+
+axios.interceptors.response.use(
+ (res) => (res.data ? camelcaseKeys(res.data, { deep: true }) : res),
+);
+
+axios.defaults.headers.common.Accept = 'application/json';
+
+export {
+ useDag,
Review comment:
They are hooks, which are a particular type of function in React. https://reactjs.org/docs/hooks-intro.html
>A custom Hook is a JavaScript function whose name starts with ”use” and that may call other Hooks.
--
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
[GitHub] [airflow] ashb edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084716869
Feedback. (some of these can be in follow up PRs.)
1. Dag name clickable but doesn't do anything
URL: `http://localhost:8080/dags/mapped_bash_notaskgroup/grid` (click dag name from home page
It's a bit odd to have the dag id look and act like a link, but not do anything
2. ~Clicking on the DagRun bar gives react error~
~Click where arrow is pointing~
Error in web console:
```
Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
Check the render method of `DagRun`.
DagRun@webpack-internal:///./static/js/tree/details/content/dagRun/index.jsx:48:16
```
Edit: was yarn/webpack issue. Rebuilding and it works now.
3. Lack of pushState means back button doesn't do what I expect (takes me back to home page, not previous view.
4. For a mapped TI, "All Instances" doesn't filter to the current dag run,
It's _all_ instances of that task, This was unexpected to me. (The task hadn't yet run, it was scheduled.)
5. Something made a GET request that included a csrf_token -- it's should _not_ be sent for GET requests.
`[2022-03-31 16:02:07,660] 647276 MainProcess {{werkzeug _internal.py:113}} INFO - 127.0.0.1 - - [31/Mar/2022 16:02:07] "GET /dags/mapped_bash_notaskgroup/grid?root=&dag_id=mapped_bash_notaskgroup&_csrf_token=ImU5NzNhNTNjNmQ2NTE3N2U0MTYzMGQyODEwOTkxNDU3NzNiMThlMjIi.YkXBwg.RkGHB2tbVF1KVM7dzy3wkDKxjp0&base_date=1971-04-07+00%3A00%3A00%2B00%3A00&num_runs=365 HTTP/1.1" 200 -`
Oh, and that also includes a dag_id query parameter which it shouldn't.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837938241
##########
File path: airflow/www/views.py
##########
@@ -2554,6 +2552,20 @@ def grid(self, dag_id, session=None):
'dag_runs': encoded_runs,
}
+ dag_details_url = ''
+ tasks_url = ''
+ # This is a workround to use try/except if url_for() throws an error
Review comment:
During tests `url_for` was throwing an error, so I moved the logic into `views.py` but I'm all ears on a better way to handle this.
It would be nice to have a sort of `url_for(RestAPI)` function.
--
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
[GitHub] [airflow] bbovenzi commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1071470388
> > 2. I think in the confirm pop-ups we should default the confirm button
>
> What did the current confirm page default to?
It highlighted "Cancel" by default.
--
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
[GitHub] [airflow] bbovenzi edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1074191905
Thanks for all the testing @eladkal!
1. I can do that in a later PR
2. Already fixed in a different PR
3. The whole DAG details page will change pretty soon, so I won't fix for now
4. I think this should be a later PR. It should totally happen, but to do correctly I think we need to redo how `tree_data` works in `views.py`. I will absolutely do it before 2.3.0, but I think making that change to this PR will make it even harder than it already is to review all of the code changes. (Edit: I could add Dag Run details refresh easily, Task Instance requires API changes).
5. Added a "Reset Root" button if you filter upstream
6. Updated all the "More Details" to specifiy which details we are linking to
7. That title was backwards and now redundant. Removed.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r832449943
##########
File path: airflow/www/static/js/tree/StatusBox.jsx
##########
@@ -24,55 +24,76 @@ import { isEqual } from 'lodash';
import {
Box,
Tooltip,
+ useTheme,
} from '@chakra-ui/react';
-import { callModal } from '../dag';
import InstanceTooltip from './InstanceTooltip';
+import { useContainerRef } from './providers/containerRef';
+import { useSelection } from './providers/selection';
export const boxSize = 10;
export const boxSizePx = `${boxSize}px`;
+export const SimpleStatus = ({ state, ...rest }) => (
+ <Box
+ width={boxSizePx}
+ height={boxSizePx}
+ backgroundColor={stateColors[state] || 'white'}
+ borderRadius="2px"
+ borderWidth={state ? 0 : 1}
+ {...rest}
+ />
+);
+
const StatusBox = ({
- group, instance, containerRef, extraLinks = [],
+ group, instance,
}) => {
- const {
- executionDate, taskId, tryNumber = 0, operator, runId, mapIndex,
- } = instance;
- const onClick = () => executionDate && callModal(taskId, executionDate, extraLinks, tryNumber, operator === 'SubDagOperator', runId, mapIndex);
+ const containerRef = useContainerRef();
+ const { selected, onSelect } = useSelection();
+ const { runId, taskId } = instance;
+ const { colors } = useTheme();
+ const hoverBlue = `${colors.blue[100]}50`;
Review comment:
Yes, the `50` is the transparency value like you were using `rgba()` instead of `rgb()`
--
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
[GitHub] [airflow] norm commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
norm commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084673424
Apparently I *can* approve 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
[GitHub] [airflow] bbovenzi closed pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi closed pull request #22123:
URL: https://github.com/apache/airflow/pull/22123
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837938241
##########
File path: airflow/www/views.py
##########
@@ -2554,6 +2552,20 @@ def grid(self, dag_id, session=None):
'dag_runs': encoded_runs,
}
+ dag_details_url = ''
+ tasks_url = ''
+ # This is a workround to use try/except if url_for() throws an error
Review comment:
During tests `url_for` was throwing an error, so I moved the logic into `views.py` but I'm all ears on a better way to handle this.
In the future, it would be nice to have a sort of `url_for(RestAPI)` function.
--
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
[GitHub] [airflow] uranusjr commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1077048869
Python logic looks good to me, but we can probably do better sharing logic between HTML and JSON requests.
--
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
[GitHub] [airflow] ashb commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r823662829
##########
File path: airflow/www/static/js/tree/InstanceTooltip.jsx
##########
@@ -78,86 +76,27 @@ const InstanceTooltip = ({
});
}
- const taskIdTitle = isGroup ? 'Task Group Id: ' : 'Task Id: ';
-
return (
- <Box fontSize="12px" py="4px">
+ <Box fontSize="12px" py="2px">
{group.tooltip && (
<Text>{group.tooltip}</Text>
)}
<Text>
- <Text as="strong">Status:</Text>
+ {isGroup ? 'Overall ' : ''}
+ Status:
{' '}
{state || 'no status'}
</Text>
- {isGroup && (
- <>
- <br />
- <Text as="strong">Group Summary</Text>
- {groupSummary}
- </>
- )}
- {group.isMapped && (
- <>
- <br />
- <Text as="strong">
- {mappedStates.length}
- {' '}
- {mappedStates.length === 1 ? 'Task ' : 'Tasks '}
- Mapped
- </Text>
- {mapSummary}
- </>
- )}
- <br />
- <Text>
- {taskIdTitle}
- {taskId}
- </Text>
- <Text whiteSpace="nowrap">
- Run Id:
- {' '}
- {runId}
- </Text>
- {operator && (
- <Text>
- Operator:
- {' '}
- {operator}
- </Text>
- )}
- <Text>
- Duration:
- {' '}
- {formatDuration(duration || getDuration(startDate, endDate))}
- </Text>
- <br />
- <Text as="strong">UTC</Text>
- <Text>
- Started:
- {' '}
- {startDate && formatDateTime(moment.utc(startDate))}
- </Text>
- <Text>
- Ended:
- {' '}
- {endDate && formatDateTime(moment.utc(endDate))}
- </Text>
- <br />
- <Text as="strong">
- Local:
- {' '}
- {moment().format('Z')}
- </Text>
+ {isGroup && groupSummary}
<Text>
Started:
{' '}
{startDate && formatDateTime(startDate)}
</Text>
<Text>
- Ended:
+ Duration:
{' '}
- {endDate && formatDateTime(endDate)}
+ {formatDuration(duration || getDuration(startDate, endDate))}
Review comment:
What does this show when the task is still in progress?
Do we not show end date anymore?
--
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
[GitHub] [airflow] eladkal edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1073341799
Played with it a bit in local env this is **really really good!**
Have a few suggestions/bugs **all are minor**:
1. [Suggestion] Can we have the ability to easily copy to clipboard?
To give example of what i had in mind:
2. [Tweak] Need to fix the name to Grid also in:
3. [Bug] When changing Timezone there is a weird "jump" of values :
Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?
4. [Bug] When opening the panel a fresh task (state None) when the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
The values in the panel should be refreshed when with the auto-refresh property.
(Edit: noticed also the status left as no-status though that is Success and probably duration is also not updated.. so I guess the whole panel isn't being updated with the auto refresh functionality)
5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?
6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.
7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and vise versia. The tool tip is redundant any anyway isn't 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
[GitHub] [airflow] eladkal edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1073341799
Played with it a bit in local env this is **really really good!**
Have a few suggestions/bugs **all are minor**:
1. [Suggestion] Can we have the ability to easily copy to clipboard?
To give example of what i had in mind:
2. [Tweak] Need to fix the name to Grid also in:
3. [Bug] When changing Timezone there is a weird "jump" of values :
Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?
4. [Bug] When opening the panel a fresh task (state None) when the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
The values in the panel should be refreshed when with the auto-refresh property.
5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?
6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.
7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and vise versia. The tool tip is redundant any anyway isn't 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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r832600438
##########
File path: airflow/www/static/js/tree/StatusBox.jsx
##########
@@ -24,55 +24,76 @@ import { isEqual } from 'lodash';
import {
Box,
Tooltip,
+ useTheme,
} from '@chakra-ui/react';
-import { callModal } from '../dag';
import InstanceTooltip from './InstanceTooltip';
+import { useContainerRef } from './providers/containerRef';
+import { useSelection } from './providers/selection';
export const boxSize = 10;
export const boxSizePx = `${boxSize}px`;
+export const SimpleStatus = ({ state, ...rest }) => (
+ <Box
+ width={boxSizePx}
+ height={boxSizePx}
+ backgroundColor={stateColors[state] || 'white'}
+ borderRadius="2px"
+ borderWidth={state ? 0 : 1}
+ {...rest}
+ />
+);
+
const StatusBox = ({
- group, instance, containerRef, extraLinks = [],
+ group, instance,
}) => {
- const {
- executionDate, taskId, tryNumber = 0, operator, runId, mapIndex,
- } = instance;
- const onClick = () => executionDate && callModal(taskId, executionDate, extraLinks, tryNumber, operator === 'SubDagOperator', runId, mapIndex);
+ const containerRef = useContainerRef();
+ const { selected, onSelect } = useSelection();
+ const { runId, taskId } = instance;
+ const { colors } = useTheme();
+ const hoverBlue = `${colors.blue[100]}50`;
Review comment:
For reference: https://caniuse.com/css-rrggbbaa
--
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
[GitHub] [airflow] uranusjr commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r835352048
##########
File path: airflow/www/views.py
##########
@@ -527,6 +527,21 @@ def get_task_stats_from_query(qry):
return data
+def redirect_or_json(origin, return_as_json, msg, status=""):
Review comment:
Actually you don’t even need to pass in `request`; the variable is magical in Flask and will automatically refer to the correct thing when it’s called in a view function.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r836042491
##########
File path: airflow/www/views.py
##########
@@ -527,6 +527,21 @@ def get_task_stats_from_query(qry):
return data
+def redirect_or_json(origin, return_as_json, msg, status=""):
Review comment:
Hmm. That seemed to cause some testing errors with `KeyError: 'HTTP_ACCEPT'`. Would you have any idea on how to address that?
--
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
[GitHub] [airflow] ashb commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084716869
Feedback. (some of these can be in follow up PRs.)
1. Dag name clickable but doesn't do anything
URL: `http://localhost:8080/dags/mapped_bash_notaskgroup/grid` (click dag name from home page
It's a bit odd to have the dag id look and act like a link, but not do anything
2. Clicking on the DagRun bar gives react error
Click where arrow is pointing
Error in web console:
```
Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
Check the render method of `DagRun`.
DagRun@webpack-internal:///./static/js/tree/details/content/dagRun/index.jsx:48:16
```
3. Lack of pushState means back button doesn't do what I expect (takes me back to home page, not previous view.
4. For a mapped TI, "All Instances" doesn't filter to the current dag run,
It's _all_ instances of that task, This was unexpected to me. (The task hadn't yet run, it was scheduled.)
5. Something made a GET request that included a csrf_token -- it's should _not_ be sent for GET requests.
`[2022-03-31 16:02:07,660] 647276 MainProcess {{werkzeug _internal.py:113}} INFO - 127.0.0.1 - - [31/Mar/2022 16:02:07] "GET /dags/mapped_bash_notaskgroup/grid?root=&dag_id=mapped_bash_notaskgroup&_csrf_token=ImU5NzNhNTNjNmQ2NTE3N2U0MTYzMGQyODEwOTkxNDU3NzNiMThlMjIi.YkXBwg.RkGHB2tbVF1KVM7dzy3wkDKxjp0&base_date=1971-04-07+00%3A00%3A00%2B00%3A00&num_runs=365 HTTP/1.1" 200 -`
--
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
[GitHub] [airflow] bbovenzi edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084992971
@ashb
1\. Is fixed
3\. Can explore later
4\. Fixed
5\. Happened before this PR, will do separately
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837704087
##########
File path: airflow/www/templates/airflow/calendar.html
##########
@@ -22,7 +22,6 @@
{% block head_css %}
{{ super() }}
- <meta name="grid_url" content="{{ url_for('Airflow.grid', dag_id=dag.dag_id) }}">
Review comment:
Adding `grid_url` to `dag.html` so this is now redundant.
--
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
[GitHub] [airflow] bbovenzi commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084992971
@ashb
1. Is fixed
3. Can explore later
4. Fixed
5. Happened before this PR, will do separately
--
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
[GitHub] [airflow] eladkal commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1071347002
Looks really good!
2 comments:
1. for me the arrow is associated with the browser arrows (back / forward). In this case the action is close so wouldn't X icon be more suitable?
2. I think in the confirm pop-ups we should default the confirm button
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r829370840
##########
File path: airflow/www/static/js/tree/InstanceTooltip.jsx
##########
@@ -78,86 +76,27 @@ const InstanceTooltip = ({
});
}
- const taskIdTitle = isGroup ? 'Task Group Id: ' : 'Task Id: ';
-
return (
- <Box fontSize="12px" py="4px">
+ <Box fontSize="12px" py="2px">
{group.tooltip && (
<Text>{group.tooltip}</Text>
)}
<Text>
- <Text as="strong">Status:</Text>
+ {isGroup ? 'Overall ' : ''}
+ Status:
{' '}
{state || 'no status'}
</Text>
- {isGroup && (
- <>
- <br />
- <Text as="strong">Group Summary</Text>
- {groupSummary}
- </>
- )}
- {group.isMapped && (
- <>
- <br />
- <Text as="strong">
- {mappedStates.length}
- {' '}
- {mappedStates.length === 1 ? 'Task ' : 'Tasks '}
- Mapped
- </Text>
- {mapSummary}
- </>
- )}
- <br />
- <Text>
- {taskIdTitle}
- {taskId}
- </Text>
- <Text whiteSpace="nowrap">
- Run Id:
- {' '}
- {runId}
- </Text>
- {operator && (
- <Text>
- Operator:
- {' '}
- {operator}
- </Text>
- )}
- <Text>
- Duration:
- {' '}
- {formatDuration(duration || getDuration(startDate, endDate))}
- </Text>
- <br />
- <Text as="strong">UTC</Text>
- <Text>
- Started:
- {' '}
- {startDate && formatDateTime(moment.utc(startDate))}
- </Text>
- <Text>
- Ended:
- {' '}
- {endDate && formatDateTime(moment.utc(endDate))}
- </Text>
- <br />
- <Text as="strong">
- Local:
- {' '}
- {moment().format('Z')}
- </Text>
+ {isGroup && groupSummary}
<Text>
Started:
{' '}
{startDate && formatDateTime(startDate)}
</Text>
<Text>
- Ended:
+ Duration:
{' '}
- {endDate && formatDateTime(endDate)}
+ {formatDuration(duration || getDuration(startDate, endDate))}
Review comment:
`endDate` is in the details but not in the tooltip.
When in progress it will use `Date.now()` instead of enddate
--
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
[GitHub] [airflow] eladkal edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1073341799
Played with it a bit in local env this is **really really good!**
Have a few suggestions/bugs **all are minor**:
1. [Suggestion] Can we have the ability to easily copy to clipboard?
To give example of what i had in mind:
2. [Tweak] Need to fix the name to Grid also in:
3. [Bug] When changing Timezone there is a weird "jump" of values :
Also the timezone changes only in start date but not in Timezone (last row in the table) - why?
Is this the timezone the DAG is defined with in the code? If so maybe better to call it DAG defined timezone or something like that?
4. [Bug] When opening the panel of a fresh task (state None) the panel isn't refreshed while the task progresses. So when the task finishes the started/ended values are empty (you must refresh the screen manually to solve this)
The values in the panel should be refreshed when with the auto-refresh property.
(Edit: noticed also the status left as no-status though that is Success and probably duration is also not updated.. so I guess the whole panel isn't being updated with the auto refresh functionality)
5. [Suggestion] I got a bit lost when clicked on Filter upstream. It was impossible to return where I was before I had to enter the Grid view and repeat my steps. Is there a way to make a better experience with it?
6. [Suggestion] There are two "More details" one that leads to DAG details and one that leads to Task Details. Can we simply rename it to be "DAG details"/"Task Details"?
The same goes for "Show Details Panel" It shows different panel for Task/DAG but the button has the same name.
7. [Bug] I'm not really sure how I managed to do that but I get a tooltip of Show when the button says hide and vise versia. The tool tip is redundant any anyway isn't 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
[GitHub] [airflow] bbovenzi edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1074191905
Thanks for all the testing @eladkal!
1. I can do that in a later PR
2. Already fixed in a different PR
3. The whole DAG details page will change pretty soon, so I won't fix for now
4. ~~I think this should be a later PR. It should totally happen, but to do correctly I think we need to redo how `tree_data` works in `views.py`. I will absolutely do it before 2.3.0, but I think making that change to this PR will make it even harder than it already is to review all of the code changes.~~ (Edit: I figured out a workable solution for this PR.)
5. Added a "Reset Root" button if you filter upstream
6. Updated all the "More Details" to specifiy which details we are linking to
7. That title was backwards and now redundant. Removed.
--
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
[GitHub] [airflow] bbovenzi commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r835348315
##########
File path: airflow/www/views.py
##########
@@ -527,6 +527,21 @@ def get_task_stats_from_query(qry):
return data
+def redirect_or_json(origin, return_as_json, msg, status=""):
Review comment:
Yeah, I didn't like that variable name. Updated.
--
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
[GitHub] [airflow] eladkal commented on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1071347002
Looks really good!
2 comments:
1. for me the arrow is associated with the browser arrows (back / forward). In this case the action is close so wouldn't X icon be more suitable?
2. I think in the confirm pop-ups we should default the confirm button
--
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
[GitHub] [airflow] jedcunningham commented on a change in pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#discussion_r837956949
##########
File path: airflow/www/views.py
##########
@@ -2554,6 +2552,20 @@ def grid(self, dag_id, session=None):
'dag_runs': encoded_runs,
}
+ dag_details_url = ''
+ tasks_url = ''
+ # This is a workround to use try/except if url_for() throws an error
Review comment:
The api init step is skipped in those test. Try this instead:
```
--- a/tests/www/views/test_views_log.py
+++ b/tests/www/views/test_views_log.py
@@ -56,7 +56,12 @@ def backup_modules():
@pytest.fixture(scope="module")
def log_app(backup_modules):
@dont_initialize_flask_app_submodules(
- skip_all_except=["init_appbuilder", "init_jinja_globals", "init_appbuilder_views"]
+ skip_all_except=[
+ "init_appbuilder",
+ "init_jinja_globals",
+ "init_appbuilder_views",
+ "init_api_connexion",
+ ]
)
@conf_vars({('logging', 'logging_config_class'): 'airflow_local_settings.LOGGING_CONFIG'})
def factory():
```
--
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
[GitHub] [airflow] ashb edited a comment on pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on pull request #22123:
URL: https://github.com/apache/airflow/pull/22123#issuecomment-1084716869
Feedback. (some of these can be in follow up PRs.)
1. Dag name clickable but doesn't do anything
URL: `http://localhost:8080/dags/mapped_bash_notaskgroup/grid` (click dag name from home page
It's a bit odd to have the dag id look and act like a link, but not do anything
2. Clicking on the DagRun bar gives react error
Click where arrow is pointing
Error in web console:
```
Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in, or you might have mixed up default and named imports.
Check the render method of `DagRun`.
DagRun@webpack-internal:///./static/js/tree/details/content/dagRun/index.jsx:48:16
```
3. Lack of pushState means back button doesn't do what I expect (takes me back to home page, not previous view.
4. For a mapped TI, "All Instances" doesn't filter to the current dag run,
It's _all_ instances of that task, This was unexpected to me. (The task hadn't yet run, it was scheduled.)
5. Something made a GET request that included a csrf_token -- it's should _not_ be sent for GET requests.
`[2022-03-31 16:02:07,660] 647276 MainProcess {{werkzeug _internal.py:113}} INFO - 127.0.0.1 - - [31/Mar/2022 16:02:07] "GET /dags/mapped_bash_notaskgroup/grid?root=&dag_id=mapped_bash_notaskgroup&_csrf_token=ImU5NzNhNTNjNmQ2NTE3N2U0MTYzMGQyODEwOTkxNDU3NzNiMThlMjIi.YkXBwg.RkGHB2tbVF1KVM7dzy3wkDKxjp0&base_date=1971-04-07+00%3A00%3A00%2B00%3A00&num_runs=365 HTTP/1.1" 200 -`
Oh, and that also includes a dag_id query parameter which it shouldn't.
--
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
[GitHub] [airflow] bbovenzi closed pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi closed pull request #22123:
URL: https://github.com/apache/airflow/pull/22123
--
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
[GitHub] [airflow] bbovenzi merged pull request #22123: Add details drawer to Grid View
Posted by GitBox <gi...@apache.org>.
bbovenzi merged pull request #22123:
URL: https://github.com/apache/airflow/pull/22123
--
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