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/09/24 17:50:59 UTC
[GitHub] [airflow] Sangarshanan opened a new pull request #11132: Add loggingconfig rule
Sangarshanan opened a new pull request #11132:
URL: https://github.com/apache/airflow/pull/11132
Closes: #11046
Adds LoggingConfigurationRule rule to upgrade/rules as per:
https://github.com/apache/airflow/blob/master/UPDATING.md#logging-configuration-has-been-moved-to-new-section
---
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/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
[GitHub] [airflow] turbaszek commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703326369
@Sangarshanan I think that the `Conditional MySQL Client installation (#11174)` and `Allow overrides for pod_template_file (#11162)` should not be part of your 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
[GitHub] [airflow] Sangarshanan commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
Sangarshanan commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495710052
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return (
+ "The following configurations have been to moved from [core] to the new [logging] section. \n"
+ "- base_log_folder \n"
+ "- remote_logging \n"
+ "- remote_log_conn_id \n"
+ "- remote_base_log_folder \n"
+ "- encrypt_s3_logs \n"
+ "- logging_level \n"
+ "- fab_logging_level \n"
+ "- logging_config_class \n"
+ "- colored_console_log \n"
+ "- colored_log_format \n"
+ "- colored_formatter_class \n"
+ "- log_format \n"
+ "- simple_log_format \n"
+ "- task_log_prefix_template \n"
+ "- log_filename_template \n"
+ "- log_processor_filename_template \n"
+ "- dag_processor_manager_log_location \n"
+ "- task_log_reader \n"
Review comment:
Ah I was not thinking of this cause this change is backwards compatible and not something that can break if not moved but you are right, showing only the incompatibility really helps while taking actions, I can do a comparison and return only the actively used keys but what of the default/ empty keys is it right that they still remain in the old section ?
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703486579
> @turbaszek This is an interesting problem that also occurs in other cases. I think that it is also worth solving separately. If you have a configuration file from Airflow 2 and you run it on Airflow 1.10, then you can't run: ʻairflow config`
I agree, but as you said, this should be done in separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703666844
All upgrade tests passed:
```
tests/upgrade/test_formattes.py . [ 86%]
706
tests/upgrade/test_problem.py .. [ 86%]
707
tests/upgrade/rules/test_base_rule.py . [ 86%]
708
tests/upgrade/rules/test_conn_id_is_unique.py . [ 86%]
709
tests/upgrade/rules/test_conn_type_is_not_nullable.py . [ 86%]
710
tests/upgrade/rules/test_fernet_enabled.py .. [ 86%]
711
tests/upgrade/rules/test_import_changes.py ... [ 86%]
712
tests/upgrade/rules/test_logging_configuration.py . [ 86%]
713
tests/upgrade/rules/test_send_grid_moved.py .. [ 86%]
````
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek edited a comment on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek edited a comment on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703483096
@Sangarshanan it seems that one test is failing:
```
_______________________________________ TestLoggingConfigurationRule.test_valid_check _______________________________________
/usr/local/lib/python3.7/contextlib.py:73: in inner
with self._recreate_cm():
/usr/local/lib/python3.7/contextlib.py:112: in __enter__
return next(self.gen)
tests/test_utils/config.py:45: in conf_vars
conf.set(section, key, value)
/usr/local/lib/python3.7/site-packages/backports/configparser/__init__.py:1239: in set
super(ConfigParser, self).set(section, option, value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <airflow.configuration.AirflowConfigParser object at 0x7fa366479bd0>, section = 'logging', option = 'base_log_folder'
value = 'DUMMY'
def set(self, section, option, value=None):
"""Set an option."""
if value:
value = self._interpolation.before_set(self, section, option,
value)
if not section or section == self.default_section:
sectdict = self._defaults
else:
try:
sectdict = self._sections[section]
except KeyError:
> raise from_none(NoSectionError(section))
E backports.configparser.NoSectionError: No section: 'logging'
```
Personally I would remove the `test_valid_check` test 👍
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek merged pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #11132:
URL: https://github.com/apache/airflow/pull/11132
----------------------------------------------------------------
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
[GitHub] [airflow] github-actions[bot] commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703414536
The CI and PROD Docker Images for the build are prepared in a separate "Build Image" workflow,
that you will not see in the list of checks (you will see "Wait for images" jobs instead).
You can checks the status of those images in [The workflow run](https://github.com/apache/airflow/actions/runs/288807554)
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r497341878
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,83 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf, AIRFLOW_HOME
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ logging_configs = [
+ ("base_log_folder", "{}/logs".format(AIRFLOW_HOME)),
+ ("remote_logging", "False"),
+ ("remote_log_conn_id", ""),
+ ("remote_base_log_folder", ""),
+ ("encrypt_s3_logs", "False"),
+ ("logging_level", "INFO"),
+ ("fab_logging_level", "WARN"),
+ ("logging_config_class", ""),
+ ("colored_console_log", "True"),
+ (
+ "colored_log_format",
+ "[%(blue)s%(asctime)s%(reset)s] {%(blue)s%(filename)s:%(reset)s%(lineno)d} "
+ "%(log_color)s%(levelname)s%(reset)s - %(log_color)s%(message)s%(reset)s",
+ ),
+ (
+ "colored_formatter_class",
+ "airflow.utils.log.colored_log.CustomTTYColoredFormatter",
+ ),
+ (
+ "log_format",
+ "[%(asctime)s] {%(filename)s:%(lineno)d} %(levelname)s - %(message)s",
+ ),
+ ("simple_log_format", "%(asctime)s %(levelname)s - %(message)s"),
+ ("task_log_prefix_template", ""),
+ (
+ "log_filename_template",
+ "{{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log",
+ ),
+ ("log_processor_filename_template", "{{ filename }}.log"),
+ (
+ "dag_processor_manager_log_location",
+ "{}/logs/dag_processor_manager/dag_processor_manager.log".format(
+ AIRFLOW_HOME
+ ),
+ ),
+ ("task_log_reader", "task"),
+ ]
+
+ mismatches = []
+ for logging_config, default in logging_configs:
+ if not conf.has_option("logging", logging_config) and conf.has_option(
+ "core", logging_config
+ ):
+ existing_config = conf.get("core", logging_config)
+ if existing_config != default:
+ mismatches.append(
+ "{} has been moved from [core] to a the new [logging] section.\n".format(
+ logging_config
+ )
+ )
+
+ if mismatches:
+ return "".join(mismatches)
Review comment:
```suggestion
return mismatches
```
We can return multiple errors or an empty list
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek edited a comment on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek edited a comment on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703483096
@Sangarshanan it seems that one test is failing
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703483096
@Sangarshanan it seems that one test is failing:
```
_ TestStatNameHandlerNotSupportedRule.test_check_failed[plugins0--stat_name_handler field is no longer supported in AirflowTestPlugin. stat_name_handler option in [scheduler] section in airflow.cfg should be used to achieve the same effect.] _
self = <tests.upgrade.rules.test_stat_name_handler_not_supported.TestStatNameHandlerNotSupportedRule object at 0x7f6224a8ca50>
plugins = [<class 'tests.upgrade.rules.test_stat_name_handler_not_supported.MockPluginWithStatNameHandler'>, <class 'tests.plugins.test_plugin.MockPluginA'>]
test_config = ''
expected = 'stat_name_handler field is no longer supported in AirflowTestPlugin. stat_name_handler option in [scheduler] section in airflow.cfg should be used to achieve the same effect.'
@pytest.mark.parametrize(
'plugins, test_config, expected',
[
(PLUGINS_WITH_STAT_NAME_HANDLER, "", CHECK_ERR_MSG),
(PLUGINS_WITHOUT_STAT_NAME_HANDLER, "", None),
(PLUGINS_WITHOUT_STAT_NAME_HANDLER, TEST_CONFIG, None),
(PLUGINS_WITH_STAT_NAME_HANDLER, TEST_CONFIG, None),
]
)
def test_check_failed(self, plugins, test_config, expected):
rule = StatNameHandlerNotSupportedRule()
assert isinstance(rule.description, str)
assert isinstance(rule.title, str)
test_conf = AirflowConfigParser()
test_conf.read_string(test_config)
mock_conf = mock.patch("airflow.upgrade.rules.stat_name_handler_not_supported.conf", test_conf)
with mock_plugin_manager(plugins=plugins), mock_conf:
msg = rule.check()
> assert msg == expected
E AssertionError: assert None == ('stat_name_handler field is no longer supported in AirflowTestPlugin. '\n 'stat_name_handler option in [scheduler] section in airflow.cfg should be '\n 'used to achieve the same effect.')
E +None
E -'stat_name_handler field is no longer supported in AirflowTestPlugin. stat_name_handler option in [scheduler] section in airflow.cfg should be used to achieve the same effect.'
```
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495568279
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,31 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return "The logging configurations have been to moved from [core] to the new [logging] section."
Review comment:
I think it would be good to tell users which keys should be moved
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r497341275
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,83 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf, AIRFLOW_HOME
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ logging_configs = [
+ ("base_log_folder", "{}/logs".format(AIRFLOW_HOME)),
+ ("remote_logging", "False"),
+ ("remote_log_conn_id", ""),
+ ("remote_base_log_folder", ""),
+ ("encrypt_s3_logs", "False"),
+ ("logging_level", "INFO"),
+ ("fab_logging_level", "WARN"),
+ ("logging_config_class", ""),
+ ("colored_console_log", "True"),
+ (
+ "colored_log_format",
+ "[%(blue)s%(asctime)s%(reset)s] {%(blue)s%(filename)s:%(reset)s%(lineno)d} "
+ "%(log_color)s%(levelname)s%(reset)s - %(log_color)s%(message)s%(reset)s",
+ ),
+ (
+ "colored_formatter_class",
+ "airflow.utils.log.colored_log.CustomTTYColoredFormatter",
+ ),
+ (
+ "log_format",
+ "[%(asctime)s] {%(filename)s:%(lineno)d} %(levelname)s - %(message)s",
+ ),
+ ("simple_log_format", "%(asctime)s %(levelname)s - %(message)s"),
+ ("task_log_prefix_template", ""),
+ (
+ "log_filename_template",
+ "{{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log",
+ ),
+ ("log_processor_filename_template", "{{ filename }}.log"),
+ (
+ "dag_processor_manager_log_location",
+ "{}/logs/dag_processor_manager/dag_processor_manager.log".format(
+ AIRFLOW_HOME
+ ),
+ ),
+ ("task_log_reader", "task"),
+ ]
+
+ mismatches = []
+ for logging_config, default in logging_configs:
+ if not conf.has_option("logging", logging_config) and conf.has_option(
+ "core", logging_config
+ ):
+ existing_config = conf.get("core", logging_config)
+ if existing_config != default:
+ mismatches.append(
+ "{} has been moved from [core] to a the new [logging] section.\n".format(
Review comment:
```suggestion
"{} has been moved from [core] to a the new [logging] section.".format(
```
----------------------------------------------------------------
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
[GitHub] [airflow] Sangarshanan commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
Sangarshanan commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r497606455
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,83 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf, AIRFLOW_HOME
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ logging_configs = [
+ ("base_log_folder", "{}/logs".format(AIRFLOW_HOME)),
+ ("remote_logging", "False"),
+ ("remote_log_conn_id", ""),
+ ("remote_base_log_folder", ""),
+ ("encrypt_s3_logs", "False"),
+ ("logging_level", "INFO"),
+ ("fab_logging_level", "WARN"),
+ ("logging_config_class", ""),
+ ("colored_console_log", "True"),
+ (
+ "colored_log_format",
+ "[%(blue)s%(asctime)s%(reset)s] {%(blue)s%(filename)s:%(reset)s%(lineno)d} "
+ "%(log_color)s%(levelname)s%(reset)s - %(log_color)s%(message)s%(reset)s",
+ ),
+ (
+ "colored_formatter_class",
+ "airflow.utils.log.colored_log.CustomTTYColoredFormatter",
+ ),
+ (
+ "log_format",
+ "[%(asctime)s] {%(filename)s:%(lineno)d} %(levelname)s - %(message)s",
+ ),
+ ("simple_log_format", "%(asctime)s %(levelname)s - %(message)s"),
+ ("task_log_prefix_template", ""),
+ (
+ "log_filename_template",
+ "{{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log",
+ ),
+ ("log_processor_filename_template", "{{ filename }}.log"),
+ (
+ "dag_processor_manager_log_location",
+ "{}/logs/dag_processor_manager/dag_processor_manager.log".format(
+ AIRFLOW_HOME
+ ),
+ ),
+ ("task_log_reader", "task"),
+ ]
+
+ mismatches = []
+ for logging_config, default in logging_configs:
+ if not conf.has_option("logging", logging_config) and conf.has_option(
Review comment:
Yep `has_option` returns `false` when the section is not present
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703244284
Please rebase your PR, we added few changes that should fix the CI issues. Please do rebase and try to avoid merge commits, more information:
https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#id9
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703485663
@turbaszek This is an interesting problem that also occurs in other cases. I think that it is also worth solving separately. If you have a configuration file from Airflow 2 and you run it on Airflow 1.10, then you can't run: ʻairflow config`
----------------------------------------------------------------
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
[GitHub] [airflow] kaxil commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495856534
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return (
+ "The following configurations have been to moved from [core] to the new [logging] section. \n"
+ "- base_log_folder \n"
+ "- remote_logging \n"
+ "- remote_log_conn_id \n"
+ "- remote_base_log_folder \n"
+ "- encrypt_s3_logs \n"
+ "- logging_level \n"
+ "- fab_logging_level \n"
+ "- logging_config_class \n"
+ "- colored_console_log \n"
+ "- colored_log_format \n"
+ "- colored_formatter_class \n"
+ "- log_format \n"
+ "- simple_log_format \n"
+ "- task_log_prefix_template \n"
+ "- log_filename_template \n"
+ "- log_processor_filename_template \n"
+ "- dag_processor_manager_log_location \n"
+ "- task_log_reader \n"
Review comment:
Yea default values can be hardcoded.
And also agree that showing a precise message for the used config will be better
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek edited a comment on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek edited a comment on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703483096
@Sangarshanan it seems that one test is failing:
```
_______________________________________ TestLoggingConfigurationRule.test_valid_check _______________________________________
/usr/local/lib/python3.7/contextlib.py:73: in inner
with self._recreate_cm():
/usr/local/lib/python3.7/contextlib.py:112: in __enter__
return next(self.gen)
tests/test_utils/config.py:45: in conf_vars
conf.set(section, key, value)
/usr/local/lib/python3.7/site-packages/backports/configparser/__init__.py:1239: in set
super(ConfigParser, self).set(section, option, value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <airflow.configuration.AirflowConfigParser object at 0x7fa366479bd0>, section = 'logging', option = 'base_log_folder'
value = 'DUMMY'
def set(self, section, option, value=None):
"""Set an option."""
if value:
value = self._interpolation.before_set(self, section, option,
value)
if not section or section == self.default_section:
sectdict = self._defaults
else:
try:
sectdict = self._sections[section]
except KeyError:
> raise from_none(NoSectionError(section))
E backports.configparser.NoSectionError: No section: 'logging'
```
----------------------------------------------------------------
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
[GitHub] [airflow] github-actions[bot] commented on pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#issuecomment-703536099
The CI and PROD Docker Images for the build are prepared in a separate "Build Image" workflow,
that you will not see in the list of checks (you will see "Wait for images" jobs instead).
You can checks the status of those images in [The workflow run](https://github.com/apache/airflow/actions/runs/289172600)
----------------------------------------------------------------
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
[GitHub] [airflow] Sangarshanan commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
Sangarshanan commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495605068
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,31 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return "The logging configurations have been to moved from [core] to the new [logging] section."
Review comment:
Ah yes will add them
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495623800
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return (
+ "The following configurations have been to moved from [core] to the new [logging] section. \n"
+ "- base_log_folder \n"
+ "- remote_logging \n"
+ "- remote_log_conn_id \n"
+ "- remote_base_log_folder \n"
+ "- encrypt_s3_logs \n"
+ "- logging_level \n"
+ "- fab_logging_level \n"
+ "- logging_config_class \n"
+ "- colored_console_log \n"
+ "- colored_log_format \n"
+ "- colored_formatter_class \n"
+ "- log_format \n"
+ "- simple_log_format \n"
+ "- task_log_prefix_template \n"
+ "- log_filename_template \n"
+ "- log_processor_filename_template \n"
+ "- dag_processor_manager_log_location \n"
+ "- task_log_reader \n"
Review comment:
I would suggest checking if any of this key is used by users (not empty or different than default - this can be tricky) and then print a message for each incompatibility. See #11056 for reference. In this way, the message will be more precise. 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
[GitHub] [airflow] turbaszek commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495836540
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return (
+ "The following configurations have been to moved from [core] to the new [logging] section. \n"
+ "- base_log_folder \n"
+ "- remote_logging \n"
+ "- remote_log_conn_id \n"
+ "- remote_base_log_folder \n"
+ "- encrypt_s3_logs \n"
+ "- logging_level \n"
+ "- fab_logging_level \n"
+ "- logging_config_class \n"
+ "- colored_console_log \n"
+ "- colored_log_format \n"
+ "- colored_formatter_class \n"
+ "- log_format \n"
+ "- simple_log_format \n"
+ "- task_log_prefix_template \n"
+ "- log_filename_template \n"
+ "- log_processor_filename_template \n"
+ "- dag_processor_manager_log_location \n"
+ "- task_log_reader \n"
Review comment:
> what of the default/ empty keys is it right that they still remain in the old section ?
I think it will be ok to hardcode them in rule using default values from `airflow.cfg` of 1.10.x. WDYT @kaxil ?
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r497340967
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,83 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf, AIRFLOW_HOME
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ logging_configs = [
+ ("base_log_folder", "{}/logs".format(AIRFLOW_HOME)),
+ ("remote_logging", "False"),
+ ("remote_log_conn_id", ""),
+ ("remote_base_log_folder", ""),
+ ("encrypt_s3_logs", "False"),
+ ("logging_level", "INFO"),
+ ("fab_logging_level", "WARN"),
+ ("logging_config_class", ""),
+ ("colored_console_log", "True"),
+ (
+ "colored_log_format",
+ "[%(blue)s%(asctime)s%(reset)s] {%(blue)s%(filename)s:%(reset)s%(lineno)d} "
+ "%(log_color)s%(levelname)s%(reset)s - %(log_color)s%(message)s%(reset)s",
+ ),
+ (
+ "colored_formatter_class",
+ "airflow.utils.log.colored_log.CustomTTYColoredFormatter",
+ ),
+ (
+ "log_format",
+ "[%(asctime)s] {%(filename)s:%(lineno)d} %(levelname)s - %(message)s",
+ ),
+ ("simple_log_format", "%(asctime)s %(levelname)s - %(message)s"),
+ ("task_log_prefix_template", ""),
+ (
+ "log_filename_template",
+ "{{ ti.dag_id }}/{{ ti.task_id }}/{{ ts }}/{{ try_number }}.log",
+ ),
+ ("log_processor_filename_template", "{{ filename }}.log"),
+ (
+ "dag_processor_manager_log_location",
+ "{}/logs/dag_processor_manager/dag_processor_manager.log".format(
+ AIRFLOW_HOME
+ ),
+ ),
+ ("task_log_reader", "task"),
+ ]
+
+ mismatches = []
+ for logging_config, default in logging_configs:
+ if not conf.has_option("logging", logging_config) and conf.has_option(
Review comment:
Will this work if the `logging` section is not present? Have you tested 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
[GitHub] [airflow] Sangarshanan commented on a change in pull request #11132: Add loggingconfig rule
Posted by GitBox <gi...@apache.org>.
Sangarshanan commented on a change in pull request #11132:
URL: https://github.com/apache/airflow/pull/11132#discussion_r495710052
##########
File path: airflow/upgrade/rules/logging_configuration.py
##########
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import
+
+from airflow.configuration import conf
+from airflow.upgrade.rules.base_rule import BaseRule
+
+
+class LoggingConfigurationRule(BaseRule):
+ title = "Logging configuration has been moved to new section"
+
+ description = "The logging configurations have been moved from [core] to the new [logging] section."
+
+ def check(self):
+ if not conf.has_option("logging", "logging_level"):
+ return (
+ "The following configurations have been to moved from [core] to the new [logging] section. \n"
+ "- base_log_folder \n"
+ "- remote_logging \n"
+ "- remote_log_conn_id \n"
+ "- remote_base_log_folder \n"
+ "- encrypt_s3_logs \n"
+ "- logging_level \n"
+ "- fab_logging_level \n"
+ "- logging_config_class \n"
+ "- colored_console_log \n"
+ "- colored_log_format \n"
+ "- colored_formatter_class \n"
+ "- log_format \n"
+ "- simple_log_format \n"
+ "- task_log_prefix_template \n"
+ "- log_filename_template \n"
+ "- log_processor_filename_template \n"
+ "- dag_processor_manager_log_location \n"
+ "- task_log_reader \n"
Review comment:
Ah I was not thinking of this cause this change is backwards compatible and not something that can break if not moved but
you are right, showing only the incompatibility really helps while taking actions, I can do a comparison and return only the actively used keys but what of the default/ empty keys is it right that they still remain in the old section ?
----------------------------------------------------------------
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