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 2021/07/12 21:10:42 UTC
[GitHub] [airflow] sharingan-no-kakashi opened a new pull request #16953: Feature: Added Task Notes
sharingan-no-kakashi opened a new pull request #16953:
URL: https://github.com/apache/airflow/pull/16953
In reference to #16790, I have worked on this "Task Note" feature.
In a nutshell, a task note is a user-provided annotation for a task instance that keeps track of Airflow-external events that are not collected in the logs.
**Model**
The model consists of (dag_id, task_id, execution_date, timestamp) that form the primary key a user_name, and a task_note field. The task not field is an unbounded collection of text.
**Table**
I have created a new table "Task notes" to store notes.
**Views**
There are two new views. The task instance details view has a new "sub view" that contains the task note and there is a new menu called "task notes".
**Task Note view**:
Users can view in a tabular fashion the notes related to that task and, if authorized, can submit a new task note.
**Task notes menu**:
Users can list all the task notes, edit them and delete them (Similar to xcom menu)
**Security**
I have introduced a new resource "Task note", viewers can list all the task notes, while users can edit/create/delete them.
(In the current implementation, users can edit other users' notes)
**Screenshot**
[Viewers without creating task note permissions](https://ibb.co/h2B6xps)
[Users with creating task note permission](https://ibb.co/CWdMBkb)
[Task note menu](https://ibb.co/VCVcrSp)
**Question**
1. It's not entirely clear to me how ` airflow/migrations/versions` works, I have just added the table creation there, but it's likely wrong.
2. Should task notes' sizes be limited?
3. Once this flow has been reviewed and validated, I will be happy to write a piece of documentation maybe `docs/apache-airflow/concepts/task-note.rst` ?
4. API endpoints are missing from this, do you think they should be added?
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r670469213
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
Review comment:
Yes, it's used in the jinja templating. The task note view extends the task instance view and it needs the dag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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 #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-881464732
Yep. I loved that one :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r669922136
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
+
+ ti = (
+ session.query(ti_db)
+ .filter(and_(ti_db.dag_id == dag_id, ti_db.task_id == task_id, ti_db.execution_date == dttm))
+ .first()
+ )
+
+ if not ti:
+ flash(f"Task [{dag_id}.{task_id}.{execution_date}] doesn't seem to exist at the moment", "error")
+ return redirect(url_for('Airflow.index'))
+
+ can_add_note = current_app.appbuilder.sm.has_access(
+ permissions.ACTION_CAN_CREATE, permissions.RESOURCE_TASK_NOTE
+ )
+
+ if request.method == 'GET':
+ notes = (
+ TaskNote.get_many(dag_ids=dag_id, task_ids=task_id, execution_date=dttm)
+ .order_by(TaskNote.timestamp.asc())
+ .all()
+ )
+
+ attributes = [(note.task_note, note.timestamp, note.user_name) for note in notes]
+
+ return self.render_template(
+ 'airflow/task_notes.html',
+ attributes=attributes,
+ task_id=task_id,
+ execution_date=execution_date,
+ form=form,
+ can_add_note=can_add_note,
+ root=root,
+ dag=dag,
+ title=title,
+ )
+
+ if request.method == 'POST':
+ if not can_add_note:
+ flash("Current user cannot add notes")
+ else:
+ TaskNote.set(
Review comment:
true, I'll take care
--
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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-882732582
> > Indeed, it's very interesting. I am missing the context though, from where does that demo come from? Is it going to be integrated into the open-source version of airflow?
>
> Context: They consider contributing some of that code to Airflow - but it is based on Airflow 1.10.5 so it might as well be easier to implement it from scratch in Airflow 2 (and take inspiration from what they've done). Sometimes reuse is on the "concept reuse" level rather than "code reuse".
>
> > OK, I have an idea: what about having a decorator similar to action_logging which is note_logger or similar, this decorator can be used in a similar fashion to action_logging but instead of updating the logs, it will, if the note if not empty, update the "note table".
>
> I think decorator in this case would not work too well. What we need to do, is to mark all the actions to be "note'able" so-to-speak. so we do not want to add decorator IMHO (which adds automated logging action) but rather we need to add reusable logic for displaying the note view in FAB and some reusable code that every action that is performed should call to store the note. I am not sure if the decorator would help here. But maybe I am not getting the idea you have :). Maybe you could put together a simple POC and prepare a draft PR where you could just implement it for one action and then we could discuss how to generalise it best, and when we agree in PR you could complete it for other views and then we could merge it?
>
> I think that would be best way of handling it.
Sounds good. I'll do 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] sharingan-no-kakashi edited a comment on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi edited a comment on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-890561270
Hi! I am sorry to be THAT guy (😄 ), but this is taking way longer than I expected, I am closing this, I am no longer interested in contributing. 👍
--
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 #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-881254173
I wonder if it is worth generalizing this to be able to attach notes to more than just Tasks -- for instance being able to say why you just cleared some tasks
(Check out https://www.crowdcast.io/e/airflowsummit2021/46 at the 43min mark for inspiration here)
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r670475229
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
+
+ ti = (
+ session.query(ti_db)
+ .filter(and_(ti_db.dag_id == dag_id, ti_db.task_id == task_id, ti_db.execution_date == dttm))
+ .first()
+ )
+
+ if not ti:
+ flash(f"Task [{dag_id}.{task_id}.{execution_date}] doesn't seem to exist at the moment", "error")
+ return redirect(url_for('Airflow.index'))
+
+ can_add_note = current_app.appbuilder.sm.has_access(
+ permissions.ACTION_CAN_CREATE, permissions.RESOURCE_TASK_NOTE
+ )
+
+ if request.method == 'GET':
+ notes = (
+ TaskNote.get_many(dag_ids=dag_id, task_ids=task_id, execution_date=dttm)
+ .order_by(TaskNote.timestamp.asc())
+ .all()
+ )
+
+ attributes = [(note.task_note, note.timestamp, note.user_name) for note in notes]
+
+ return self.render_template(
+ 'airflow/task_notes.html',
+ attributes=attributes,
+ task_id=task_id,
+ execution_date=execution_date,
+ form=form,
+ can_add_note=can_add_note,
+ root=root,
+ dag=dag,
+ title=title,
+ )
+
+ if request.method == 'POST':
+ if not can_add_note:
+ flash("Current user cannot add notes")
+ else:
+ TaskNote.set(
+ timestamp=pendulum.now(),
+ task_note=request_note,
+ user_name=str(g.user) if g.user else default_user,
Review comment:
It should indeed
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r669922301
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
Review comment:
valid point
--
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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-885272009
Hi, I opened a new draft PR #17181 😄
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r669918090
##########
File path: airflow/migrations/versions/e3a246e0dc1_current_schema.py
##########
@@ -220,6 +220,18 @@ def upgrade():
sa.PrimaryKeyConstraint('id'),
)
+ if 'task_notes' not in tables:
+ op.create_table(
+ 'task_notes',
+ sa.Column('task_id', sa.String(length=250, **COLLATION_ARGS), nullable=False),
+ sa.Column('dag_id', sa.String(length=250, **COLLATION_ARGS), nullable=False),
+ sa.Column('execution_date', sa.DateTime(), nullable=False),
+ sa.Column('timestamp', sa.DateTime(), nullable=False),
+ sa.Column('user_name', sa.String(length=250), nullable=True),
+ sa.Column('task_note', sa.Text(), nullable=True),
+ sa.PrimaryKeyConstraint('task_id', 'dag_id', 'execution_date', 'timestamp'),
+ )
+
Review comment:
Yes, I was not aware. I'll fix 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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-882732582
> > Indeed, it's very interesting. I am missing the context though, from where does that demo come from? Is it going to be integrated into the open-source version of airflow?
>
> Context: They consider contributing some of that code to Airflow - but it is based on Airflow 1.10.5 so it might as well be easier to implement it from scratch in Airflow 2 (and take inspiration from what they've done). Sometimes reuse is on the "concept reuse" level rather than "code reuse".
>
> > OK, I have an idea: what about having a decorator similar to action_logging which is note_logger or similar, this decorator can be used in a similar fashion to action_logging but instead of updating the logs, it will, if the note if not empty, update the "note table".
>
> I think decorator in this case would not work too well. What we need to do, is to mark all the actions to be "note'able" so-to-speak. so we do not want to add decorator IMHO (which adds automated logging action) but rather we need to add reusable logic for displaying the note view in FAB and some reusable code that every action that is performed should call to store the note. I am not sure if the decorator would help here. But maybe I am not getting the idea you have :). Maybe you could put together a simple POC and prepare a draft PR where you could just implement it for one action and then we could discuss how to generalise it best, and when we agree in PR you could complete it for other views and then we could merge it?
>
> I think that would be best way of handling it.
Sounds good. I'll do 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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-881588532
OK, I have an idea: what about having a decorator similar to `action_logging` which is `note_logger` or similar, this decorator can be used in a similar fashion to `action_logging` but instead of updating the logs, it will, if the note if not empty, update the "note table".
In this case, we can add in the "confirm view" a txt form at the bottom and in the post request we can update the note.
In this implementation a note can be related to anything that happens, (clear, mark success, mark failed, trigger run etc).
The Note model could refer to a dag_id, task_id and an "event".
What do you think @potiuk @ashb @uranusjr ?
--
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 #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-890591044
That's sad to hear.
--
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 #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-879378004
`airflow/migrations/versions` contains database migration files. They are managed by Alembic, and each version describes a change in the database schema in time. So to add a new model, you should create a new migration file, instead of midofying existing migrations.
You can find more about Alembic here: https://alembic.sqlalchemy.org/en/latest/tutorial.html
To create a new migration file for Airflow: https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#metadata-database-updates
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r670473336
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
Review comment:
What I have in mind is to have two "views"
- One in the menu whose endpoint is "task_notes" (screenshot here https://ibb.co/VCVcrSp) which is modeled by `TaskNoteModelView` where users can have a summary of all notes and can edit/delete/add them.
- At the same time I am proposing to extend the "Task instance" view with a task note view (endpoint /task_note) screen https://ibb.co/CWdMBkb where a user can post and get all the notes related to specific task execution. This would require a different endpoint and a different view from what I can understand. Is that not 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] potiuk edited a comment on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-882087162
> Indeed, it's very interesting. I am missing the context though, from where does that demo come from? Is it going to be integrated into the open-source version of airflow?
Context: They consider contributing some of that code to Airflow - but it is based on Airflow 1.10.5 so it might as well be easier to implement it from scratch in Airflow 2 (and take inspiration from what they've done). Sometimes reuse is on the "concept reuse" level rather than "code reuse".
> OK, I have an idea: what about having a decorator similar to action_logging which is note_logger or similar, this decorator can be used in a similar fashion to action_logging but instead of updating the logs, it will, if the note if not empty, update the "note table".
I think decorator in this case would not work too well. What we need to do, is to mark all the actions to be "note'able" so-to-speak. so we do not want to add decorator IMHO (which adds automated logging action) but rather we need to add reusable logic for displaying the note view in FAB and some reusable code that every action that is performed should call to store the note. I am not sure if the decorator would help here. But maybe I am not getting the idea you have :). Maybe you could put together a simple POC and prepare a draft PR where you could just implement it for one action and then we could discuss how to generalise it best, and when we agree in PR you could complete it for other views and then we could merge it?
I think that would be best way of handling 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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-880174592
> `airflow/migrations/versions` contains database migration files. They are managed by Alembic, and each version describes a change in the database schema in time. So to add a new model, you should create a new migration file, instead of midofying existing migrations.
>
> You can find more about Alembic here: https://alembic.sqlalchemy.org/en/latest/tutorial.html
> To create a new migration file for Airflow: https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#metadata-database-updates
Thanks, I will do 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] sharingan-no-kakashi closed pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi closed pull request #16953:
URL: https://github.com/apache/airflow/pull/16953
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r669921969
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
Review comment:
I am not sure I understand this right.
I took inspiration from XCOM which has an `XComModelView` and a dedicated `xcom` component in the views (xcom is an old component and I should not have followed that?).
Should I expose these two endpoints with two different functions in the TaskNoteModelView?
--
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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r670473336
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
Review comment:
What I hand in mind is to have two "views"
- One in the menu whose endpoint is "task_notes" (screenshot here https://ibb.co/VCVcrSp) which is modeled by `TaskNoteModelView` where users can have a summary of all notes and can edit/delete/add them.
- At the same time I am proposing to extend the "Task instance" view with a task note view (endpoint /task_note) screen https://ibb.co/CWdMBkb where a user can post and get all the notes related to specific task execution. This would require a different endpoint and a different view from what I can understand. Is that not 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] sharingan-no-kakashi commented on a change in pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r670475890
##########
File path: airflow/models/task_note.py
##########
@@ -0,0 +1,219 @@
+#
+# 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 logging
+from typing import Any, Iterable, Optional, Union
+
+import pendulum
+from sqlalchemy import Column, String, Text, and_
+from sqlalchemy.orm import Query, Session
+
+from airflow.models.base import COLLATION_ARGS, ID_LEN, Base
+from airflow.utils.helpers import is_container
+from airflow.utils.session import provide_session
+from airflow.utils.sqlalchemy import UtcDateTime
+
+log = logging.getLogger(__name__)
+
+
+class TaskNote(Base):
+ """Model that stores a note for a task id."""
+
+ __tablename__ = "task_notes"
+
+ task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ execution_date = Column(UtcDateTime, primary_key=True)
+ timestamp = Column(UtcDateTime, primary_key=True)
+ user_name = Column(String(ID_LEN))
+ task_note = Column(Text())
+
+ def __repr__(self):
+ return str(self.__key())
+
+ def __key(self):
+ return self.dag_id, self.task_id, self.execution_date, self.timestamp, self.user_name, self.task_note
+
+ def __hash__(self):
+ return hash(self.__key())
+
+ def __eq__(self, other):
+ if isinstance(other, TaskNote):
+ return self.__key() == other.__key()
+ return False
Review comment:
not AFAIK, but i'll double check!
##########
File path: airflow/models/task_note.py
##########
@@ -0,0 +1,219 @@
+#
+# 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 logging
+from typing import Any, Iterable, Optional, Union
+
+import pendulum
+from sqlalchemy import Column, String, Text, and_
+from sqlalchemy.orm import Query, Session
+
+from airflow.models.base import COLLATION_ARGS, ID_LEN, Base
+from airflow.utils.helpers import is_container
+from airflow.utils.session import provide_session
+from airflow.utils.sqlalchemy import UtcDateTime
+
+log = logging.getLogger(__name__)
+
+
+class TaskNote(Base):
+ """Model that stores a note for a task id."""
+
+ __tablename__ = "task_notes"
+
+ task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ execution_date = Column(UtcDateTime, primary_key=True)
+ timestamp = Column(UtcDateTime, primary_key=True)
+ user_name = Column(String(ID_LEN))
+ task_note = Column(Text())
+
+ def __repr__(self):
+ return str(self.__key())
+
+ def __key(self):
+ return self.dag_id, self.task_id, self.execution_date, self.timestamp, self.user_name, self.task_note
+
+ def __hash__(self):
+ return hash(self.__key())
+
+ def __eq__(self, other):
+ if isinstance(other, TaskNote):
+ return self.__key() == other.__key()
+ return False
+
+ @classmethod
+ @provide_session
+ def set(cls, task_note, timestamp, user_name, execution_date, task_id, dag_id, session=None):
+ """
+ Store a TaskNote
+ :return: None
+ """
+ session.expunge_all()
+
+ # remove any duplicate TaskNote
+ session.query(cls).filter(
+ cls.execution_date == execution_date,
+ cls.user_name == user_name,
+ cls.task_id == task_id,
+ cls.dag_id == dag_id,
+ cls.timestamp == timestamp,
+ ).delete()
+
+ # insert new TaskNote
+ session.add(
+ TaskNote(
+ timestamp=timestamp,
+ task_note=task_note,
+ user_name=user_name,
+ execution_date=execution_date,
+ task_id=task_id,
+ dag_id=dag_id,
+ )
+ )
+
+ session.commit()
+
+ @classmethod
+ @provide_session
+ def delete(cls, notes, session=None):
+ """Delete TaskNote"""
+ if isinstance(notes, TaskNote):
+ notes = [notes]
+ for note in notes:
+ if not isinstance(note, TaskNote):
+ raise TypeError(f'Expected TaskNote; received {note.__class__.__name__}')
+ session.delete(note)
+ session.commit()
Review comment:
True.
--
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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-882732582
> > Indeed, it's very interesting. I am missing the context though, from where does that demo come from? Is it going to be integrated into the open-source version of airflow?
>
> Context: They consider contributing some of that code to Airflow - but it is based on Airflow 1.10.5 so it might as well be easier to implement it from scratch in Airflow 2 (and take inspiration from what they've done). Sometimes reuse is on the "concept reuse" level rather than "code reuse".
>
> > OK, I have an idea: what about having a decorator similar to action_logging which is note_logger or similar, this decorator can be used in a similar fashion to action_logging but instead of updating the logs, it will, if the note if not empty, update the "note table".
>
> I think decorator in this case would not work too well. What we need to do, is to mark all the actions to be "note'able" so-to-speak. so we do not want to add decorator IMHO (which adds automated logging action) but rather we need to add reusable logic for displaying the note view in FAB and some reusable code that every action that is performed should call to store the note. I am not sure if the decorator would help here. But maybe I am not getting the idea you have :). Maybe you could put together a simple POC and prepare a draft PR where you could just implement it for one action and then we could discuss how to generalise it best, and when we agree in PR you could complete it for other views and then we could merge it?
>
> I think that would be best way of handling it.
Sounds good. I'll do 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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-881529125
> I wonder if it is worth generalizing this to be able to attach notes to more than just Tasks -- for instance being able to say why you just cleared some tasks
>
> (Check out https://www.crowdcast.io/e/airflowsummit2021/46 at the 43min mark for inspiration here)
Indeed, it's very interesting. I am missing the context though, from where does that demo come from? Is it going to be integrated into the open-source version of airflow?
--
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 #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#discussion_r669412887
##########
File path: airflow/models/task_note.py
##########
@@ -0,0 +1,219 @@
+#
+# 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 logging
+from typing import Any, Iterable, Optional, Union
+
+import pendulum
+from sqlalchemy import Column, String, Text, and_
+from sqlalchemy.orm import Query, Session
+
+from airflow.models.base import COLLATION_ARGS, ID_LEN, Base
+from airflow.utils.helpers import is_container
+from airflow.utils.session import provide_session
+from airflow.utils.sqlalchemy import UtcDateTime
+
+log = logging.getLogger(__name__)
+
+
+class TaskNote(Base):
+ """Model that stores a note for a task id."""
+
+ __tablename__ = "task_notes"
+
+ task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ execution_date = Column(UtcDateTime, primary_key=True)
+ timestamp = Column(UtcDateTime, primary_key=True)
+ user_name = Column(String(ID_LEN))
+ task_note = Column(Text())
+
+ def __repr__(self):
+ return str(self.__key())
+
+ def __key(self):
+ return self.dag_id, self.task_id, self.execution_date, self.timestamp, self.user_name, self.task_note
+
+ def __hash__(self):
+ return hash(self.__key())
+
+ def __eq__(self, other):
+ if isinstance(other, TaskNote):
+ return self.__key() == other.__key()
+ return False
Review comment:
Do we need these? Doesn't SQLA Base give us some of these already?
##########
File path: airflow/migrations/versions/e3a246e0dc1_current_schema.py
##########
@@ -220,6 +220,18 @@ def upgrade():
sa.PrimaryKeyConstraint('id'),
)
+ if 'task_notes' not in tables:
+ op.create_table(
+ 'task_notes',
+ sa.Column('task_id', sa.String(length=250, **COLLATION_ARGS), nullable=False),
+ sa.Column('dag_id', sa.String(length=250, **COLLATION_ARGS), nullable=False),
+ sa.Column('execution_date', sa.DateTime(), nullable=False),
+ sa.Column('timestamp', sa.DateTime(), nullable=False),
+ sa.Column('user_name', sa.String(length=250), nullable=True),
+ sa.Column('task_note', sa.Text(), nullable=True),
+ sa.PrimaryKeyConstraint('task_id', 'dag_id', 'execution_date', 'timestamp'),
+ )
+
Review comment:
You will need to create a new migration instead of editing the existing one -- when upgrading existing installs old migrations are not re-run.
##########
File path: airflow/models/task_note.py
##########
@@ -0,0 +1,219 @@
+#
+# 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 logging
+from typing import Any, Iterable, Optional, Union
+
+import pendulum
+from sqlalchemy import Column, String, Text, and_
+from sqlalchemy.orm import Query, Session
+
+from airflow.models.base import COLLATION_ARGS, ID_LEN, Base
+from airflow.utils.helpers import is_container
+from airflow.utils.session import provide_session
+from airflow.utils.sqlalchemy import UtcDateTime
+
+log = logging.getLogger(__name__)
+
+
+class TaskNote(Base):
+ """Model that stores a note for a task id."""
+
+ __tablename__ = "task_notes"
+
+ task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ execution_date = Column(UtcDateTime, primary_key=True)
+ timestamp = Column(UtcDateTime, primary_key=True)
+ user_name = Column(String(ID_LEN))
+ task_note = Column(Text())
+
+ def __repr__(self):
+ return str(self.__key())
+
+ def __key(self):
+ return self.dag_id, self.task_id, self.execution_date, self.timestamp, self.user_name, self.task_note
+
+ def __hash__(self):
+ return hash(self.__key())
+
+ def __eq__(self, other):
+ if isinstance(other, TaskNote):
+ return self.__key() == other.__key()
+ return False
+
+ @classmethod
+ @provide_session
+ def set(cls, task_note, timestamp, user_name, execution_date, task_id, dag_id, session=None):
+ """
+ Store a TaskNote
+ :return: None
+ """
+ session.expunge_all()
+
+ # remove any duplicate TaskNote
+ session.query(cls).filter(
+ cls.execution_date == execution_date,
+ cls.user_name == user_name,
+ cls.task_id == task_id,
+ cls.dag_id == dag_id,
+ cls.timestamp == timestamp,
+ ).delete()
+
+ # insert new TaskNote
+ session.add(
+ TaskNote(
+ timestamp=timestamp,
+ task_note=task_note,
+ user_name=user_name,
+ execution_date=execution_date,
+ task_id=task_id,
+ dag_id=dag_id,
+ )
+ )
+
+ session.commit()
+
+ @classmethod
+ @provide_session
+ def delete(cls, notes, session=None):
+ """Delete TaskNote"""
+ if isinstance(notes, TaskNote):
+ notes = [notes]
+ for note in notes:
+ if not isinstance(note, TaskNote):
+ raise TypeError(f'Expected TaskNote; received {note.__class__.__name__}')
+ session.delete(note)
+ session.commit()
+
+ @classmethod
+ @provide_session
+ def get_one(
+ cls,
+ execution_date: pendulum.DateTime,
+ timestamp: pendulum.DateTime,
+ user_names: Optional[Union[str, Iterable[str]]] = None,
+ task_ids: Optional[Union[str, Iterable[str]]] = None,
+ dag_ids: Optional[Union[str, Iterable[str]]] = None,
+ session: Session = None,
+ ) -> Optional[Any]:
+ """
+ Retrieve a TaskNote value, optionally meeting certain criteria. Returns None
+ of there are no results.
+
+ :param execution_date: Execution date for the task
+ :type execution_date: pendulum.datetime
+ :param task_ids: Only TaskNotes from task with matching id will be
+ pulled. Can pass None to remove the filter.
+ :type task_ids: str
+ :param dag_ids: If provided, only pulls TaskNote from this DAG.
+ If None (default), the DAG of the calling task is used.
+ :type dag_ids str or iterable of strings (representing dag ids)
+ :param timestamp: Timestamp of the TaskNote creation
+ :type timestamp: pendulum.datetime
+ :param user_names: If provided, only pulls TaskNote from these users
+ :type user_names: str or iterable of strings (representing usernames)
+ :type dag_ids: str
+ :param session: database session
+ :type session: sqlalchemy.orm.session.Session
+ """
+ return cls.get_many(
+ execution_date=execution_date,
+ timestamp=timestamp,
+ user_names=user_names,
+ task_ids=task_ids,
+ dag_ids=dag_ids,
+ session=session,
+ limit=1,
Review comment:
```suggestion
```
`first()` does `limit 1` for us
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
+
+ ti = (
+ session.query(ti_db)
+ .filter(and_(ti_db.dag_id == dag_id, ti_db.task_id == task_id, ti_db.execution_date == dttm))
+ .first()
+ )
+
+ if not ti:
+ flash(f"Task [{dag_id}.{task_id}.{execution_date}] doesn't seem to exist at the moment", "error")
+ return redirect(url_for('Airflow.index'))
+
+ can_add_note = current_app.appbuilder.sm.has_access(
+ permissions.ACTION_CAN_CREATE, permissions.RESOURCE_TASK_NOTE
+ )
+
+ if request.method == 'GET':
+ notes = (
+ TaskNote.get_many(dag_ids=dag_id, task_ids=task_id, execution_date=dttm)
+ .order_by(TaskNote.timestamp.asc())
+ .all()
+ )
+
+ attributes = [(note.task_note, note.timestamp, note.user_name) for note in notes]
+
+ return self.render_template(
+ 'airflow/task_notes.html',
+ attributes=attributes,
+ task_id=task_id,
+ execution_date=execution_date,
+ form=form,
+ can_add_note=can_add_note,
+ root=root,
+ dag=dag,
+ title=title,
+ )
+
+ if request.method == 'POST':
+ if not can_add_note:
+ flash("Current user cannot add notes")
+ else:
+ TaskNote.set(
Review comment:
If you just submit an empty form (i.e. no note) it would still be written to the DB -- you should add some validation of the input.
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
Review comment:
If you are creating a TaskNoteModelView then we shouldn't have this method here -- but use the model view. Having both is wrong.
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
+
+ ti = (
+ session.query(ti_db)
+ .filter(and_(ti_db.dag_id == dag_id, ti_db.task_id == task_id, ti_db.execution_date == dttm))
+ .first()
+ )
+
+ if not ti:
+ flash(f"Task [{dag_id}.{task_id}.{execution_date}] doesn't seem to exist at the moment", "error")
+ return redirect(url_for('Airflow.index'))
+
+ can_add_note = current_app.appbuilder.sm.has_access(
+ permissions.ACTION_CAN_CREATE, permissions.RESOURCE_TASK_NOTE
+ )
Review comment:
Make post handler a separate method, and then let the permissions decorator handle permissions for us.
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
Review comment:
Is this ever used?
##########
File path: airflow/models/task_note.py
##########
@@ -0,0 +1,219 @@
+#
+# 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 logging
+from typing import Any, Iterable, Optional, Union
+
+import pendulum
+from sqlalchemy import Column, String, Text, and_
+from sqlalchemy.orm import Query, Session
+
+from airflow.models.base import COLLATION_ARGS, ID_LEN, Base
+from airflow.utils.helpers import is_container
+from airflow.utils.session import provide_session
+from airflow.utils.sqlalchemy import UtcDateTime
+
+log = logging.getLogger(__name__)
+
+
+class TaskNote(Base):
+ """Model that stores a note for a task id."""
+
+ __tablename__ = "task_notes"
+
+ task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ execution_date = Column(UtcDateTime, primary_key=True)
+ timestamp = Column(UtcDateTime, primary_key=True)
+ user_name = Column(String(ID_LEN))
+ task_note = Column(Text())
+
+ def __repr__(self):
+ return str(self.__key())
+
+ def __key(self):
+ return self.dag_id, self.task_id, self.execution_date, self.timestamp, self.user_name, self.task_note
+
+ def __hash__(self):
+ return hash(self.__key())
+
+ def __eq__(self, other):
+ if isinstance(other, TaskNote):
+ return self.__key() == other.__key()
+ return False
+
+ @classmethod
+ @provide_session
+ def set(cls, task_note, timestamp, user_name, execution_date, task_id, dag_id, session=None):
+ """
+ Store a TaskNote
+ :return: None
+ """
+ session.expunge_all()
Review comment:
```suggestion
```
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
+ request_note = request.values.get('note')
+ default_user = "Anonymous"
+ title = "Notes"
+
+ dag = session.query(dm_db).filter(dm_db.dag_id == dag_id).first()
+
+ ti = (
+ session.query(ti_db)
+ .filter(and_(ti_db.dag_id == dag_id, ti_db.task_id == task_id, ti_db.execution_date == dttm))
+ .first()
+ )
+
+ if not ti:
+ flash(f"Task [{dag_id}.{task_id}.{execution_date}] doesn't seem to exist at the moment", "error")
+ return redirect(url_for('Airflow.index'))
+
+ can_add_note = current_app.appbuilder.sm.has_access(
+ permissions.ACTION_CAN_CREATE, permissions.RESOURCE_TASK_NOTE
+ )
+
+ if request.method == 'GET':
+ notes = (
+ TaskNote.get_many(dag_ids=dag_id, task_ids=task_id, execution_date=dttm)
+ .order_by(TaskNote.timestamp.asc())
+ .all()
+ )
+
+ attributes = [(note.task_note, note.timestamp, note.user_name) for note in notes]
+
+ return self.render_template(
+ 'airflow/task_notes.html',
+ attributes=attributes,
+ task_id=task_id,
+ execution_date=execution_date,
+ form=form,
+ can_add_note=can_add_note,
+ root=root,
+ dag=dag,
+ title=title,
+ )
+
+ if request.method == 'POST':
+ if not can_add_note:
+ flash("Current user cannot add notes")
+ else:
+ TaskNote.set(
+ timestamp=pendulum.now(),
+ task_note=request_note,
+ user_name=str(g.user) if g.user else default_user,
Review comment:
This shouldn't be necessary -- there's always got to be a user here right?
##########
File path: airflow/www/views.py
##########
@@ -1354,6 +1356,82 @@ def xcom(self, session=None):
title=title,
)
+ @expose('/task_note', methods=['POST', 'GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_INSTANCE),
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_TASK_NOTE),
+ ]
+ )
+ @action_logging
+ @provide_session
+ def task_note(self, session=None):
+ """Retrieve and store task notes"""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ dttm = timezone.parse(execution_date)
+ form = DateTimeForm(data={'execution_date': dttm})
+ root = request.args.get('root', '')
+ dm_db = models.DagModel
+ ti_db = models.TaskInstance
Review comment:
```suggestion
DM = models.DagModel
TI = models.TaskInstance
```
to follow the convention
##########
File path: airflow/models/task_note.py
##########
@@ -0,0 +1,219 @@
+#
+# 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 logging
+from typing import Any, Iterable, Optional, Union
+
+import pendulum
+from sqlalchemy import Column, String, Text, and_
+from sqlalchemy.orm import Query, Session
+
+from airflow.models.base import COLLATION_ARGS, ID_LEN, Base
+from airflow.utils.helpers import is_container
+from airflow.utils.session import provide_session
+from airflow.utils.sqlalchemy import UtcDateTime
+
+log = logging.getLogger(__name__)
+
+
+class TaskNote(Base):
+ """Model that stores a note for a task id."""
+
+ __tablename__ = "task_notes"
+
+ task_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ dag_id = Column(String(ID_LEN, **COLLATION_ARGS), primary_key=True)
+ execution_date = Column(UtcDateTime, primary_key=True)
+ timestamp = Column(UtcDateTime, primary_key=True)
+ user_name = Column(String(ID_LEN))
+ task_note = Column(Text())
+
+ def __repr__(self):
+ return str(self.__key())
+
+ def __key(self):
+ return self.dag_id, self.task_id, self.execution_date, self.timestamp, self.user_name, self.task_note
+
+ def __hash__(self):
+ return hash(self.__key())
+
+ def __eq__(self, other):
+ if isinstance(other, TaskNote):
+ return self.__key() == other.__key()
+ return False
+
+ @classmethod
+ @provide_session
+ def set(cls, task_note, timestamp, user_name, execution_date, task_id, dag_id, session=None):
+ """
+ Store a TaskNote
+ :return: None
+ """
+ session.expunge_all()
+
+ # remove any duplicate TaskNote
+ session.query(cls).filter(
+ cls.execution_date == execution_date,
+ cls.user_name == user_name,
+ cls.task_id == task_id,
+ cls.dag_id == dag_id,
+ cls.timestamp == timestamp,
+ ).delete()
+
+ # insert new TaskNote
+ session.add(
+ TaskNote(
+ timestamp=timestamp,
+ task_note=task_note,
+ user_name=user_name,
+ execution_date=execution_date,
+ task_id=task_id,
+ dag_id=dag_id,
+ )
+ )
+
+ session.commit()
+
+ @classmethod
+ @provide_session
+ def delete(cls, notes, session=None):
+ """Delete TaskNote"""
+ if isinstance(notes, TaskNote):
+ notes = [notes]
+ for note in notes:
+ if not isinstance(note, TaskNote):
+ raise TypeError(f'Expected TaskNote; received {note.__class__.__name__}')
+ session.delete(note)
+ session.commit()
Review comment:
This function dcoesn't seem all that necessary.
--
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] sharingan-no-kakashi commented on pull request #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
sharingan-no-kakashi commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-890561270
Hi! I am sorry to THAT guy (😄 ), but this is taking way longer than I expected, I am closing this, I am no longer interested in contributing. 👍
--
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 #16953: Feature: Added Task Notes
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16953:
URL: https://github.com/apache/airflow/pull/16953#issuecomment-882087162
> Indeed, it's very interesting. I am missing the context though, from where does that demo come from? Is it going to be integrated into the open-source version of airflow?
Context: They consider contributing some of that code to Airflow - but it is based on Airflow 1.10.5 so it might as well be easier to implement it from scratch in 2.0 (and take inspiration from what they've done). Sometimes reuse is on the "concept reuse" level rather than "code reuse".
> OK, I have an idea: what about having a decorator similar to action_logging which is note_logger or similar, this decorator can be used in a similar fashion to action_logging but instead of updating the logs, it will, if the note if not empty, update the "note table".
I think decorator in this case would not work too well. What we need to do, is to mark all the actions to be "note'able" so-to-speak. so we do not want to add decorator IMHO (which adds automated logging action) but rather we need to add reusable logic for displaying the note view in FAB and some reusable code that every action that is performed should call to store the note. I am not sure if the decorator would help here. But maybe I am not getting the idea you have :). Maybe you could put together a simple POC and prepare a draft PR where you could just implement it for one action and then we could discuss how to generalise it best, and when we agree in PR you could complete it for other views and then we could merge it?
I think that would be best way of handling 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