-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add muldelete action to TaskInstanceModelView and tests. Change permissions for TaskInstanceModelView.action_clear to "delete" #18438
Conversation
tests/www/views/test_views_tasks.py
Outdated
assert resp.status_code == 200 | ||
for task_search_tuple in task_search_tuples: | ||
dag_id, task_id = task_search_tuple | ||
session.query(TaskInstance).filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add an assert here... Adjusting now.
acb0bf3
to
5e27c6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
There is a security issue here. |
I created a PR to add a test to avoid the security issue in the future: #18467. |
No offense taken, I'm happy that you commented because now I can learn something from this. I was wondering how I missed something like that since I just referred to the DagRun equivalent of the muldelete function but it looks like that decorator was added 2 days ago and I just wasn't up to date on my main branch. Good catch. I'm going to add that now. |
Nice, PR LGTM :) |
Any update on this one, merge team? |
fa75bac
to
80841e7
Compare
@jedcunningham Changes addressed! |
Nice work 👍 |
Closes #18333. Adds a Delete option in
TaskInstanceModelView
and deletes multipleTaskInstance
s based on which options are checked in the model view. Added unit tests for this as well. Tests that the right tasks get deleted and also tests that deletes get cascaded toTaskReschedule
s in the meta DB.Also, changed permissions for
DagRunModelView
'saction_clear
to beedit
instead ofdelete
.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.