Copilot commented on code in PR #64538:
URL: https://github.com/apache/airflow/pull/64538#discussion_r3066481396
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks_workflow.py:
##########
@@ -333,6 +357,7 @@ def __init__(
**kwargs,
):
self.databricks_conn_id = databricks_conn_id
+ self.access_control_list = access_control_list or []
Review Comment:
Same issue as in `_CreateDatabricksWorkflowOperator`: normalizing
`access_control_list` with `or []` removes the ability to represent “not
provided” vs “explicit empty list”. If callers should be able to intentionally
pass an empty ACL, keep `self.access_control_list` as `None` when unset and
pass that through to the launch operator.
##########
providers/databricks/tests/unit/databricks/operators/test_databricks_workflow.py:
##########
@@ -214,6 +242,7 @@ def
test_task_group_exit_creates_operator(mock_databricks_workflow_operator):
task_group=task_group,
task_id="launch",
databricks_conn_id="databricks_conn",
+ access_control_list=[],
existing_clusters=[],
Review Comment:
The test currently asserts the TaskGroup passes `access_control_list=[]` to
`_CreateDatabricksWorkflowOperator` even when the user did not set any ACL. If
the implementation is updated to preserve `None` vs an explicit empty list,
this expectation should change (and it would be good to add a regression test
that an explicitly provided empty list is preserved and results in an
`access_control_list` key in the generated job spec).
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks_workflow.py:
##########
@@ -188,6 +200,10 @@ def create_workflow_json(self, context: Context | None =
None) -> dict[str, obje
"job_clusters": self.job_clusters,
"max_concurrent_runs": self.max_concurrent_runs,
}
+
+ if self.access_control_list:
Review Comment:
`if self.access_control_list:` will omit `access_control_list` when it is an
empty list, even if the user explicitly provided `[]`. If the intent is to
support overriding/clearing permissions (and to match other Databricks
operators), check `self.access_control_list is not None` instead of truthiness.
--
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]
