Copilot commented on code in PR #64538:
URL: https://github.com/apache/airflow/pull/64538#discussion_r3038203089
##########
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::
+ This will only be used on create. In order to reset ACL consider
using the Databricks
+ UI.
Review Comment:
The `DatabricksWorkflowTaskGroup` docstring says `access_control_list` “will
only be used on create”, but the task group passes this through to
`_CreateDatabricksWorkflowOperator`, which applies the ACL on both job creation
and `reset_job()` for existing jobs. Please update this docstring to match the
actual behavior (and clarify that it replaces existing permissions when set).
```suggestion
When provided, this is applied when the job is created and when
an existing job is
reset. Setting this replaces any existing job permissions.
```
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks_workflow.py:
##########
@@ -137,6 +148,7 @@ def __init__(
**kwargs,
):
self.databricks_conn_id = databricks_conn_id
+ self.access_control_list = access_control_list or []
self.existing_clusters = existing_clusters or []
Review Comment:
`access_control_list` is normalized with `access_control_list or []`, which
makes it impossible to distinguish an explicitly provided empty list from
`None`. Combined with `if self.access_control_list:` in `create_workflow_json`,
passing `access_control_list=[]` will silently omit the field from the job
spec. If callers should be able to explicitly send an empty ACL (e.g. to
clear/override permissions) or to match other Databricks operators that include
the key when the argument is not `None`, keep `self.access_control_list` as
`None` by default and include the key when `access_control_list is not None`
(even if empty).
--
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]
