Build DataprocCreateClusterOperator cluster_config from legacy kwargs after rendering - #70922
bujjibabukatta wants to merge 10 commits into
Conversation
82398f0 to
e12ee19
Compare
|
Hi @SameerMesiah97 thanks for catching both — fixed. Wrapped the operator Separately: Static checks keeps failing on validate-operators-init, flagging |
9d1b55f to
6dbb39b
Compare
df95d98 to
4561488
Compare
4561488 to
99b08c4
Compare
|
Hi @olegkachur-e, @SameerMesiah97 can you please review and approve pull request ? |
| 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() |
There was a problem hiding this comment.
Why changing that?
kwargs is not in template_fields
| @@ -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 | |||
There was a problem hiding this comment.
project_id is a template field, so this statement should be dropped and we should rely only on self.project_id
| 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!") |
There was a problem hiding this comment.
The message should say what is wrong ("here" is not clear enough)
|
|
||
| @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): |
There was a problem hiding this 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)
…_id injection, remove exemption
f143916 to
d63b680
Compare
| @@ -685,6 +685,7 @@ class DataprocCreateClusterOperator(GoogleCloudBaseOperator): | |||
| "labels", | |||
| "gcp_conn_id", | |||
| "impersonation_chain", | |||
| "_legacy_cluster_kwargs", | |||
There was a problem hiding this comment.
Looks like previously legacy kwargs built cluster_config during init. Now cluster_config remains None until execute().
Normal task execution should remain compatible, but code that reads operator.cluster_config before execution—cluster policies, subclasses, custom validation, or tests—will observe different behavior. Invalid legacy configuration also moves from Dag parsing time to task execution time.
There was a problem hiding this comment.
Also DataprocCreateClusterOperator remains in validate_operators_init_exemptions.txt. but this PR does not finish this operator’s part of issue #70296.
The new _legacy_cluster_kwargs field is synthesized in init, so it does not follow the validator’s normal direct-assignment rule. The implementation and exemption strategy need to be resolved together.
| cluster_config = ClusterGenerator(**kwargs).make() | ||
|
|
||
| # Defer building cluster_config until execute(), after templated fields render. | ||
| self._legacy_cluster_kwargs: dict | None = dict(kwargs) |
There was a problem hiding this comment.
this captures more than Dataproc legacy parameters, including params, dag, task_group, callbacks, executor configuration, and other BaseOperator values.
Only ClusterGenerator arguments should be captured, no?
Summary
DataprocCreateClusterOperator's deprecated keyword-based cluster
construction built cluster_config in init, using individually
templated kwargs (zone, cluster_name, project_id, ...) before rendering.
Root cause
ClusterGenerator(**kwargs).make() ran in init. These kwargs aren't
declared as template fields, so Jinja expressions passed this way never
got rendered before being baked into cluster_config.
Fix
Stash the raw kwargs in a new template field (_legacy_cluster_kwargs)
instead of building cluster_config immediately. Being a template field, it
survives DAG serialization and gets rendered automatically. cluster_config
is now built in execute(), after rendering.
related: #70296
Was generative AI tooling used ?
Generated-by: Claude following the guidelines