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/07/08 18:05:20 UTC
[GitHub] [airflow] josh-fell opened a new pull request, #24931: Apply flake8-logging-format changes to tests
josh-fell opened a new pull request, #24931:
URL: https://github.com/apache/airflow/pull/24931
Related: #23597 #24910
This is some pre-work for hopefully including the [flake8-logging-format](https://github.com/globality-corp/flake8-logging-format) extension in CI. There are roughly 88 file changes so splitting these up into smaller chunks.
After the initial sweeps are completed we can do a final pass when formally implementing the extension in CI.
---
**^ 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 changes, an 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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r917055812
##########
tests/utils/test_compression.py:
##########
@@ -54,8 +54,8 @@ def setUp(self):
f_bz2.writelines([header, line1, line2])
# Base Exception so it catches Keyboard Interrupt
- except BaseException as e:
- logging.error(e)
+ except BaseException:
+ logging.exception("An exception has occurred.")
Review Comment:
But perhaps going this route for core and provider modules, we get into a situation where log handlers don't output the same (possibly useful?) information. I'd love feedback 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r935868845
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
This log really adds no value here.
After looking at at that - It should be REALLY ok to just remove the except clause altogether. It's not needed here at all.
The session.close() method in finally will automatically rollback the transaction, there is no need to do it manually in "except" clause https://docs.sqlalchemy.org/en/14/orm/session_basics.html#closing:
> The close() method issues a expunge_all(), and [releases](http://docs.sqlalchemy.org/en/latest/glossary.html#term-releases) any transactional/connection resources. When connections are returned to the connection pool, transactional state is rolled back 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r937627625
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Yep. That should do :)
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r917040105
##########
tests/utils/test_compression.py:
##########
@@ -54,8 +54,8 @@ def setUp(self):
f_bz2.writelines([header, line1, line2])
# Base Exception so it catches Keyboard Interrupt
- except BaseException as e:
- logging.error(e)
+ except BaseException:
+ logging.exception("An exception has occurred.")
Review Comment:
The flake8-logging-format extension isn't fond of using the exception itself in the logging message.
`G200 Logging statements should not include the exception in logged string (use exception or exc_info=True)`
--
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] potiuk commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r935868845
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
This log really adds no value here.
After looking at at that - It should be REALLY ok to just remove the except clause altogether. It's not needed here at all.
The session.close() method in finally will automatically rollback the transaction, there is no need to do it manually in "except" clausehttps://docs.sqlalchemy.org/en/14/orm/session_basics.html#closing:
> The close() method issues a expunge_all(), and [releases](http://docs.sqlalchemy.org/en/latest/glossary.html#term-releases) any transactional/connection resources. When connections are returned to the connection pool, transactional state is rolled back 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24931:
URL: https://github.com/apache/airflow/pull/24931
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r936179220
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Being a SQLAlchemy novice here, but it looks like using a context manager here for the session object should suffice? And the `session.commit()` doesn't seem necessary since there is only read activity with the session. Unless I'm missing something looking at the docs.
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r933608141
##########
tests/utils/test_compression.py:
##########
@@ -54,8 +54,8 @@ def setUp(self):
f_bz2.writelines([header, line1, line2])
# Base Exception so it catches Keyboard Interrupt
- except BaseException as e:
- logging.error(e)
+ except BaseException:
+ logging.exception("An exception has occurred.")
Review Comment:
Shuffled some pieces around and now this test is using a fixture for teardown activity + converted to `pytest` since I was messing around anyway.
--
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] potiuk commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r918703890
##########
tests/providers/apache/hive/transfers/test_s3_to_hive.py:
##########
@@ -107,8 +107,8 @@ def setUp(self):
self._set_fn(fn_bz2, '.bz2', False)
f_bz2_nh.writelines([line1, line2])
# Base Exception so it catches Keyboard Interrupt
- except BaseException as e:
- logging.error(e)
+ except BaseException:
Review Comment:
This is rather strange - why do we need except/calling tearDown at all? tearDown should be called regardless (and BTW. we should convert it to fixture providing tmpdir (and cleaning 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r918705658
##########
tests/utils/test_compression.py:
##########
@@ -54,8 +54,8 @@ def setUp(self):
f_bz2.writelines([header, line1, line2])
# Base Exception so it catches Keyboard Interrupt
- except BaseException as e:
- logging.error(e)
+ except BaseException:
+ logging.exception("An exception has occurred.")
Review Comment:
And here 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r933608261
##########
tests/providers/apache/hive/transfers/test_s3_to_hive.py:
##########
@@ -107,8 +107,8 @@ def setUp(self):
self._set_fn(fn_bz2, '.bz2', False)
f_bz2_nh.writelines([line1, line2])
# Base Exception so it catches Keyboard Interrupt
- except BaseException as e:
- logging.error(e)
+ except BaseException:
Review Comment:
Same shuffling and conversion to `pytest` 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r918705328
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Same here. we do not need to catch BaseException here, we just need a fixture that calls rollback() (it is safe to call rollback even after commit) . then we do not heed to have try/except at all.
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -133,8 +133,8 @@ def set_dictionary_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Same 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r933609230
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -133,8 +133,8 @@ def set_dictionary_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Updating this testing util I am a little apprehensive about. I'm not sure of the blast radius for updating. Would it be OK to not include this module in the fixture updates?
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r917056072
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
But perhaps going this route for core and provider modules, we get into a situation where log handlers don't output the same (possibly useful?) information. I'd love feedback 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r936179220
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Being a SQLAlchemy novice here, but it looks like using a context manager for the `Session` object should suffice? And the `session.commit()` doesn't seem necessary since there is only read activity with the session. Unless I'm missing something looking at the docs.
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r917039113
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
I thought that the `raise` was sufficient here for the additional traceback information which includes the exception message. Generally I was thinking to keep `logging.error()` when there was an accompanying `raise`, but use `logging.exception()` or using `exc_info=True` when there isn't. Happy to hear other thoughts of course!
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r917039113
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -107,8 +107,8 @@ def set_key_path_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra if self.project_extra else self.project_id
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
I thought that the `raise` was sufficient here for the additional traceback information which includes the exception message. Generally I was thinking to keep `logging.error()` when there was an accompanying `raise`, but use `logging.exception()` when there isn't. Happy to hear other thoughts of course!
--
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] josh-fell commented on a diff in pull request #24931: Apply flake8-logging-format changes to tests
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24931:
URL: https://github.com/apache/airflow/pull/24931#discussion_r933609230
##########
tests/providers/google/cloud/utils/gcp_authenticator.py:
##########
@@ -133,8 +133,8 @@ def set_dictionary_in_airflow_connection(self):
extras[PROJECT_EXTRA] = self.project_extra
conn.extra = json.dumps(extras)
session.commit()
- except BaseException as ex:
- self.log.error('Airflow DB Session error: %s', str(ex))
+ except BaseException:
+ self.log.error('Airflow DB Session error.')
Review Comment:
Updating this testing util I am a little apprehensive about. I'm not sure of the blast radius for updating. Would it be OK to not include this module in the fixture updates within the scope of this PR?
--
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