subhramit commented on code in PR #73592:
URL: https://github.com/apache/airflow/pull/73592#discussion_r4080058940
##########
providers/google/src/airflow/providers/google/cloud/operators/cloud_build.py:
##########
@@ -205,12 +203,14 @@ def __init__(
def prepare_template(self) -> None:
# if no file is specified, skip
- if not isinstance(self.build_raw, str):
+ if not isinstance(self.build, str):
return
- with open(self.build_raw) as file:
- if self.build_raw.endswith((".yaml", ".yml")):
+ # Keep the path in a local: the .yaml branch below replaces self.build
with a dict.
Review Comment:
Since the json branch also replaces it
```suggestion
# Keep the path in a local: the branches below replace self.build
with a dict.
```
##########
providers/google/tests/unit/google/cloud/operators/test_cloud_build.py:
##########
@@ -165,6 +165,26 @@ def test_load_templated(self, file_type, file_content):
expected_body = {"steps": [{"name": "ubuntu", "args": ["echo",
"Hello {{ params.name }}!"]}]}
assert expected_body == operator.build
+ def test_init_does_not_duplicate_build(self):
+ build_path = "path/to/build.json"
+ operator = CloudBuildCreateBuildOperator(build=build_path,
task_id="task-id")
+ assert operator.build == build_path
+ assert not hasattr(operator, "build_raw")
Review Comment:
A bit skeptical about this one being too specific - the problem was that
`__init__` copied build into a second attribute, so can't someone make the same
mistake with a different name, lets say
```python
self._original_build = build
```
then the hasstr wont catch this.
Can we maybe do some sort of behavioral testing instead?
--
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]
