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/06/03 05:42:05 UTC
[GitHub] [airflow] msumit opened a new pull request #16233: WIP: Fix TI success/failure links
msumit opened a new pull request #16233:
URL: https://github.com/apache/airflow/pull/16233
This is to fix the issue https://github.com/apache/airflow/issues/15234.
As of now, TI success & failure endpoints are POST only and behave differently as per the `confirmed` flag. They either render a confirmation page or updates the TI states on the basis of that flag, something which is not a great design.
Also, as these endpoints are POST only, they throw a 404 error when someone clicks on the link received via email.
To fix the issue, extracting the rendering functionalities into a diff endpoint `/confirm` & keeping these endpoints as pure POST endpoints.
If the approach looks good, I can work on testcases later.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649126518
##########
File path: airflow/www/templates/airflow/confirm.html
##########
@@ -28,11 +28,17 @@ <h2>Wait a minute.</h2>
<pre><code>{{ details }}</code></pre>
{% endif %}
</div>
- <form method="POST">
+ {% if endpoint %}
+ <form method="POST" action="/{{ endpoint }}">
+ {% else %}
+ <form method="POST">
+ {% endif %}
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}"/>
<input type="hidden" name="confirmed" value="true">
- {% for name,val in request.form.items() if name != "csrf_token" %}
- <input type="hidden" name="{{ name }}" value="{{ val }}">
+ {% for args_gen_dict in (request.args.items(), request.form.items()) %}
Review comment:
https://flask.palletsprojects.com/en/1.1.x/api/#flask.Request.values is both of them combined in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648864028
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
Ok, will do it into the same file, but have to use if-else..
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648559782
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -317,12 +317,12 @@ <h4>Task Actions</h4>
</div>
</form>
<hr style="margin-bottom: 8px;">
- <form method="POST" data-action="{{ url_for('Airflow.success') }}">
- <input type="hidden" name="csrf_token" value="{{ csrf_token() }}">
Review comment:
Why did we remove this? I'm running the branch locally and getting a `Bad Request The CSRF token is missing.` error when marking a dag run as success or failure. Task instances work fine.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649128969
##########
File path: airflow/www/templates/airflow/confirm.html
##########
@@ -28,11 +28,17 @@ <h2>Wait a minute.</h2>
<pre><code>{{ details }}</code></pre>
{% endif %}
</div>
- <form method="POST">
+ {% if endpoint %}
+ <form method="POST" action="/{{ endpoint }}">
Review comment:
This won't work if Airflow is not rooted at `/` -- endpoint should converted to a URL by the view function using `url_for` and the template can then just print 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-855933229
Yeah. I believe this is something of the AWS infrastructure for runners which we are going to replace (I hope this week) with GCP one - simplified and improved.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649131608
##########
File path: airflow/www/views.py
##########
@@ -1810,47 +1810,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
from airflow.api.common.experimental.mark_tasks import set_state
- if confirmed:
- with create_session() as session:
- altered = set_state(
- tasks=[task],
- execution_date=execution_date,
- upstream=upstream,
- downstream=downstream,
- future=future,
- past=past,
- state=state,
- commit=True,
- session=session,
- )
+ with create_session() as session:
+ altered = set_state(
+ tasks=[task],
+ execution_date=execution_date,
+ upstream=upstream,
+ downstream=downstream,
+ future=future,
+ past=past,
+ state=state,
+ commit=True,
+ session=session,
+ )
- # Clear downstream tasks that are in failed/upstream_failed state to resume them.
- # Flush the session so that the tasks marked success are reflected in the db.
- session.flush()
- subdag = dag.partial_subset(
- task_ids_or_regex={task_id},
- include_downstream=True,
- include_upstream=False,
- )
+ # Clear downstream tasks that are in failed/upstream_failed state to resume them.
+ # Flush the session so that the tasks marked success are reflected in the db.
+ session.flush()
+ subdag = dag.partial_subset(
+ task_ids_or_regex={task_id},
+ include_downstream=True,
+ include_upstream=False,
+ )
- end_date = execution_date if not future else None
- start_date = execution_date if not past else None
-
- subdag.clear(
- start_date=start_date,
- end_date=end_date,
- include_subdags=True,
- include_parentdag=True,
- only_failed=True,
- session=session,
- # Exclude the task itself from being cleared
- exclude_task_ids={task_id},
- )
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
- session.commit()
+ subdag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=True,
+ include_parentdag=True,
+ only_failed=True,
+ session=session,
+ # Exclude the task itself from being cleared
+ exclude_task_ids={task_id},
+ )
- flash(f"Marked {state} on {len(altered)} task instances")
- return redirect(origin)
+ session.commit()
+
+ flash(f"Marked {state} on {len(altered)} task instances")
+ return redirect(origin)
+
+ @expose('/confirm', methods=['GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE),
+ ]
+ )
+ @action_logging
+ def confirm(self):
+ """Show confirmation page for marking tasks as success or failed."""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ state = request.args.get('state')
+
+ upstream = request.args.get('failed_upstream') == "true"
+ downstream = request.args.get('failed_downstream') == "true"
+ future = request.args.get('failed_future') == "true"
+ past = request.args.get('failed_past') == "true"
Review comment:
Let's use `airflow.utils.strings.to_boolean()` function here.
##########
File path: airflow/www/views.py
##########
@@ -1810,47 +1810,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
from airflow.api.common.experimental.mark_tasks import set_state
- if confirmed:
- with create_session() as session:
- altered = set_state(
- tasks=[task],
- execution_date=execution_date,
- upstream=upstream,
- downstream=downstream,
- future=future,
- past=past,
- state=state,
- commit=True,
- session=session,
- )
+ with create_session() as session:
+ altered = set_state(
+ tasks=[task],
+ execution_date=execution_date,
+ upstream=upstream,
+ downstream=downstream,
+ future=future,
+ past=past,
+ state=state,
+ commit=True,
+ session=session,
+ )
- # Clear downstream tasks that are in failed/upstream_failed state to resume them.
- # Flush the session so that the tasks marked success are reflected in the db.
- session.flush()
- subdag = dag.partial_subset(
- task_ids_or_regex={task_id},
- include_downstream=True,
- include_upstream=False,
- )
+ # Clear downstream tasks that are in failed/upstream_failed state to resume them.
+ # Flush the session so that the tasks marked success are reflected in the db.
+ session.flush()
+ subdag = dag.partial_subset(
+ task_ids_or_regex={task_id},
+ include_downstream=True,
+ include_upstream=False,
+ )
- end_date = execution_date if not future else None
- start_date = execution_date if not past else None
-
- subdag.clear(
- start_date=start_date,
- end_date=end_date,
- include_subdags=True,
- include_parentdag=True,
- only_failed=True,
- session=session,
- # Exclude the task itself from being cleared
- exclude_task_ids={task_id},
- )
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
- session.commit()
+ subdag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=True,
+ include_parentdag=True,
+ only_failed=True,
+ session=session,
+ # Exclude the task itself from being cleared
+ exclude_task_ids={task_id},
+ )
- flash(f"Marked {state} on {len(altered)} task instances")
- return redirect(origin)
+ session.commit()
+
+ flash(f"Marked {state} on {len(altered)} task instances")
+ return redirect(origin)
+
+ @expose('/confirm', methods=['GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE),
+ ]
+ )
+ @action_logging
+ def confirm(self):
+ """Show confirmation page for marking tasks as success or failed."""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ state = request.args.get('state')
+
+ upstream = request.args.get('failed_upstream') == "true"
+ downstream = request.args.get('failed_downstream') == "true"
+ future = request.args.get('failed_future') == "true"
+ past = request.args.get('failed_past') == "true"
+
+ dag = current_app.dag_bag.get_dag(dag_id)
+ task = dag.get_task(task_id)
+ task.dag = dag
+
+ if state not in (
+ 'success',
+ 'failed',
+ ):
+ flash(f"Invalid state {state}, must be either 'success' or 'failed'", "error")
+ return redirect(request.referrer or url_for('Airflow.index'))
+
+ latest_execution_date = dag.get_latest_execution_date()
+ if not latest_execution_date:
+ flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
Review comment:
```suggestion
flash(f"Cannot mark tasks as {state}, seem that dag {dag_id} has never run", "error")
```
##########
File path: airflow/www/views.py
##########
@@ -1810,47 +1810,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
from airflow.api.common.experimental.mark_tasks import set_state
- if confirmed:
- with create_session() as session:
- altered = set_state(
- tasks=[task],
- execution_date=execution_date,
- upstream=upstream,
- downstream=downstream,
- future=future,
- past=past,
- state=state,
- commit=True,
- session=session,
- )
+ with create_session() as session:
+ altered = set_state(
+ tasks=[task],
+ execution_date=execution_date,
+ upstream=upstream,
+ downstream=downstream,
+ future=future,
+ past=past,
+ state=state,
+ commit=True,
+ session=session,
+ )
- # Clear downstream tasks that are in failed/upstream_failed state to resume them.
- # Flush the session so that the tasks marked success are reflected in the db.
- session.flush()
- subdag = dag.partial_subset(
- task_ids_or_regex={task_id},
- include_downstream=True,
- include_upstream=False,
- )
+ # Clear downstream tasks that are in failed/upstream_failed state to resume them.
+ # Flush the session so that the tasks marked success are reflected in the db.
+ session.flush()
+ subdag = dag.partial_subset(
+ task_ids_or_regex={task_id},
+ include_downstream=True,
+ include_upstream=False,
+ )
- end_date = execution_date if not future else None
- start_date = execution_date if not past else None
-
- subdag.clear(
- start_date=start_date,
- end_date=end_date,
- include_subdags=True,
- include_parentdag=True,
- only_failed=True,
- session=session,
- # Exclude the task itself from being cleared
- exclude_task_ids={task_id},
- )
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
- session.commit()
+ subdag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=True,
+ include_parentdag=True,
+ only_failed=True,
+ session=session,
+ # Exclude the task itself from being cleared
+ exclude_task_ids={task_id},
+ )
- flash(f"Marked {state} on {len(altered)} task instances")
- return redirect(origin)
+ session.commit()
+
+ flash(f"Marked {state} on {len(altered)} task instances")
+ return redirect(origin)
+
+ @expose('/confirm', methods=['GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE),
+ ]
+ )
+ @action_logging
+ def confirm(self):
+ """Show confirmation page for marking tasks as success or failed."""
+ dag_id = request.args.get('dag_id')
+ task_id = request.args.get('task_id')
+ execution_date = request.args.get('execution_date')
+ state = request.args.get('state')
+
+ upstream = request.args.get('failed_upstream') == "true"
+ downstream = request.args.get('failed_downstream') == "true"
+ future = request.args.get('failed_future') == "true"
+ past = request.args.get('failed_past') == "true"
+
+ dag = current_app.dag_bag.get_dag(dag_id)
Review comment:
Dag could be not found here -- that should return a 400/404 if so.
Ditto for task not found in dag.
##########
File path: airflow/www/views.py
##########
@@ -1867,6 +1906,7 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
response = self.render_template(
"airflow/confirm.html",
+ endpoint=state,
Review comment:
```suggestion
endpoint=url_for(f'Airflow.{state}'),
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-853625608
The approach sounds good to me. I have some opinions about the implementation (naming and error-handling), but I assume they can be discussed later.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-859382744
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648549288
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
I still don't see why we need this separate template - changing the other one to request.args should be safe, and then we don't have a second one.
(Is confirm even used anywhere else?)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644621780
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
What do we need a special confirm template for? (How is this different from our current confirm.html template?)
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
Referrer is not always sent (some AV blocks it for instance) so we can't just do this.
I also wonder if this makes it function as an open redirector of sorts
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
Nope, referrer can still be blocked
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r654405547
##########
File path: airflow/www/views.py
##########
@@ -1921,23 +1972,22 @@ def failed(self):
@action_logging
def success(self):
"""Mark task as success."""
- dag_id = request.form.get('dag_id')
- task_id = request.form.get('task_id')
- origin = get_safe_url(request.form.get('origin'))
- execution_date = request.form.get('execution_date')
+ args = request.form
+ dag_id = args.get('dag_id')
+ task_id = args.get('task_id')
+ origin = get_safe_url(args.get('origin'))
+ execution_date = args.get('execution_date')
- confirmed = request.form.get('confirmed') == "true"
- upstream = request.form.get('success_upstream') == "true"
- downstream = request.form.get('success_downstream') == "true"
- future = request.form.get('success_future') == "true"
- past = request.form.get('success_past') == "true"
+ upstream = to_boolean(args.get('failed_upstream'))
Review comment:
Thanks for catching this @yuqian90.. opened the fix for this -> https://github.com/apache/airflow/pull/16524
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-853625608
The approach sounds good to me. I have some opinions about the implementation (naming and error-handling), but I assume they can be discussed later.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644621780
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
What do we need a special confirm template for? (How is this different from our current confirm.html template?)
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
Referrer is not always sent (some AV blocks it for instance) so we can't just do this.
I also wonder if this makes it function as an open redirector of sorts
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-855789149
@potiuk this `CI Build / Wait for CI images` check is very unstable and always fails. Can we do something about 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644637354
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
@ashb this is inside the POST call, so the only way to hit this endpoint via the confirm page.. so in that case, the referrer should be always available?
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
1 major change - provision for the action endpoint i.e. `success` or `failed`
1 minor change - got rid of hideen confirmation input., though no harm in keeping it.
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
sorry, another major change is that the iteration is happening on `request.args.items` instead of `request.form.items`
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
Ok, brought the `origin` back. In case someone is clicking it via an email link, then after the operation, it loads the home page, which is fine in my opinion.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648559782
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -317,12 +317,12 @@ <h4>Task Actions</h4>
</div>
</form>
<hr style="margin-bottom: 8px;">
- <form method="POST" data-action="{{ url_for('Airflow.success') }}">
- <input type="hidden" name="csrf_token" value="{{ csrf_token() }}">
Review comment:
Why did we remove this? I'm running the branch locally and getting a `Bad Request The CSRF token is missing.` error when marking a dag run as success or failure. Task instances work fine.
Edit: I commented on the wrong line. See Kaxil's comment 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644692702
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
Nope, referrer can still be blocked
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644637354
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
@ashb this is inside the POST call, so the only way to hit this endpoint via the confirm page.. so in that case, the referrer should be always available?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644743697
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
sorry, another major change is that the iteration is happening on `request.args.items` instead of `request.form.items`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-856404610
Thanks @kaxil
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649126518
##########
File path: airflow/www/templates/airflow/confirm.html
##########
@@ -28,11 +28,17 @@ <h2>Wait a minute.</h2>
<pre><code>{{ details }}</code></pre>
{% endif %}
</div>
- <form method="POST">
+ {% if endpoint %}
+ <form method="POST" action="/{{ endpoint }}">
+ {% else %}
+ <form method="POST">
+ {% endif %}
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}"/>
<input type="hidden" name="confirmed" value="true">
- {% for name,val in request.form.items() if name != "csrf_token" %}
- <input type="hidden" name="{{ name }}" value="{{ val }}">
+ {% for args_gen_dict in (request.args.items(), request.form.items()) %}
Review comment:
https://flask.palletsprojects.com/en/2.0.x/api/?highlight=request#flask.Request.values is both of them combined in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644638689
##########
File path: airflow/www/templates/airflow/ti_confirm.html
##########
@@ -0,0 +1,39 @@
+{#
+ 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.
+#}
+
+{% extends base_template %}
+
+{% block content %}
Review comment:
1 major change - provision for the action endpoint i.e. `success` or `failed`
1 minor change - got rid of hideen confirmation input., though no harm in keeping 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648876039
##########
File path: airflow/www/templates/airflow/confirm.html
##########
@@ -28,11 +28,17 @@ <h2>Wait a minute.</h2>
<pre><code>{{ details }}</code></pre>
{% endif %}
</div>
- <form method="POST">
+ {% if endpoint %}
+ <form method="POST" action="/{{ endpoint }}">
+ {% else %}
+ <form method="POST">
+ {% endif %}
<input type="hidden" name="csrf_token" value="{{ csrf_token() }}"/>
<input type="hidden" name="confirmed" value="true">
- {% for name,val in request.form.items() if name != "csrf_token" %}
- <input type="hidden" name="{{ name }}" value="{{ val }}">
+ {% for args_gen_dict in (request.args.items(), request.form.items()) %}
Review comment:
@ashb `request.args != request.form`, so looping over both of them. Is there any other better way you can think of?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] yuqian90 commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
yuqian90 commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r654327148
##########
File path: airflow/www/views.py
##########
@@ -1921,23 +1972,22 @@ def failed(self):
@action_logging
def success(self):
"""Mark task as success."""
- dag_id = request.form.get('dag_id')
- task_id = request.form.get('task_id')
- origin = get_safe_url(request.form.get('origin'))
- execution_date = request.form.get('execution_date')
+ args = request.form
+ dag_id = args.get('dag_id')
+ task_id = args.get('task_id')
+ origin = get_safe_url(args.get('origin'))
+ execution_date = args.get('execution_date')
- confirmed = request.form.get('confirmed') == "true"
- upstream = request.form.get('success_upstream') == "true"
- downstream = request.form.get('success_downstream') == "true"
- future = request.form.get('success_future') == "true"
- past = request.form.get('success_past') == "true"
+ upstream = to_boolean(args.get('failed_upstream'))
Review comment:
This looks like a bug, why is the options `failed_upstream`? It should be `success_upstream`. Same for the other args. @msumit could you fix?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649408501
##########
File path: airflow/www/views.py
##########
@@ -1803,54 +1804,104 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
- flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
+ flash(f"Cannot mark tasks as {state}, seem that dag {dag_id} has never run", "error")
return redirect(origin)
execution_date = timezone.parse(execution_date)
from airflow.api.common.experimental.mark_tasks import set_state
- if confirmed:
- with create_session() as session:
- altered = set_state(
- tasks=[task],
- execution_date=execution_date,
- upstream=upstream,
- downstream=downstream,
- future=future,
- past=past,
- state=state,
- commit=True,
- session=session,
- )
+ with create_session() as session:
+ altered = set_state(
+ tasks=[task],
+ execution_date=execution_date,
+ upstream=upstream,
+ downstream=downstream,
+ future=future,
+ past=past,
+ state=state,
+ commit=True,
+ session=session,
+ )
- # Clear downstream tasks that are in failed/upstream_failed state to resume them.
- # Flush the session so that the tasks marked success are reflected in the db.
- session.flush()
- subdag = dag.partial_subset(
- task_ids_or_regex={task_id},
- include_downstream=True,
- include_upstream=False,
- )
+ # Clear downstream tasks that are in failed/upstream_failed state to resume them.
+ # Flush the session so that the tasks marked success are reflected in the db.
+ session.flush()
+ subdag = dag.partial_subset(
+ task_ids_or_regex={task_id},
+ include_downstream=True,
+ include_upstream=False,
+ )
- end_date = execution_date if not future else None
- start_date = execution_date if not past else None
-
- subdag.clear(
- start_date=start_date,
- end_date=end_date,
- include_subdags=True,
- include_parentdag=True,
- only_failed=True,
- session=session,
- # Exclude the task itself from being cleared
- exclude_task_ids={task_id},
- )
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
- session.commit()
+ subdag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=True,
+ include_parentdag=True,
+ only_failed=True,
+ session=session,
+ # Exclude the task itself from being cleared
+ exclude_task_ids={task_id},
+ )
- flash(f"Marked {state} on {len(altered)} task instances")
- return redirect(origin)
+ session.commit()
+
+ flash(f"Marked {state} on {len(altered)} task instances")
+ return redirect(origin)
+
+ @expose('/confirm', methods=['GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE),
+ ]
+ )
+ @action_logging
+ def confirm(self):
+ """Show confirmation page for marking tasks as success or failed."""
+ args = request.args
+ dag_id = args.get('dag_id')
+ task_id = args.get('task_id')
+ execution_date = args.get('execution_date')
+ state = args.get('state')
+
+ upstream = to_boolean(args.get('failed_upstream'))
Review comment:
IMO it's safe to assume that `None` is `False`. Just saves a lot of code repetition & also safeguard against handling an edge case for the future.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-856080760
@ashb @uranusjr @kaxil @potiuk can you guys plz review this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649781077
##########
File path: airflow/www/views.py
##########
@@ -1807,54 +1808,104 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
- flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
+ flash(f"Cannot mark tasks as {state}, seem that dag {dag_id} has never run", "error")
return redirect(origin)
execution_date = timezone.parse(execution_date)
from airflow.api.common.experimental.mark_tasks import set_state
- if confirmed:
- with create_session() as session:
- altered = set_state(
- tasks=[task],
- execution_date=execution_date,
- upstream=upstream,
- downstream=downstream,
- future=future,
- past=past,
- state=state,
- commit=True,
- session=session,
- )
+ with create_session() as session:
+ altered = set_state(
+ tasks=[task],
+ execution_date=execution_date,
+ upstream=upstream,
+ downstream=downstream,
+ future=future,
+ past=past,
+ state=state,
+ commit=True,
+ session=session,
+ )
- # Clear downstream tasks that are in failed/upstream_failed state to resume them.
- # Flush the session so that the tasks marked success are reflected in the db.
- session.flush()
- subdag = dag.partial_subset(
- task_ids_or_regex={task_id},
- include_downstream=True,
- include_upstream=False,
- )
+ # Clear downstream tasks that are in failed/upstream_failed state to resume them.
+ # Flush the session so that the tasks marked success are reflected in the db.
+ session.flush()
+ subdag = dag.partial_subset(
+ task_ids_or_regex={task_id},
+ include_downstream=True,
+ include_upstream=False,
+ )
- end_date = execution_date if not future else None
- start_date = execution_date if not past else None
-
- subdag.clear(
- start_date=start_date,
- end_date=end_date,
- include_subdags=True,
- include_parentdag=True,
- only_failed=True,
- session=session,
- # Exclude the task itself from being cleared
- exclude_task_ids={task_id},
- )
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
- session.commit()
+ subdag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=True,
+ include_parentdag=True,
+ only_failed=True,
+ session=session,
+ # Exclude the task itself from being cleared
+ exclude_task_ids={task_id},
+ )
- flash(f"Marked {state} on {len(altered)} task instances")
- return redirect(origin)
+ session.commit()
+
+ flash(f"Marked {state} on {len(altered)} task instances")
+ return redirect(origin)
+
+ @expose('/confirm', methods=['GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE),
+ ]
+ )
+ @action_logging
+ def confirm(self):
+ """Show confirmation page for marking tasks as success or failed."""
+ args = request.args
+ dag_id = args.get('dag_id')
+ task_id = args.get('task_id')
+ execution_date = args.get('execution_date')
+ state = args.get('state')
+
+ upstream = to_boolean(args.get('failed_upstream'))
+ downstream = to_boolean(args.get('failed_downstream'))
+ future = to_boolean(args.get('failed_future'))
+ past = to_boolean(args.get('failed_past'))
+
+ try:
+ dag = current_app.dag_bag.get_dag(dag_id)
+ except airflow.exceptions.SerializedDagNotFound:
+ flash(f'DAG {dag_id} not found', "error")
+ return redirect(request.referrer or url_for('Airflow.index'))
+
+ try:
+ task = dag.get_task(task_id)
+ except airflow.exceptions.TaskNotFound:
+ flash(f"Task {task_id} not found", "error")
+ return redirect(request.referrer or url_for('Airflow.index'))
+
+ task.dag = dag
+
+ if state not in (
+ 'success',
+ 'failed',
+ ):
+ flash(f"Invalid state {state}, must be either 'success' or 'failed'", "error")
+ return redirect(request.referrer or url_for('Airflow.index'))
+
+ latest_execution_date = dag.get_latest_execution_date()
+ if not latest_execution_date:
+ flash(f"Cannot mark tasks as {state}, seem that dag {dag_id} has never run", "error")
+ return redirect(request.referrer or url_for('Airflow.index'))
+
+ execution_date = timezone.parse(execution_date)
+
+ from airflow.api.common.experimental.mark_tasks import set_state
Review comment:
I see some usages of experimental API in this file (unrelated to this PR). We should replace that with stable API
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648566829
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -372,7 +372,6 @@ <h4 class="modal-title" id="dagModalLabel">
<div class="row">
<span class="btn-group col-md-8">
<form method="POST">
Review comment:
This is still POST so we should keep the csrf_token, no?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r649406401
##########
File path: airflow/www/views.py
##########
@@ -1803,54 +1804,104 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
- flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
+ flash(f"Cannot mark tasks as {state}, seem that dag {dag_id} has never run", "error")
return redirect(origin)
execution_date = timezone.parse(execution_date)
from airflow.api.common.experimental.mark_tasks import set_state
- if confirmed:
- with create_session() as session:
- altered = set_state(
- tasks=[task],
- execution_date=execution_date,
- upstream=upstream,
- downstream=downstream,
- future=future,
- past=past,
- state=state,
- commit=True,
- session=session,
- )
+ with create_session() as session:
+ altered = set_state(
+ tasks=[task],
+ execution_date=execution_date,
+ upstream=upstream,
+ downstream=downstream,
+ future=future,
+ past=past,
+ state=state,
+ commit=True,
+ session=session,
+ )
- # Clear downstream tasks that are in failed/upstream_failed state to resume them.
- # Flush the session so that the tasks marked success are reflected in the db.
- session.flush()
- subdag = dag.partial_subset(
- task_ids_or_regex={task_id},
- include_downstream=True,
- include_upstream=False,
- )
+ # Clear downstream tasks that are in failed/upstream_failed state to resume them.
+ # Flush the session so that the tasks marked success are reflected in the db.
+ session.flush()
+ subdag = dag.partial_subset(
+ task_ids_or_regex={task_id},
+ include_downstream=True,
+ include_upstream=False,
+ )
- end_date = execution_date if not future else None
- start_date = execution_date if not past else None
-
- subdag.clear(
- start_date=start_date,
- end_date=end_date,
- include_subdags=True,
- include_parentdag=True,
- only_failed=True,
- session=session,
- # Exclude the task itself from being cleared
- exclude_task_ids={task_id},
- )
+ end_date = execution_date if not future else None
+ start_date = execution_date if not past else None
- session.commit()
+ subdag.clear(
+ start_date=start_date,
+ end_date=end_date,
+ include_subdags=True,
+ include_parentdag=True,
+ only_failed=True,
+ session=session,
+ # Exclude the task itself from being cleared
+ exclude_task_ids={task_id},
+ )
- flash(f"Marked {state} on {len(altered)} task instances")
- return redirect(origin)
+ session.commit()
+
+ flash(f"Marked {state} on {len(altered)} task instances")
+ return redirect(origin)
+
+ @expose('/confirm', methods=['GET'])
+ @auth.has_access(
+ [
+ (permissions.ACTION_CAN_READ, permissions.RESOURCE_DAG),
+ (permissions.ACTION_CAN_EDIT, permissions.RESOURCE_TASK_INSTANCE),
+ ]
+ )
+ @action_logging
+ def confirm(self):
+ """Show confirmation page for marking tasks as success or failed."""
+ args = request.args
+ dag_id = args.get('dag_id')
+ task_id = args.get('task_id')
+ execution_date = args.get('execution_date')
+ state = args.get('state')
+
+ upstream = to_boolean(args.get('failed_upstream'))
Review comment:
I would avoid changing `to_boolean` and do `args.get('failed_upstream', '')` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit merged pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit merged pull request #16233:
URL: https://github.com/apache/airflow/pull/16233
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#issuecomment-856116981
rebased your PR on latest Main, let's see if it solves some Build issues -- I know there was some bug few days back
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: WIP: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r644753339
##########
File path: airflow/www/views.py
##########
@@ -1804,53 +1802,86 @@ def _mark_task_instance_state( # pylint: disable=too-many-arguments
latest_execution_date = dag.get_latest_execution_date()
if not latest_execution_date:
flash(f"Cannot make {state}, seem that dag {dag_id} has never run", "error")
- return redirect(origin)
+ return redirect(request.referrer)
Review comment:
Ok, brought the `origin` back. In case someone is clicking it via an email link, then after the operation, it loads the home page, which is fine in my opinion.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] msumit commented on a change in pull request #16233: Fix TI success/failure links
Posted by GitBox <gi...@apache.org>.
msumit commented on a change in pull request #16233:
URL: https://github.com/apache/airflow/pull/16233#discussion_r648847407
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -372,7 +372,6 @@ <h4 class="modal-title" id="dagModalLabel">
<div class="row">
<span class="btn-group col-md-8">
<form method="POST">
Review comment:
Oops, that was a wrong edit.. reverting it.. thanks @kaxil & @bbovenzi
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org