Skip to content
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

Ensures DatabricksWorkflowOperator updates ACL (if available) when resetting a job. #47827

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hardeybisey
Copy link
Contributor

...

closes: #45738


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@hardeybisey hardeybisey marked this pull request as draft March 16, 2025 15:17
@rawwar rawwar self-requested a review March 16, 2025 15:26
@hardeybisey hardeybisey force-pushed the update-acl-on-databricks-workflowoperator-reset branch from c13647c to ce4d7c4 Compare March 19, 2025 11:54
@hardeybisey hardeybisey marked this pull request as ready for review March 19, 2025 11:55
@adamgorkaextbi
Copy link

:-)

@rawwar
Copy link
Contributor

rawwar commented Apr 3, 2025

@hardeybisey , can we add a test?

@rawwar rawwar requested a review from pankajkoti April 3, 2025 10:24
@hardeybisey
Copy link
Contributor Author

@hardeybisey , can we add a test?

Sure, I will add the test and tag you once it's done.

@hardeybisey hardeybisey force-pushed the update-acl-on-databricks-workflowoperator-reset branch 3 times, most recently from fd92a88 to 9b387d1 Compare April 5, 2025 09:44
@hardeybisey hardeybisey force-pushed the update-acl-on-databricks-workflowoperator-reset branch from 9b387d1 to 1172e6c Compare April 5, 2025 09:45
@hardeybisey
Copy link
Contributor Author

@rawwar I have added test to the PR.

self.job_name,
access_control_list,
)
self._hook.update_job_permission(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is always expected to happen while resetting the job when the ACL is available, I suggest we move this call in the hook's reset_job method. Like that any further usage of the reset_job method will ensure that updating ACL automatically happens in case someone forgets to make this isolated call.

As I am checking, I see we have a similar occurrence in the Create Jobs operator at https://github.com/apache/airflow/blob/main/providers/databricks/src/airflow/providers/databricks/operators/databricks.py#L402. We could refactor that once we move this call to the reset_job method.

@hardeybisey hardeybisey force-pushed the update-acl-on-databricks-workflowoperator-reset branch from 116bcfe to 5695ffe Compare April 8, 2025 18:31
@hardeybisey hardeybisey marked this pull request as draft April 8, 2025 19:43
@hardeybisey hardeybisey force-pushed the update-acl-on-databricks-workflowoperator-reset branch 3 times, most recently from 24f9b1c to 48b115e Compare April 10, 2025 16:11
@hardeybisey hardeybisey force-pushed the update-acl-on-databricks-workflowoperator-reset branch from 48b115e to 7512fe3 Compare April 10, 2025 16:25
@hardeybisey hardeybisey marked this pull request as ready for review April 10, 2025 16:26
@hardeybisey hardeybisey requested a review from pankajkoti April 10, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DatabricksWorkflowOperator do not update ACL on workflow reset
4 participants