kaxil commented on code in PR #64538:
URL: https://github.com/apache/airflow/pull/64538#discussion_r3113735505
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks_workflow.py:
##########
@@ -81,6 +81,15 @@ class _CreateDatabricksWorkflowOperator(BaseOperator):
:param task_id: The task_id of the operator
:param databricks_conn_id: The connection ID to use when connecting to
Databricks.
+ :param access_control_list: List of permissions to set on the job. Array
of object
+ (AccessControlRequestForUser) or object (AccessControlRequestForGroup)
or object
+ (AccessControlRequestForServicePrincipal).
+
+ .. seealso::
+ The access control list is applied both when creating a new job
and when resetting
+ an existing job. When provided for an existing job, the supplied
ACL replaces the
+ job's current permissions. You can also manage job permissions
directly in the
Review Comment:
This claim doesn't quite hold for `[]`. `DatabricksHook.reset_job` uses `if
access_control_list:` (truthy check) before calling `update_job_permission`, so
passing `access_control_list=[]` on an existing job is a no-op -- the prior ACL
stays in place rather than getting replaced. Worth tightening the wording (e.g.
"a non-empty ACL replaces the current permissions") or handling the
clear-permissions case explicitly.
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks_workflow.py:
##########
@@ -299,6 +315,13 @@ class DatabricksWorkflowTaskGroup(TaskGroup):
:param databricks_conn_id: The name of the databricks connection to use.
:param existing_clusters: A list of existing clusters to use for this
workflow.
+ :param access_control_list: List of permissions to set on the job. Array
of object
+ (AccessControlRequestForUser) or object (AccessControlRequestForGroup)
or object
+ (AccessControlRequestForServicePrincipal).
+
+ .. seealso::
+ When provided, this is applied when the job is created and when an
existing job is
+ reset. Setting this replaces any existing job permissions.
Review Comment:
Same as the `_CreateDatabricksWorkflowOperator` docstring: "Setting this
replaces any existing job permissions" is only true for a non-empty list
because `DatabricksHook.reset_job` short-circuits on the truthy check. Also
worth adding `access_control_list` to the parameter list in
`providers/databricks/docs/operators/workflow.rst` so users discover it from
the guide, not only via autodoc.
##########
providers/databricks/tests/unit/databricks/operators/test_databricks_workflow.py:
##########
@@ -98,6 +105,30 @@ def test_create_workflow_json(mock_databricks_hook,
context, mock_task_group):
assert workflow_json["max_concurrent_runs"] == 1
assert workflow_json["timeout_seconds"] == 0
+ assert "access_control_list" not in workflow_json
+
+
[email protected]("access_control_list", [ACCESS_CONTROL_LIST, []])
Review Comment:
The `[]` parametrize case only asserts the JSON payload contains the key.
Given the docstring claims an empty list replaces permissions on reset,
consider a test that goes through `_create_or_reset_job` and verifies what
reaches `reset_job` / `update_job_permission` when `access_control_list=[]`,
otherwise the docstring and the hook's truthy check will silently drift apart.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
