pierrejeambrun commented on code in PR #26457:
URL: https://github.com/apache/airflow/pull/26457#discussion_r982742687
##########
airflow/api_connexion/endpoints/task_instance_endpoint.py:
##########
@@ -545,3 +546,54 @@ def post_set_task_instances_state(*, dag_id: str, session:
Session = NEW_SESSION
session=session,
)
return
task_instance_reference_collection_schema.dump(TaskInstanceReferenceCollection(task_instances=tis))
+
+
[email protected]_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG_RUN),
+ ],
+)
+@provide_session
+def set_task_instance_note(
Review Comment:
```suggestion
def set_task_instance_notes(
```
I think the name should match the field we set.
##########
airflow/www/static/js/api/useSetTaskInstanceNotes.ts:
##########
@@ -0,0 +1,107 @@
+/*!
+ * 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 { useMutation, useQueryClient } from 'react-query';
+import { getMetaValue, getTask } from 'src/utils';
+import type { GridData } from 'src/api/useGridData';
+import { emptyGridData } from 'src/api/useGridData';
+import type { API } from 'src/types';
+import useErrorToast from '../utils/useErrorToast';
+
+const setTaskInstancesNotesURI = getMetaValue('set_task_instance_note');
+
+export default function useSetTaskInstanceNotes(
+ dagId: string,
+ runId: string,
+ taskId: string,
+ mapIndex: number,
+ newNotesValue: string,
+) {
+ const queryClient = useQueryClient();
+ const errorToast = useErrorToast();
+ // Note: Werkzeug does not like the META URL with an integer. It can not put
_MAP_INDEX_ there
Review Comment:
cf my comment to maybe try to avoid this, and useTaskInstance handle this
same scenario. (maybe not in the best way but might be worth to take a look)
##########
airflow/models/dagrun.py:
##########
@@ -119,6 +119,7 @@ class DagRun(Base, LoggingMixin):
# When a scheduler last attempted to schedule TIs for this DagRun
last_scheduling_decision = Column(UtcDateTime)
dag_hash = Column(String(32))
+ notes = Column(String(1000))
Review Comment:
nit: `notes` as a plural made me think at first sight that it would be a `1
to N` relationship, like several notes. Maybe that's just me but I feel like
choosing the singular `note` would avoid any confusion.
##########
airflow/api_connexion/endpoints/dag_run_endpoint.py:
##########
@@ -413,3 +414,28 @@ def clear_dag_run(*, dag_id: str, dag_run_id: str,
session: Session = NEW_SESSIO
)
dag_run.refresh_from_db()
return dagrun_schema.dump(dag_run)
+
+
[email protected]_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG_RUN),
+ ],
+)
+@provide_session
+def set_dag_run_note(*, dag_id: str, dag_run_id: str, session: Session =
NEW_SESSION) -> APIResponse:
Review Comment:
```suggestion
def set_dag_run_notes(*, dag_id: str, dag_run_id: str, session: Session =
NEW_SESSION) -> APIResponse:
```
Same
##########
airflow/www/static/js/api/useSetDagRunNotes.ts:
##########
@@ -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 axios from 'axios';
+import { useMutation, useQueryClient } from 'react-query';
+import { getMetaValue } from 'src/utils';
+import useErrorToast from '../utils/useErrorToast';
+import type { GridData } from './useGridData';
+import { emptyGridData } from './useGridData';
+
+const setDagRunNotesURI = getMetaValue('set_dag_run_note');
+
+export default function useSetDagRunNotes(
+ dagId: string,
+ runId: string,
+ newNotesValue: string,
+) {
+ const queryClient = useQueryClient();
+ const errorToast = useErrorToast();
+ const setDagRunNotes = setDagRunNotesURI.replace('_DAG_RUN_ID_', runId);
+
+ const updateGridData = (oldValue: GridData | undefined) => {
+ if (oldValue == null) return emptyGridData;
+ const run = oldValue.dagRuns.find((dr) => dr.runId === runId);
+ if (run) {
+ run.notes = newNotesValue;
Review Comment:
I'm not really sure about mutation like this, as we usually try to avoid it,
maybe it's fine here though :thinking:. (I have no idea to be honest)
##########
airflow/api_connexion/endpoints/task_instance_endpoint.py:
##########
@@ -545,3 +546,54 @@ def post_set_task_instances_state(*, dag_id: str, session:
Session = NEW_SESSION
session=session,
)
return
task_instance_reference_collection_schema.dump(TaskInstanceReferenceCollection(task_instances=tis))
+
+
[email protected]_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG_RUN),
Review Comment:
Here we are updating a TaskInstance, should we also require the permission
for that ?
```python
(permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE)
```
##########
airflow/www/static/js/api/useSetDagRunNotes.ts:
##########
@@ -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 axios from 'axios';
+import { useMutation, useQueryClient } from 'react-query';
+import { getMetaValue } from 'src/utils';
+import useErrorToast from '../utils/useErrorToast';
+import type { GridData } from './useGridData';
+import { emptyGridData } from './useGridData';
+
+const setDagRunNotesURI = getMetaValue('set_dag_run_note');
+
+export default function useSetDagRunNotes(
Review Comment:
Types are auto generated for the API. This way typescript types are always
in sync with the api spec. (allows us to write less code and spot bugs much
more easily). For instance in this case you can directly use:
```
API.SetDagRunNoteVariables
```
You can take a look at `useTaskInstance` hook to get an example.
Could directly write something like:
##########
airflow/www/static/js/api/useSetTaskInstanceNotes.ts:
##########
@@ -0,0 +1,107 @@
+/*!
+ * 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 { useMutation, useQueryClient } from 'react-query';
+import { getMetaValue, getTask } from 'src/utils';
+import type { GridData } from 'src/api/useGridData';
+import { emptyGridData } from 'src/api/useGridData';
+import type { API } from 'src/types';
+import useErrorToast from '../utils/useErrorToast';
+
+const setTaskInstancesNotesURI = getMetaValue('set_task_instance_note');
+
+export default function useSetTaskInstanceNotes(
+ dagId: string,
+ runId: string,
+ taskId: string,
+ mapIndex: number,
+ newNotesValue: string,
Review Comment:
Same here you should be able to directly use the API generated type.
`SetTaskInstanceNoteVariables`
##########
airflow/api_connexion/endpoints/task_instance_endpoint.py:
##########
@@ -545,3 +546,54 @@ def post_set_task_instances_state(*, dag_id: str, session:
Session = NEW_SESSION
session=session,
)
return
task_instance_reference_collection_schema.dump(TaskInstanceReferenceCollection(task_instances=tis))
+
+
[email protected]_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_DAG_RUN),
+ ],
+)
+@provide_session
+def set_task_instance_note(
+ *, dag_id: str, dag_run_id: str, task_id: str, session: Session =
NEW_SESSION
+) -> APIResponse:
+ """Set the note for a dag run."""
+ try:
+ post_body =
set_task_instance_note_form_schema.load(get_json_request_dict())
+ new_value_for_notes = post_body["notes"]
+ # Note: We can't add map_index to the url as subpaths can't start with
dashes.
+ map_index = post_body["map_index"]
+ except ValidationError as err:
+ raise BadRequest(detail=str(err))
+
+ query = (
+ session.query(TI)
+ .filter(TI.dag_id == dag_id)
+ .filter(TI.run_id == dag_run_id)
+ .filter(TI.task_id == task_id)
+ )
+ if map_index == -1:
+ query = query.filter(or_(TI.map_index == -1, TI.map_index is None))
+ else:
+ query = query.filter(TI.map_index == map_index)
+
+ try:
+ ti: TI | None = query.one_or_none()
+ except MultipleResultsFound:
+ raise NotFound(
+ "Task instance not found", detail="Task instance is mapped, add
the map_index value to the URL"
+ )
+ if ti is None:
+ error_message = f'Task Instance not found for dag_id={dag_id},
run_id={dag_run_id}, task_id={task_id}'
+ raise NotFound(error_message)
+
+ ti.notes = new_value_for_notes
+ session.commit()
+
+ if map_index == -1:
+ return get_task_instance(dag_id=dag_id, dag_run_id=dag_run_id,
task_id=task_id)
Review Comment:
We already have the TaskInstance available in the scope, is it possible to
directly serialize this instance and return it, instead of calling different
functions that will make an additional query. Also Avoid the `if else` logic:
```
return task_instance_schema.dump((ti, None)) # The task_instance_schema
seems a little special :)
```
If we need the `SlaMiss` and `rendered_task_instance_fields` you can add
them to your default query and update the dump call.
##########
airflow/www/static/js/api/useSetDagRunNotes.ts:
##########
@@ -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 axios from 'axios';
+import { useMutation, useQueryClient } from 'react-query';
+import { getMetaValue } from 'src/utils';
+import useErrorToast from '../utils/useErrorToast';
+import type { GridData } from './useGridData';
+import { emptyGridData } from './useGridData';
+
+const setDagRunNotesURI = getMetaValue('set_dag_run_note');
+
+export default function useSetDagRunNotes(
+ dagId: string,
+ runId: string,
+ newNotesValue: string,
+) {
+ const queryClient = useQueryClient();
+ const errorToast = useErrorToast();
+ const setDagRunNotes = setDagRunNotesURI.replace('_DAG_RUN_ID_', runId);
+
+ const updateGridData = (oldValue: GridData | undefined) => {
+ if (oldValue == null) return emptyGridData;
+ const run = oldValue.dagRuns.find((dr) => dr.runId === runId);
+ if (run) {
+ run.notes = newNotesValue;
+ }
+ return oldValue;
+ };
+
+ return useMutation(
+ ['setDagRunNotes', dagId, runId],
+ () => axios.patch(setDagRunNotes, { notes: newNotesValue }),
+ {
+
+ onSuccess: async () => {
+ // Cancel any outgoing refetches (so they don't overwrite our
optimistic update)
+ await queryClient.cancelQueries('gridData');
+ // Optimistically update to the new value
Review Comment:
Really interesting :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
