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 2020/04/04 22:08:24 UTC
[GitHub] [airflow] jmcarp opened a new pull request #8145: Drop redundant
project id null checks.
jmcarp opened a new pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145
The `fallback_to_default_project_id` method already raises an exception
if both `self.project_id` and `kwargs["project_id"]` are missing, so
skip redundant null checks in hook methods.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on a change in pull request #8145:
Drop redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r405294959
##########
File path: tests/providers/google/cloud/hooks/test_cloud_sql.py
##########
@@ -561,29 +513,6 @@ def test_get_instance_overridden_project_id(
execute_method.assert_called_once_with(num_retries=5)
wait_for_operation_to_complete.assert_not_called()
- @mock.patch(
- 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook.project_id',
- new_callable=PropertyMock,
- return_value=None
- )
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook.get_conn')
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook._wait_for_operation_to_complete')
- def test_get_instance_missing_project_id(
- self, wait_for_operation_to_complete, get_conn, mock_project_id
- ):
- get_method = get_conn.return_value.instances.return_value.get
- execute_method = get_method.return_value.execute
- execute_method.return_value = {"name": "instance"}
- wait_for_operation_to_complete.return_value = None
- with self.assertRaises(AirflowException) as cm:
- self.cloudsql_hook_no_default_project_id.get_instance(
- instance='instance')
- get_method.assert_not_called()
- execute_method.assert_not_called()
- err = cm.exception
- self.assertIn("The project id must be passed", str(err))
- wait_for_operation_to_complete.assert_not_called()
-
Review comment:
Why remove those tests?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609499427
> Quick thought: The same issue about project_id also applies to session/provide_session
As well as any other decorator that will inject arguments. @jmcarp will we be able to somehow check if all decorators that require it use this plugin? `project_id` is only GCP stuff an probably not so much problematic but `provide_session` is common and its case can be worth considering.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406385421
##########
File path: airflow/providers/google/cloud/operators/dataflow.py
##########
@@ -172,6 +172,7 @@ def __init__(
job_name: str = '{{task.task_id}}',
dataflow_default_options: Optional[dict] = None,
options: Optional[dict] = None,
+ project_id: Optional[str] = None,
Review comment:
Nice! But needs UPDATING.md entry
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612370458
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=h1) Report
> Merging [#8145](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/77412ac6ea3486673d5e2340d10b355e12317fc0&el=desc) will **decrease** coverage by `0.40%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8145 +/- ##
==========================================
- Coverage 88.33% 87.93% -0.41%
==========================================
Files 936 937 +1
Lines 45321 45197 -124
==========================================
- Hits 40036 39743 -293
- Misses 5285 5454 +169
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/mypy/plugin/decorators.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9teXB5L3BsdWdpbi9kZWNvcmF0b3JzLnB5) | `0.00% <0.00%> (ø)` | |
| [...google/cloud/example\_dags/example\_automl\_tables.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2V4YW1wbGVfZGFncy9leGFtcGxlX2F1dG9tbF90YWJsZXMucHk=) | `92.72% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/automl.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2F1dG9tbC5weQ==) | `98.94% <ø> (+10.42%)` | :arrow_up: |
| [...rflow/providers/google/cloud/hooks/bigquery\_dts.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2JpZ3F1ZXJ5X2R0cy5weQ==) | `86.00% <ø> (+6.68%)` | :arrow_up: |
| [.../providers/google/cloud/hooks/cloud\_memorystore.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX21lbW9yeXN0b3JlLnB5) | `74.07% <ø> (ø)` | |
| [...irflow/providers/google/cloud/hooks/datacatalog.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFjYXRhbG9nLnB5) | `91.74% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/datafusion.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFmdXNpb24ucHk=) | `84.53% <ø> (+3.22%)` | :arrow_up: |
| [airflow/providers/google/cloud/hooks/dataproc.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFwcm9jLnB5) | `98.60% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/dlp.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RscC5weQ==) | `98.55% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/mlengine.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL21sZW5naW5lLnB5) | `82.74% <ø> (+1.27%)` | :arrow_up: |
| ... and [32 more](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=footer). Last update [77412ac...f33e29e](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609466462
I was also hesitant about writing a mypy plugin, even a simple one, but as far as I can tell, it's the only way to write decorators that don't produce callables with type `Callable[..., Any]`. I also think it's not ideal that all decorated functions in the airflow codebase have that type. For example, users might see that `PubSubHook.publish` has type annotations and expect that mypy will catch type errors. But that's not the case, since the `fallback_to_default_project_id` decorator doesn't preserve function types. Adding the mypy plugin also caused the type checker to find some type errors in the codebase that it couldn't catch previously, again because decorated functions are effectively untyped.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406383403
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -585,10 +579,10 @@ def start_python_dataflow( # pylint: disable=too-many-arguments
variables: Dict,
dataflow: str,
py_options: List[str],
+ project_id: str,
Review comment:
I think it needs an entry in UPDATING.md. If someone uses positional args - it will break.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406382787
##########
File path: airflow/providers/google/cloud/hooks/datacatalog.py
##########
@@ -659,7 +659,7 @@ def list_tags(
location: str,
entry_group: str,
entry: str,
- page_size: int = 100,
+ page_size: Optional[int] = 100,
Review comment:
Is it optional really ? I think it's quite ok to set it as mandatory since it has default
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609463052
I have mixed feelings... First, I am not a big fan of adding more and more custom plugins (especially to solve problems that should be solved on language level #static_types_fan) . Especially to solve problems that I am not even sure are real problems. Since we have the `if` clause I even started to like the "explicit" information about `project_id` ("at this point, it will be provided by decorator").
On the other hand, this change does no harm. It makes our codebase bigger and adds next moving part to it. So, I am not sure.
I am curious about what others think? @mik-laj @kaxil @ashb @potiuk
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406995108
##########
File path: airflow/providers/google/cloud/example_dags/example_automl_tables.py
##########
@@ -139,7 +139,6 @@ def get_target_column_spec(columns_specs: List[Dict], column_name: str) -> str:
task_id="update_dataset_task",
dataset=update,
location=GCP_AUTOML_LOCATION,
- project_id=GCP_PROJECT_ID,
Review comment:
This parameter was unused in `CloudAutoMLHook.update_dataset`, so I dropped the parameter from that method, as well as `AutoMLTablesUpdateDatasetOperator` and the example code here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612365354
@turbaszek @mik-laj -> looks good to me. Can you take another look?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407003212
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -730,10 +718,10 @@ def is_job_dataflow_running(self, name: str, variables: Dict, project_id: Option
@GoogleBaseHook.fallback_to_default_project_id
def cancel_job(
self,
+ project_id: str,
Review comment:
It is the only keyword argument method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk merged pull request #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on a change in pull request #8145:
Drop redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407044132
##########
File path: airflow/providers/google/cloud/hooks/bigtable.py
##########
@@ -54,7 +54,7 @@ def _get_client(self, project_id: str):
return self._client
@GoogleBaseHook.fallback_to_default_project_id
- def get_instance(self, instance_id: str, project_id: Optional[str] = None) -> Instance:
+ def get_instance(self, instance_id: str, project_id: str) -> Instance:
Review comment:
@jmcarp I checked the changes and in some places where we use the decorator we are using `project_id: str` and in others `project_id: Optional[str] = None`. I think we should be consistent :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407034896
##########
File path: tests/providers/google/cloud/hooks/test_cloud_sql.py
##########
@@ -561,29 +513,6 @@ def test_get_instance_overridden_project_id(
execute_method.assert_called_once_with(num_retries=5)
wait_for_operation_to_complete.assert_not_called()
- @mock.patch(
- 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook.project_id',
- new_callable=PropertyMock,
- return_value=None
- )
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook.get_conn')
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook._wait_for_operation_to_complete')
- def test_get_instance_missing_project_id(
- self, wait_for_operation_to_complete, get_conn, mock_project_id
- ):
- get_method = get_conn.return_value.instances.return_value.get
- execute_method = get_method.return_value.execute
- execute_method.return_value = {"name": "instance"}
- wait_for_operation_to_complete.return_value = None
- with self.assertRaises(AirflowException) as cm:
- self.cloudsql_hook_no_default_project_id.get_instance(
- instance='instance')
- get_method.assert_not_called()
- execute_method.assert_not_called()
- err = cm.exception
- self.assertIn("The project id must be passed", str(err))
- wait_for_operation_to_complete.assert_not_called()
-
Review comment:
Agree. No need to repeat it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407006276
##########
File path: tests/providers/google/cloud/hooks/test_cloud_sql.py
##########
@@ -561,29 +513,6 @@ def test_get_instance_overridden_project_id(
execute_method.assert_called_once_with(num_retries=5)
wait_for_operation_to_complete.assert_not_called()
- @mock.patch(
- 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook.project_id',
- new_callable=PropertyMock,
- return_value=None
- )
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook.get_conn')
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook._wait_for_operation_to_complete')
- def test_get_instance_missing_project_id(
- self, wait_for_operation_to_complete, get_conn, mock_project_id
- ):
- get_method = get_conn.return_value.instances.return_value.get
- execute_method = get_method.return_value.execute
- execute_method.return_value = {"name": "instance"}
- wait_for_operation_to_complete.return_value = None
- with self.assertRaises(AirflowException) as cm:
- self.cloudsql_hook_no_default_project_id.get_instance(
- instance='instance')
- get_method.assert_not_called()
- execute_method.assert_not_called()
- err = cm.exception
- self.assertIn("The project id must be passed", str(err))
- wait_for_operation_to_complete.assert_not_called()
-
Review comment:
We have a large number of similar tests that all basically test the `fallback_to_default_project_id` decorator. I think it's sufficient to test the decorator on its own--I don't think we need to test every method that it's applied to.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612794117
Thanks @jmcarp! Great change!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609396663
Sure, would be awesome to remove it. But typehints with decorators are tricky :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609396993
Hold my :beer: and :cat:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609405070
Yep. That's my secret plan. Just started to write it :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406384956
##########
File path: airflow/providers/google/cloud/operators/automl.py
##########
@@ -568,7 +568,6 @@ def __init__(
self,
dataset: dict,
location: str,
- project_id: Optional[str] = None,
Review comment:
Should be removed from the docstring and mentioned in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406380707
##########
File path: airflow/providers/google/cloud/hooks/automl.py
##########
@@ -499,9 +481,6 @@ def update_dataset(
:param update_mask: The update mask applies to the resource. If a dict is provided, it must
be of the same form as the protobuf message FieldMask.
:type update_mask: Union[dict, FieldMask]
- :param project_id: ID of the Google Cloud project where dataset is located if None then
Review comment:
While this is correct (seems that update_dataset does not need project_id) I think it should have an entry in 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612370458
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=h1) Report
> Merging [#8145](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/12b9b64e10626bb1bc382815edeac9cbedd2701a&el=desc) will **decrease** coverage by `0.23%`.
> The diff coverage is `27.65%`.
[](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8145 +/- ##
==========================================
- Coverage 88.32% 88.08% -0.24%
==========================================
Files 936 937 +1
Lines 45319 45195 -124
==========================================
- Hits 40029 39812 -217
- Misses 5290 5383 +93
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/mypy/plugin/decorators.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9teXB5L3BsdWdpbi9kZWNvcmF0b3JzLnB5) | `0.00% <0.00%> (ø)` | |
| [...google/cloud/example\_dags/example\_automl\_tables.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2V4YW1wbGVfZGFncy9leGFtcGxlX2F1dG9tbF90YWJsZXMucHk=) | `92.72% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/automl.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2F1dG9tbC5weQ==) | `98.94% <ø> (+10.42%)` | :arrow_up: |
| [...rflow/providers/google/cloud/hooks/bigquery\_dts.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2JpZ3F1ZXJ5X2R0cy5weQ==) | `86.00% <ø> (+6.68%)` | :arrow_up: |
| [...irflow/providers/google/cloud/hooks/cloud\_build.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX2J1aWxkLnB5) | `100.00% <ø> (+2.56%)` | :arrow_up: |
| [airflow/providers/google/cloud/hooks/cloud\_sql.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX3NxbC5weQ==) | `67.58% <ø> (+0.33%)` | :arrow_up: |
| [...ogle/cloud/hooks/cloud\_storage\_transfer\_service.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX3N0b3JhZ2VfdHJhbnNmZXJfc2VydmljZS5weQ==) | `89.67% <ø> (+0.99%)` | :arrow_up: |
| [...irflow/providers/google/cloud/hooks/datacatalog.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFjYXRhbG9nLnB5) | `91.74% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/datafusion.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFmdXNpb24ucHk=) | `84.53% <ø> (+3.22%)` | :arrow_up: |
| [airflow/providers/google/cloud/hooks/mlengine.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL21sZW5naW5lLnB5) | `82.74% <ø> (+1.27%)` | :arrow_up: |
| ... and [23 more](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=footer). Last update [12b9b64...e0f61d7](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406387520
##########
File path: tests/providers/google/cloud/hooks/test_cloud_sql.py
##########
@@ -561,29 +513,6 @@ def test_get_instance_overridden_project_id(
execute_method.assert_called_once_with(num_retries=5)
wait_for_operation_to_complete.assert_not_called()
- @mock.patch(
- 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook.project_id',
- new_callable=PropertyMock,
- return_value=None
- )
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook.get_conn')
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook._wait_for_operation_to_complete')
- def test_get_instance_missing_project_id(
- self, wait_for_operation_to_complete, get_conn, mock_project_id
- ):
- get_method = get_conn.return_value.instances.return_value.get
- execute_method = get_method.return_value.execute
- execute_method.return_value = {"name": "instance"}
- wait_for_operation_to_complete.return_value = None
- with self.assertRaises(AirflowException) as cm:
- self.cloudsql_hook_no_default_project_id.get_instance(
- instance='instance')
- get_method.assert_not_called()
- execute_method.assert_not_called()
- err = cm.exception
- self.assertIn("The project id must be passed", str(err))
- wait_for_operation_to_complete.assert_not_called()
-
Review comment:
I think it makes sense to remove them - they are all testing the missing project_id which is not really possible in reality.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406381557
##########
File path: airflow/providers/google/cloud/hooks/bigtable.py
##########
@@ -54,7 +54,7 @@ def _get_client(self, project_id: str):
return self._client
@GoogleBaseHook.fallback_to_default_project_id
- def get_instance(self, instance_id: str, project_id: Optional[str] = None) -> Instance:
+ def get_instance(self, instance_id: str, project_id: str) -> Instance:
Review comment:
Yeah. Agree. project_id: str is better IMHO. it is semantically correct - we expect project_id to be set. And it is used most of the time below.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609978516
I don't know if we can programmatically audit use of the plugin, but the number of decorators is small and shouldn't change often, and it's simple enough to check by hand by grepping for `wraps`. I can add more decorators to the plugin here, but this patch is already pretty big, so I was thinking of adding `provide_session` and other decorators in a follow-up if we accept this change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407034741
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -585,10 +579,10 @@ def start_python_dataflow( # pylint: disable=too-many-arguments
variables: Dict,
dataflow: str,
py_options: List[str],
+ project_id: str,
Review comment:
Ah.. Right :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407002760
##########
File path: tests/providers/google/cloud/hooks/test_cloud_sql.py
##########
@@ -561,29 +513,6 @@ def test_get_instance_overridden_project_id(
execute_method.assert_called_once_with(num_retries=5)
wait_for_operation_to_complete.assert_not_called()
- @mock.patch(
- 'airflow.providers.google.common.hooks.base_google.GoogleBaseHook.project_id',
- new_callable=PropertyMock,
- return_value=None
- )
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook.get_conn')
- @mock.patch('airflow.providers.google.cloud.hooks.cloud_sql.CloudSQLHook._wait_for_operation_to_complete')
- def test_get_instance_missing_project_id(
- self, wait_for_operation_to_complete, get_conn, mock_project_id
- ):
- get_method = get_conn.return_value.instances.return_value.get
- execute_method = get_method.return_value.execute
- execute_method.return_value = {"name": "instance"}
- wait_for_operation_to_complete.return_value = None
- with self.assertRaises(AirflowException) as cm:
- self.cloudsql_hook_no_default_project_id.get_instance(
- instance='instance')
- get_method.assert_not_called()
- execute_method.assert_not_called()
- err = cm.exception
- self.assertIn("The project id must be passed", str(err))
- wait_for_operation_to_complete.assert_not_called()
-
Review comment:
It's possible in reality. For example: If you use ``gcloud`` for authorization but you do not set the ``core/project`` option in gcloud, project_id will be empty. It is not guaranteed that every service account key must have project_id. Keys that were generated a long time ago do not have this field.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407034827
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -585,10 +579,10 @@ def start_python_dataflow( # pylint: disable=too-many-arguments
variables: Dict,
dataflow: str,
py_options: List[str],
+ project_id: str,
Review comment:
Totally forgot about it ! It was some 1.5 year ago! Thanks @mik-laj for reminding :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407034614
##########
File path: airflow/providers/google/cloud/hooks/automl.py
##########
@@ -499,9 +481,6 @@ def update_dataset(
:param update_mask: The update mask applies to the resource. If a dict is provided, it must
be of the same form as the protobuf message FieldMask.
:type update_mask: Union[dict, FieldMask]
- :param project_id: ID of the Google Cloud project where dataset is located if None then
Review comment:
Right !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406386438
##########
File path: airflow/providers/google/cloud/operators/dataflow.py
##########
@@ -437,6 +443,7 @@ def __init__( # pylint: disable=too-many-arguments
py_options: Optional[List[str]] = None,
py_requirements: Optional[List[str]] = None,
py_system_site_packages: bool = False,
+ project_id: Optional[str] = None,
Review comment:
Needs UPDATING.md entry
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609452801
I added a simple mypy plugin to preserve types for decorated functions, and to update types when decorators change function signatures. For example, `fallback_to_default_project_id` effectively changes the type of the `project_id` argument from `str` to `Optional[str]`. This also fixes the problem of decorators changing the types of all decorated callables to `Callable[..., Any]`. Still a WIP, but what do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609393518
@jmcarp those checks are only to please mypy. Initially, we used `assert project_id` but since we don't use `assert` in production code we have if clause...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609401822
> Hold my 🍺 and 🐱
Are you going to propose PEP to make Python statically typed language? 😅
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406384244
##########
File path: airflow/providers/google/cloud/hooks/stackdriver.py
##########
@@ -433,8 +433,8 @@ def list_notification_channels(
@GoogleBaseHook.fallback_to_default_project_id
def _toggle_channel_status(
self,
- new_state: str,
- project_id: Optional[str] = None,
+ new_state: bool,
Review comment:
Nice too !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612383137
I still have concerns about type annotations consistency:
https://github.com/apache/airflow/pull/8145#discussion_r407044132
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612370458
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=h1) Report
> Merging [#8145](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/77412ac6ea3486673d5e2340d10b355e12317fc0&el=desc) will **decrease** coverage by `0.40%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8145 +/- ##
==========================================
- Coverage 88.33% 87.93% -0.41%
==========================================
Files 936 937 +1
Lines 45321 45197 -124
==========================================
- Hits 40036 39743 -293
- Misses 5285 5454 +169
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/mypy/plugin/decorators.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9teXB5L3BsdWdpbi9kZWNvcmF0b3JzLnB5) | `0.00% <0.00%> (ø)` | |
| [...google/cloud/example\_dags/example\_automl\_tables.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2V4YW1wbGVfZGFncy9leGFtcGxlX2F1dG9tbF90YWJsZXMucHk=) | `92.72% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/automl.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2F1dG9tbC5weQ==) | `98.94% <ø> (+10.42%)` | :arrow_up: |
| [...rflow/providers/google/cloud/hooks/bigquery\_dts.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2JpZ3F1ZXJ5X2R0cy5weQ==) | `86.00% <ø> (+6.68%)` | :arrow_up: |
| [.../providers/google/cloud/hooks/cloud\_memorystore.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX21lbW9yeXN0b3JlLnB5) | `74.07% <ø> (ø)` | |
| [...irflow/providers/google/cloud/hooks/datacatalog.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFjYXRhbG9nLnB5) | `91.74% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/datafusion.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFmdXNpb24ucHk=) | `84.53% <ø> (+3.22%)` | :arrow_up: |
| [airflow/providers/google/cloud/hooks/dataproc.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFwcm9jLnB5) | `98.60% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/dlp.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RscC5weQ==) | `98.55% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/mlengine.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL21sZW5naW5lLnB5) | `82.74% <ø> (+1.27%)` | :arrow_up: |
| ... and [32 more](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=footer). Last update [77412ac...f33e29e](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on a change in pull request #8145:
Drop redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r405292992
##########
File path: airflow/providers/google/cloud/example_dags/example_automl_tables.py
##########
@@ -139,7 +139,6 @@ def get_target_column_spec(columns_specs: List[Dict], column_name: str) -> str:
task_id="update_dataset_task",
dataset=update,
location=GCP_AUTOML_LOCATION,
- project_id=GCP_PROJECT_ID,
Review comment:
Is this change necessary?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609394462
Some more would not hurt :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406995719
##########
File path: airflow/providers/google/cloud/hooks/datacatalog.py
##########
@@ -659,7 +659,7 @@ def list_tags(
location: str,
entry_group: str,
entry: str,
- page_size: int = 100,
+ page_size: Optional[int] = 100,
Review comment:
We mark this as `Optional[int]` in `CloudDataCatalogListTagsOperator`, so if we don't also mark it as `Optional[int]` here, mypy errors:
```
airflow/providers/google/cloud/operators/datacatalog.py:1287: error: Argument "page_size" to "list_tags" of "CloudDataCatalogHook" has incompatible type "Optional[int]"; expected "int"
```
This is the kind of type error that mypy wasn't catching before because the decorator on the hook method dropped type information about the method, by the way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #8145: Drop redundant project id
null checks.
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609464491
Quick thought: The same issue about project_id also applies to session/provide_session
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406384391
##########
File path: airflow/providers/google/cloud/hooks/stackdriver.py
##########
@@ -500,8 +500,8 @@ def enable_notification_channels(
@GoogleBaseHook.fallback_to_default_project_id
def disable_notification_channels(
self,
+ project_id: str,
Review comment:
UPDATING.md entry needed here as well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612370458
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=h1) Report
> Merging [#8145](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/77412ac6ea3486673d5e2340d10b355e12317fc0&el=desc) will **decrease** coverage by `0.40%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8145 +/- ##
==========================================
- Coverage 88.33% 87.93% -0.41%
==========================================
Files 936 937 +1
Lines 45321 45197 -124
==========================================
- Hits 40036 39743 -293
- Misses 5285 5454 +169
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/mypy/plugin/decorators.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9teXB5L3BsdWdpbi9kZWNvcmF0b3JzLnB5) | `0.00% <0.00%> (ø)` | |
| [...google/cloud/example\_dags/example\_automl\_tables.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2V4YW1wbGVfZGFncy9leGFtcGxlX2F1dG9tbF90YWJsZXMucHk=) | `92.72% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/automl.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2F1dG9tbC5weQ==) | `98.94% <ø> (+10.42%)` | :arrow_up: |
| [...rflow/providers/google/cloud/hooks/bigquery\_dts.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2JpZ3F1ZXJ5X2R0cy5weQ==) | `86.00% <ø> (+6.68%)` | :arrow_up: |
| [.../providers/google/cloud/hooks/cloud\_memorystore.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX21lbW9yeXN0b3JlLnB5) | `74.07% <ø> (ø)` | |
| [...irflow/providers/google/cloud/hooks/datacatalog.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFjYXRhbG9nLnB5) | `91.74% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/datafusion.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFmdXNpb24ucHk=) | `84.53% <ø> (+3.22%)` | :arrow_up: |
| [airflow/providers/google/cloud/hooks/dataproc.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFwcm9jLnB5) | `98.60% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/dlp.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RscC5weQ==) | `98.55% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/mlengine.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL21sZW5naW5lLnB5) | `82.74% <ø> (+1.27%)` | :arrow_up: |
| ... and [32 more](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=footer). Last update [77412ac...f33e29e](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on a change in pull request #8145:
Drop redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406473607
##########
File path: airflow/providers/google/cloud/hooks/automl.py
##########
@@ -499,9 +481,6 @@ def update_dataset(
:param update_mask: The update mask applies to the resource. If a dict is provided, it must
be of the same form as the protobuf message FieldMask.
:type update_mask: Union[dict, FieldMask]
- :param project_id: ID of the Google Cloud project where dataset is located if None then
Review comment:
@potiuk no need I think, automl is master only
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406383572
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -730,10 +718,10 @@ def is_job_dataflow_running(self, name: str, variables: Dict, project_id: Option
@GoogleBaseHook.fallback_to_default_project_id
def cancel_job(
self,
+ project_id: str,
Review comment:
Same here -> I think it needs UPDATING.md entry
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609394026
Just thinking - maybe we can do it differently and find a way to both - keep mypy happy and remove the ifs... Let me think about it a bit :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407035131
##########
File path: airflow/providers/google/cloud/example_dags/example_automl_tables.py
##########
@@ -139,7 +139,6 @@ def get_target_column_spec(columns_specs: List[Dict], column_name: str) -> str:
task_id="update_dataset_task",
dataset=update,
location=GCP_AUTOML_LOCATION,
- project_id=GCP_PROJECT_ID,
Review comment:
Yeah. I thik it's good :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407003143
##########
File path: airflow/providers/google/cloud/hooks/dataflow.py
##########
@@ -585,10 +579,10 @@ def start_python_dataflow( # pylint: disable=too-many-arguments
variables: Dict,
dataflow: str,
py_options: List[str],
+ project_id: str,
Review comment:
This method must be called using keywords arguments. Please look at this expectation added by @potiuk https://github.com/apache/airflow/blob/master/airflow/providers/google/common/hooks/base_google.py#L333-L336
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612298753
If you would like to add support for more decorators, the most popular decorator is `apply_defaullts. However, this will be best done in a separate PR. This is very problematic and has made it difficult for me to introduce metaclasses - https://github.com/apache/airflow/pull/7450
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on a change in pull request #8145:
Drop redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r405294260
##########
File path: airflow/providers/google/cloud/hooks/bigtable.py
##########
@@ -54,7 +54,7 @@ def _get_client(self, project_id: str):
return self._client
@GoogleBaseHook.fallback_to_default_project_id
- def get_instance(self, instance_id: str, project_id: Optional[str] = None) -> Instance:
+ def get_instance(self, instance_id: str, project_id: str) -> Instance:
Review comment:
Let's decide what we want to use: `str` or `Optional[str] = None` and be cosistent
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on a change in pull request #8145:
Drop redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406473921
##########
File path: airflow/providers/google/cloud/hooks/stackdriver.py
##########
@@ -500,8 +500,8 @@ def enable_notification_channels(
@GoogleBaseHook.fallback_to_default_project_id
def disable_notification_channels(
self,
+ project_id: str,
Review comment:
@potiuk was this hook released?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r407034707
##########
File path: airflow/providers/google/cloud/hooks/datacatalog.py
##########
@@ -659,7 +659,7 @@ def list_tags(
location: str,
entry_group: str,
entry: str,
- page_size: int = 100,
+ page_size: Optional[int] = 100,
Review comment:
OK. Making MyPy happy :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612530509
Thanks for reviewing. Is it all right to merge while `TestCliTaskBackfill.test_run_ignores_all_dependencies` is failing? It looks unrelated to this patch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406378752
##########
File path: airflow/providers/google/cloud/example_dags/example_automl_tables.py
##########
@@ -139,7 +139,6 @@ def get_target_column_spec(columns_specs: List[Dict], column_name: str) -> str:
task_id="update_dataset_task",
dataset=update,
location=GCP_AUTOML_LOCATION,
- project_id=GCP_PROJECT_ID,
Review comment:
Yeah. I think we should leave that in.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] turbaszek commented on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609394175
@potiuk I think @mik-laj already gave it a lot of thought
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp edited a comment on issue #8145: Drop redundant
project id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp edited a comment on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609452801
I added a simple mypy plugin to preserve types for decorated functions, and to update types when decorators change function signatures. For example, `fallback_to_default_project_id` effectively changes the type of the `project_id` argument from `str` to `Optional[str]`. This also fixes the problem of decorators changing the types of all decorated callables to `Callable[..., Any]`. Still a WIP, but what do you think?
By the way, mypy might support this kind of behavior natively at some point, but this issue has been open for a long time: https://github.com/python/mypy/issues/3157.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406384068
##########
File path: airflow/providers/google/cloud/hooks/stackdriver.py
##########
@@ -186,10 +186,10 @@ def enable_alert_policies(
@GoogleBaseHook.fallback_to_default_project_id
def disable_alert_policies(
self,
- project_id: Optional[str] = None,
+ project_id: str,
filter_: Optional[str] = None,
retry: Optional[str] = DEFAULT,
- timeout: Optional[str] = DEFAULT,
+ timeout: Optional[float] = DEFAULT,
Review comment:
Nice!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#discussion_r406378810
##########
File path: airflow/mypy/plugin/decorators.py
##########
@@ -0,0 +1,79 @@
+#
Review comment:
I like this. I think we should open-up to more custom plugins for mypy/pylint. We'd learn how to develop them and this will help us when we will do automated checks for Dags to see if they are 2.0 compliant. I think it's good to have that in our toolbox.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] jmcarp commented on issue #8145: Drop redundant project
id null checks.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-609496780
One more thing: one of the mypy developers suggests using mypy plugins to preserve types for decorated functions until mypy supports this kind of thing: https://github.com/python/mypy/issues/1927#issuecomment-305152052.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #8145: Drop
redundant project id null checks.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8145: Drop redundant project id null checks.
URL: https://github.com/apache/airflow/pull/8145#issuecomment-612370458
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=h1) Report
> Merging [#8145](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/77412ac6ea3486673d5e2340d10b355e12317fc0&el=desc) will **decrease** coverage by `0.40%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8145 +/- ##
==========================================
- Coverage 88.33% 87.93% -0.41%
==========================================
Files 936 937 +1
Lines 45321 45197 -124
==========================================
- Hits 40036 39743 -293
- Misses 5285 5454 +169
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/mypy/plugin/decorators.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9teXB5L3BsdWdpbi9kZWNvcmF0b3JzLnB5) | `0.00% <0.00%> (ø)` | |
| [...google/cloud/example\_dags/example\_automl\_tables.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2V4YW1wbGVfZGFncy9leGFtcGxlX2F1dG9tbF90YWJsZXMucHk=) | `92.72% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/automl.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2F1dG9tbC5weQ==) | `98.94% <ø> (+10.42%)` | :arrow_up: |
| [...rflow/providers/google/cloud/hooks/bigquery\_dts.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2JpZ3F1ZXJ5X2R0cy5weQ==) | `86.00% <ø> (+6.68%)` | :arrow_up: |
| [.../providers/google/cloud/hooks/cloud\_memorystore.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2Nsb3VkX21lbW9yeXN0b3JlLnB5) | `74.07% <ø> (ø)` | |
| [...irflow/providers/google/cloud/hooks/datacatalog.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFjYXRhbG9nLnB5) | `91.74% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/datafusion.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFmdXNpb24ucHk=) | `84.53% <ø> (+3.22%)` | :arrow_up: |
| [airflow/providers/google/cloud/hooks/dataproc.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RhdGFwcm9jLnB5) | `98.60% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/dlp.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL2RscC5weQ==) | `98.55% <ø> (ø)` | |
| [airflow/providers/google/cloud/hooks/mlengine.py](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvZ29vZ2xlL2Nsb3VkL2hvb2tzL21sZW5naW5lLnB5) | `82.74% <ø> (+1.27%)` | :arrow_up: |
| ... and [32 more](https://codecov.io/gh/apache/airflow/pull/8145/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=footer). Last update [77412ac...f33e29e](https://codecov.io/gh/apache/airflow/pull/8145?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services