You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hussein-awala (via GitHub)" <gi...@apache.org> on 2023/10/14 16:01:43 UTC
[PR] Some improvements/fixes for dag_run and task_instance endpoints [airflow]
hussein-awala opened a new pull request, #34942:
URL: https://github.com/apache/airflow/pull/34942
This PR updates the required permissions for some endpoints:
- `update_dag_run_state`, `clear_dag_run` and `set_dag_run_note` -> it replaces `can read dag` by `can edit dag` similar to what we have in the other endpoints (for example the one used to clear TI state)
- `post_clear_task_instances` -> it replaces `can read dagrun` by `can edit dagrun`, because the endpoint accepts a parameter `reset_dag_runs` to reset the dagruns states, which should check for edit on dagrun resource.
- `set_task_instance_note` -> it removes `can read dag` because it is useless, the edit permission should be sufficient
I'm not sure if we consider some of these changes as breaking changes, I can create different PRs if needed.
--
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
Re: [PR] Some improvements/fixes for dag_run and task_instance endpoints [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #34942:
URL: https://github.com/apache/airflow/pull/34942
--
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
Re: [PR] Some improvements/fixes for dag_run and task_instance endpoints [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34942:
URL: https://github.com/apache/airflow/pull/34942#issuecomment-1763094792
(also this change can be cherry-pickable ot 2.7.3 so that's why I think it should be merged first and cherry-picked) - provisionally marked it for 2.7.3.
--
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
Re: [PR] Some improvements/fixes for dag_run and task_instance endpoints [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34942:
URL: https://github.com/apache/airflow/pull/34942#issuecomment-1763091741
Oh yeah. Two days ago I noticed the same problems while reviewing https://github.com/apache/airflow/pull/34317
See my comments here: https://github.com/apache/airflow/pull/34317#discussion_r1355894015
This code will go away soon (cc: @vincbeck ). But maybe it's worth to reconcile the two changes and see if all the read->edit changes are same in both (@hussein-awala and @vincbeck ?). It looks they are all the same.
And maybe it makes sense to merge this change first and #34317 afterwards to clearly separate fix from the Auth manager refactor?
And no - I think it should not be treated as a breaking change, the previous settings made really no sense (having access to task_instance while not having access to dag_run was wrong because by changing task_instance state you can impact state of the dag_run), however we should have a significant note 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Some improvements/fixes for dag_run and task_instance endpoints [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #34942:
URL: https://github.com/apache/airflow/pull/34942#issuecomment-1763105010
> And maybe it makes sense to merge this change first and https://github.com/apache/airflow/pull/34317 afterwards to clearly separate fix from the Auth manager refactor?
I just checked the other PR. I agree about merging this first to make the transition to the new permissions checker more clear for the users.
--
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
Re: [PR] Some improvements/fixes for dag_run and task_instance endpoints [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #34942:
URL: https://github.com/apache/airflow/pull/34942#issuecomment-1764625106
> Oh yeah. Two days ago I noticed the same problems while reviewing #34317
>
> See my comments here: [#34317 (comment)](https://github.com/apache/airflow/pull/34317#discussion_r1355894015) and [#34317 (comment)](https://github.com/apache/airflow/pull/34317#discussion_r1355907009) (and few others).
>
> This code will go away soon (cc: @vincbeck ). But maybe it's worth to reconcile the two changes and see if all the read->edit changes are same in both (@hussein-awala and @vincbeck ?). It looks they are all the same.
>
> And maybe it makes sense to merge this change first and #34317 afterwards to clearly separate fix from the Auth manager refactor?
>
> And no - I think it should not be treated as a breaking change, the previous settings made really no sense (having access to task_instance while not having access to dag_run was wrong because by changing task_instance state you can impact state of the dag_run), however we should have a significant note about it.
Yep I agree, let's merge this one first and I'll make the change in my PR as well
--
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