shahar1 commented on code in PR #70922:
URL: https://github.com/apache/airflow/pull/70922#discussion_r3997353916
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -726,19 +725,22 @@ def __init__(
if "params" in kwargs:
del kwargs["params"]
- # Create cluster object from kwargs
if project_id is None:
raise AirflowException(
"project_id argument is required when building cluster
from keywords parameters"
)
kwargs["project_id"] = project_id
- cluster_config = ClusterGenerator(**kwargs).make()
Review Comment:
Why changing that?
`kwargs` is not in template_fields
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -761,6 +763,17 @@ def __init__(
self.polling_interval_seconds = polling_interval_seconds
self.num_retries_if_resource_is_not_ready =
num_retries_if_resource_is_not_ready
+ def _build_cluster_config_from_legacy_kwargs(self) -> dict:
+ """Build cluster_config from legacy keyword args, called post-render
in execute()."""
+ if self._legacy_cluster_kwargs is None:
+ raise RuntimeError("The _legacy_cluster_kwargs should be set
here!")
Review Comment:
The message should say what is wrong ("here" is not clear enough)
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -726,19 +725,22 @@ def __init__(
if "params" in kwargs:
del kwargs["params"]
- # Create cluster object from kwargs
if project_id is None:
raise AirflowException(
"project_id argument is required when building cluster
from keywords parameters"
)
kwargs["project_id"] = project_id
Review Comment:
`project_id` is a template field, so this statement should be dropped and we
should rely only on `self.project_id`
##########
providers/google/tests/unit/google/cloud/operators/test_dataproc.py:
##########
@@ -998,6 +998,25 @@ def test_deprecation_warning(self):
assert op.project_id == GCP_PROJECT
assert op.cluster_name == "cluster_name"
+ assert op.cluster_config is None
+ assert op._legacy_cluster_kwargs["num_workers"] == 2
+ assert op._legacy_cluster_kwargs["zone"] == "zone"
+
+ @mock.patch(DATAPROC_PATH.format("Cluster.to_dict"))
+ @mock.patch(DATAPROC_PATH.format("DataprocHook"))
+ def test_deprecated_kwargs_cluster_config_built_in_execute(self,
mock_hook, to_dict_mock):
Review Comment:
I would add some tests for the template fields (examples are available in
GCP's `test_compute.py` and `test_cloud_storage_transfer_service.py`)
--
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]
