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/11 19:05:46 UTC
[GitHub] [airflow] josh-fell opened a new pull request, #24977: Apply flake8-logging-format changes to core
josh-fell opened a new pull request, #24977:
URL: https://github.com/apache/airflow/pull/24977
Related: #23597 #24910
This is some pre-work for hopefully/maybe 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.
I'm not entirely certain on if there are impacts to useful info that will be missed with log handlers but would love feedback on general patterns.
- [ ] Fix failing tests
---
**^ 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] potiuk commented on a diff in pull request #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r919185815
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
Because if it is FAB, then I'd simply exclude the whole file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24977:
URL: https://github.com/apache/airflow/pull/24977#issuecomment-1278312790
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24977:
URL: https://github.com/apache/airflow/pull/24977#issuecomment-1229592784
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r918908347
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
or:
```
log_message(Logging.WARNING,c.LOGMSG_ERR_SEC_CREATE_DB, e)
```
To account for different log levels? That would also allow to make it performant with "if log.level< level)"
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r921261934
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
Seems like FAB generally is a mix of both. It's copied and then modified accordingly.
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r919184691
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
I thought sqa/manager is ours :)
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r921313631
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
Yeah. Seen that In SecurityManager. :exploding_head:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #24977: Apply flake8-logging-format changes to core
URL: https://github.com/apache/airflow/pull/24977
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r918905222
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
Should we extract a common function doing the logging? Then we could ignore just once.
```
log_error(c.LOGMSG_ERR_SEC_CREATE_DB, e)
```
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r918905222
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
Should we extract a common function doing the logging.
```
log_error(c.LOGMSG_ERR_SEC_CREATE_DB, e)
```
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r919182374
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
I added the ignore because it was FAB and I assume we just copy directly without making modifications, but I could be wrong. I was thinking adding the ignores would be the least invasive.
--
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 #24977: Apply flake8-logging-format changes to core
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24977:
URL: https://github.com/apache/airflow/pull/24977#discussion_r919182989
##########
airflow/www/fab_security/sqla/manager.py:
##########
@@ -106,7 +106,7 @@ def create_db(self):
log.info(c.LOGMSG_INF_SEC_ADD_DB)
super().create_db()
except Exception as e:
- log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e)))
+ log.error(c.LOGMSG_ERR_SEC_CREATE_DB.format(str(e))) # noqa: G001
Review Comment:
The common logging function is a cool and intriguing idea though. Let's see what I can cook up.
--
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