You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/14 21:04:19 UTC
[GitHub] [airflow] alekseiloginov opened a new pull request, #23025: fix empty materialization id handling
alekseiloginov opened a new pull request, #23025:
URL: https://github.com/apache/airflow/pull/23025
## Changes
- Add `materialization id` check for null in sensor
- Check `materialization id` for an empty string (not just null)
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23025: fix empty materialization id handling
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23025:
URL: https://github.com/apache/airflow/pull/23025#issuecomment-1108260589
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23025: fix empty materialization id handling
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23025:
URL: https://github.com/apache/airflow/pull/23025#discussion_r851129116
##########
tests/providers/google/cloud/operators/test_looker.py:
##########
@@ -146,3 +149,21 @@ def test_on_kill(self, mock_hook):
task.cancel_on_kill = True
task.on_kill()
mock_hook.return_value.stop_pdt_build.assert_called_once_with(materialization_id=TEST_JOB_ID)
+
+ @mock.patch(OPERATOR_PATH.format("LookerHook"))
+ def test_materialization_id_returned_as_empty_str(self, mock_hook):
+ # mock return vals from hook
+ mock_hook.return_value.start_pdt_build.return_value.materialization_id = ""
+ mock_hook.return_value.wait_for_job.return_value = None
+
+ # run task in mock context (asynchronous=False)
+ task = LookerStartPdtBuildOperator(
+ task_id=TASK_ID,
+ looker_conn_id=LOOKER_CONN_ID,
+ model=MODEL,
+ view=VIEW,
+ )
+
+ # check AirflowException is raised
+ with pytest.raises(AirflowException, match="No `materialization_id` was returned"):
Review Comment:
Can we check the complete message?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23025: fix empty materialization id handling
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23025:
URL: https://github.com/apache/airflow/pull/23025#discussion_r851129374
##########
tests/providers/google/cloud/sensors/test_looker.py:
##########
@@ -105,3 +105,15 @@ def test_cancelled(self, mock_hook):
# assert hook.pdt_build_status called once
mock_hook.return_value.pdt_build_status.assert_called_once_with(materialization_id=TEST_JOB_ID)
+
+ def test_empty_materialization_id(self):
+
+ # run task in mock context
+ sensor = LookerCheckPdtBuildSensor(
+ task_id=TASK_ID,
+ looker_conn_id=LOOKER_CONN_ID,
+ materialization_id="",
+ )
+
+ with pytest.raises(AirflowException, match="Invalid `materialization_id`"):
Review Comment:
```suggestion
with pytest.raises(AirflowException, match="^Invalid `materialization_id`$"):
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #23025: `LookerStartPdtBuildOperator`, `LookerCheckPdtBuildSensor` : fix empty materialization id handling
Posted by GitBox <gi...@apache.org>.
eladkal merged PR #23025:
URL: https://github.com/apache/airflow/pull/23025
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23025: fix empty materialization id handling
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23025:
URL: https://github.com/apache/airflow/pull/23025#discussion_r851129116
##########
tests/providers/google/cloud/operators/test_looker.py:
##########
@@ -146,3 +149,21 @@ def test_on_kill(self, mock_hook):
task.cancel_on_kill = True
task.on_kill()
mock_hook.return_value.stop_pdt_build.assert_called_once_with(materialization_id=TEST_JOB_ID)
+
+ @mock.patch(OPERATOR_PATH.format("LookerHook"))
+ def test_materialization_id_returned_as_empty_str(self, mock_hook):
+ # mock return vals from hook
+ mock_hook.return_value.start_pdt_build.return_value.materialization_id = ""
+ mock_hook.return_value.wait_for_job.return_value = None
+
+ # run task in mock context (asynchronous=False)
+ task = LookerStartPdtBuildOperator(
+ task_id=TASK_ID,
+ looker_conn_id=LOOKER_CONN_ID,
+ model=MODEL,
+ view=VIEW,
+ )
+
+ # check AirflowException is raised
+ with pytest.raises(AirflowException, match="No `materialization_id` was returned"):
Review Comment:
```suggestion
with pytest.raises(AirflowException, match="^No `materialization_id` was returned$"):
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #23025: fix empty materialization id handling
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #23025:
URL: https://github.com/apache/airflow/pull/23025#discussion_r851128973
##########
airflow/providers/google/cloud/sensors/looker.py:
##########
@@ -53,6 +53,11 @@ def poke(self, context: "Context") -> bool:
self.hook = LookerHook(looker_conn_id=self.looker_conn_id)
+ if not self.materialization_id:
+ raise AirflowException(
+ f'Invalid `materialization_id`.'
+ )
Review Comment:
```suggestion
raise AirflowException('Invalid `materialization_id`.')
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org