jedcunningham commented on code in PR #47371:
URL: https://github.com/apache/airflow/pull/47371#discussion_r1994873513
##########
tests/cli/commands/remote_commands/test_config_command.py:
##########
@@ -447,3 +445,30 @@ def test_lint_detects_configs_with_env_vars(self, env_var,
config_change, expect
assert expected_message in normalized_output
assert config_change.suggestion in normalized_output
+
+ def test_lint_detects_default_value_change_upgrade(self):
Review Comment:
Also means this test should be refactored and moved into
[test_configuration.py](https://github.com/apache/airflow/blob/main/tests/core/test_configuration.py).
##########
airflow/cli/commands/remote_commands/config_command.py:
##########
@@ -546,6 +560,9 @@ def lint_config(args) -> None:
):
lint_issues.append(configuration.message)
+ if not args.skip_problematic_default_checks:
+
conf.handle_incompatible_airflow2_defaults(upgrade=args.upgrade_problematic_defaults)
+
Review Comment:
Which means, since this has already run and upgraded, you can get rid of
this completely - and the args, etc. All of it.
##########
airflow/configuration.py:
##########
@@ -403,6 +403,52 @@ def inversed_deprecated_sections(self):
upgraded_values: dict[tuple[str, str], str]
"""Mapping of (section,option) to the old value that was upgraded"""
+ legacy_incompatible_defaults: dict[tuple[str, str], tuple[Pattern, str,
str]] = {
+ ("logging", "log_filename_template"): (
+ re.compile(
+ r"^(?:" + re.escape("{{ ti.dag_id }}/{{ ti.task_id }}/{{ ts
}}/{{ try_number }}.log") + r"|"
+
r"dag_id=\{\s*ti\.dag_id\s*\}/run_id=\{\s*ti\.run_id\s*\}/task_id=\{\s*ti\.task_id\s*\}/"
+
r"\{%%\s*if\s+ti\.map_index\s*>=\s*0\s*%%\}map_index=\{\s*ti\.map_index\s*\}/\{%%\s*endif\s*%%\}"
+ r"attempt=\{\s*try_number\s*\}\.log" + r")$"
+ ),
+ "3.0",
+ "The default value for 'log_filename_template' from Airflow 2 may
break task logs in the new UI.",
+ ),
+ ("core", "dag_ignore_file_syntax"): (
+ re.compile(r"^regexp$"),
+ "3.0",
+ "The default value changed from 'regexp' in Airflow 2.x to 'glob'
in Airflow 3.0.",
+ ),
Review Comment:
```suggestion
```
Looking at this, automatically changing folks away from the default may
break things, not using an old default. e.g. if their airflowignore was regex,
but we automatically swap to glob, things won't be happy.
We should leave this one out I believe.
##########
airflow/cli/cli_config.py:
##########
@@ -791,6 +791,16 @@ def string_lower_type(val):
("--section",),
help="The section name",
)
+ARG_LINT_CONFIG_UPGRADE_DEFAULTS = Arg(
+ ("--upgrade-defaults",),
Review Comment:
So, now that you bring it up, not sure this flag is even worth having.
Upgrading it or not in the config lint command process doesn't seem that
helpful?
But wait! In `configuration.py` you are already upgrading to the new
default, meaning by the time `config lint` runs, well, it's already run and
updated stuff. So having this run in `config lint` at all isn't doing anything
really.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
