ashb commented on code in PR #57970:
URL: https://github.com/apache/airflow/pull/57970#discussion_r2504481708
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -239,6 +241,22 @@ def __init__(
self._suppress_future_warnings = False
self._providers_configuration_loaded = False
+ @property
+ def _lookup_sequence(self) -> list[Callable]:
+ """
+ Define the sequence of lookup methods for get().
+
+ Subclasses can override this to sustomise lookup order.
Review Comment:
```suggestion
Subclasses can override this to customise lookup order.
```
##########
airflow-core/tests/unit/core/test_configuration.py:
##########
@@ -1088,6 +1088,39 @@ def test_order_of_secrets_backends_and_kwargs_on_workers(
for key, value in expected_backend_kwargs.items():
assert getattr(secrets_backend, key) == value
+ def test_lookup_sequence_order(self):
+ """Test that lookup sequence follows correct priority order."""
+ test_conf = AirflowConfigParser()
+ sequence = test_conf._lookup_sequence
+
+ assert sequence == [
+ test_conf._get_environment_variables,
+ test_conf._get_option_from_config_file,
+ test_conf._get_option_from_commands,
+ test_conf._get_option_from_secrets,
+ test_conf._get_option_from_defaults,
+ test_conf._get_option_from_provider_fallbacks,
+ ]
+
+ def test_lookup_sequence_override(self):
+ """Test that subclasses can override lookup sequence."""
+
+ class MyAirflowConfigPrser(AirflowConfigParser):
+ @property
+ def _lookup_sequence(self) -> list[callable]:
+ return [
+ self._get_option_from_secrets,
+ self._get_option_from_config_file,
+ ]
+
+ test_conf = MyAirflowConfigPrser()
+ myorder = test_conf._lookup_sequence
+ assert len(myorder) == 2
+ assert myorder == [
+ test_conf._get_option_from_secrets,
+ test_conf._get_option_from_config_file,
+ ]
+
Review Comment:
Honestly: I would remove these tests. It is just writing the same thing
twice, and I can't think of a case where these tests would help.
You _could_ have one like the override where you check that an env var is
not respected. But even that feels optional to me.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -972,131 +990,159 @@ def get_mandatory_list_value(self, section: str, key:
str, **kwargs) -> list[str
raise ValueError(f"The value {section}/{key} should be set!")
return value
- @overload # type: ignore[override]
- def get(self, section: str, key: str, fallback: str = ..., **kwargs) ->
str: ...
+ def _get_option_from_defaults(
+ self,
+ deprecated_key: str | None,
+ deprecated_section: str | None,
+ key: str,
+ section: str,
+ issue_warning: bool = True,
+ extra_stacklevel: int = 0,
+ **kwargs,
+ ) -> str | object:
+ """Get config option from default values."""
+ if self.get_default_value(section, key) is not None or "fallback" in
kwargs:
+ return expand_env_var(self.get_default_value(section, key,
**kwargs))
+ return VALUE_NOT_FOUND_SENTINEL
- @overload
- def get(self, section: str, key: str, **kwargs) -> str | None: ...
+ def _get_option_from_provider_fallbacks(
+ self,
+ deprecated_key: str | None,
+ deprecated_section: str | None,
+ key: str,
+ section: str,
+ issue_warning: bool = True,
+ extra_stacklevel: int = 0,
+ **kwargs,
+ ) -> str | object:
+ """Get config option from provider fallback defaults."""
+ if self.get_provider_config_fallback_defaults(section, key) is not
None:
+ # no expansion needed
+ return self.get_provider_config_fallback_defaults(section, key,
**kwargs)
+ return VALUE_NOT_FOUND_SENTINEL
- def get( # type: ignore[misc]
+ def _resolve_deprecated_lookup(
self,
section: str,
key: str,
- suppress_warnings: bool = False,
- lookup_from_deprecated: bool = True,
- _extra_stacklevel: int = 0,
- team_name: str | None = None,
- **kwargs,
- ) -> str | None:
+ lookup_from_deprecated: bool,
+ extra_stacklevel: int = 0,
+ ) -> tuple[str, str, str | None, str | None, bool]:
+ """
+ Resolve deprecated section/key mappings and determine deprecated
values.
+
+ :param section: Section name (will be lowercased)
+ :param key: Key name (will be lowercased)
+ :param lookup_from_deprecated: Whether to lookup from deprecated
options
+ :param extra_stacklevel: Extra stack level for warnings
+ :return: Tuple of (resolved_section, resolved_key, deprecated_section,
deprecated_key, warning_emitted)
+ """
section = section.lower()
key = key.lower()
warning_emitted = False
deprecated_section: str | None = None
deprecated_key: str | None = None
- if lookup_from_deprecated:
- option_description = (
- self.configuration_description.get(section, {}).get("options",
{}).get(key, {})
+ if not lookup_from_deprecated:
+ return section, key, deprecated_section, deprecated_key,
warning_emitted
+
+ option_description = (
+ self.configuration_description.get(section, {}).get("options",
{}).get(key, {})
+ if self.configuration_description
+ else {}
+ )
+ if option_description.get("deprecated"):
+ deprecation_reason = option_description.get("deprecation_reason",
"")
+ warnings.warn(
+ f"The '{key}' option in section {section} is deprecated.
{deprecation_reason}",
+ DeprecationWarning,
+ stacklevel=2 + extra_stacklevel,
)
- if option_description.get("deprecated"):
- deprecation_reason =
option_description.get("deprecation_reason", "")
+ # For the cases in which we rename whole sections
+ if section in self.inversed_deprecated_sections:
+ deprecated_section, deprecated_key = (section, key)
+ section = self.inversed_deprecated_sections[section]
+ if not self._suppress_future_warnings:
warnings.warn(
- f"The '{key}' option in section {section} is deprecated.
{deprecation_reason}",
- DeprecationWarning,
- stacklevel=2 + _extra_stacklevel,
+ f"The config section [{deprecated_section}] has been
renamed to "
+ f"[{section}]. Please update your `conf.get*` call to use
the new name",
+ FutureWarning,
+ stacklevel=2 + extra_stacklevel,
)
- # For the cases in which we rename whole sections
- if section in self.inversed_deprecated_sections:
- deprecated_section, deprecated_key = (section, key)
- section = self.inversed_deprecated_sections[section]
- if not self._suppress_future_warnings:
- warnings.warn(
- f"The config section [{deprecated_section}] has been
renamed to "
- f"[{section}]. Please update your `conf.get*` call to
use the new name",
- FutureWarning,
- stacklevel=2 + _extra_stacklevel,
- )
- # Don't warn about individual rename if the whole section is
renamed
- warning_emitted = True
- elif (section, key) in self.inversed_deprecated_options:
- # Handle using deprecated section/key instead of the new
section/key
- new_section, new_key =
self.inversed_deprecated_options[(section, key)]
- if not self._suppress_future_warnings and not warning_emitted:
- warnings.warn(
- f"section/key [{section}/{key}] has been deprecated,
you should use"
- f"[{new_section}/{new_key}] instead. Please update
your `conf.get*` call to use the "
- "new name",
- FutureWarning,
- stacklevel=2 + _extra_stacklevel,
- )
- warning_emitted = True
- deprecated_section, deprecated_key = section, key
- section, key = (new_section, new_key)
- elif section in self.deprecated_sections:
- # When accessing the new section name, make sure we check
under the old config name
- deprecated_key = key
- deprecated_section = self.deprecated_sections[section][0]
- else:
- deprecated_section, deprecated_key, _ =
self.deprecated_options.get(
- (section, key), (None, None, None)
+ # Don't warn about individual rename if the whole section is
renamed
+ warning_emitted = True
+ elif (section, key) in self.inversed_deprecated_options:
+ # Handle using deprecated section/key instead of the new
section/key
+ new_section, new_key = self.inversed_deprecated_options[(section,
key)]
+ if not self._suppress_future_warnings and not warning_emitted:
+ warnings.warn(
+ f"section/key [{section}/{key}] has been deprecated, you
should use"
+ f"[{new_section}/{new_key}] instead. Please update your
`conf.get*` call to use the "
+ "new name",
+ FutureWarning,
+ stacklevel=2 + extra_stacklevel,
)
- # first check environment variables
- option = self._get_environment_variables(
- deprecated_key,
- deprecated_section,
- key,
- section,
- issue_warning=not warning_emitted,
- extra_stacklevel=_extra_stacklevel,
- team_name=team_name,
- )
- if option is not None:
- return option
+ warning_emitted = True
+ deprecated_section, deprecated_key = section, key
+ section, key = (new_section, new_key)
+ elif section in self.deprecated_sections:
+ # When accessing the new section name, make sure we check under
the old config name
+ deprecated_key = key
+ deprecated_section = self.deprecated_sections[section][0]
+ else:
+ deprecated_section, deprecated_key, _ =
self.deprecated_options.get(
+ (section, key), (None, None, None)
+ )
- # ...then the config file
- option = self._get_option_from_config_file(
- deprecated_key,
- deprecated_section,
- key,
- kwargs,
- section,
- issue_warning=not warning_emitted,
- extra_stacklevel=_extra_stacklevel,
- )
- if option is not None:
- return option
+ return section, key, deprecated_section, deprecated_key,
warning_emitted
- # ...then commands
- option = self._get_option_from_commands(
- deprecated_key,
- deprecated_section,
- key,
- section,
- issue_warning=not warning_emitted,
- extra_stacklevel=_extra_stacklevel,
- )
- if option is not None:
- return option
+ @overload # type: ignore[override]
+ def get(self, section: str, key: str, fallback: str = ..., **kwargs) ->
str: ...
+
+ @overload # type: ignore[override]
+ def get(self, section: str, key: str, **kwargs) -> str | None: ...
Review Comment:
Nit: this is wrong, -- if fallback isn't specified, this won't return None,
it will raise an AirflowConfigurationException I think.
(Lets fix that in a second PR)
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -1132,7 +1179,8 @@ def _get_option_from_commands(
section: str,
issue_warning: bool = True,
extra_stacklevel: int = 0,
- ) -> str | None:
+ **kwargs,
+ ) -> str | object:
Review Comment:
I don't love this being typed as `| object` -- perhaps we should do
```python
class ValueNotFound: pass
VALUE_NOT_FOUND_SENTINEL = ValueNotFound()
```
Then this could be
```suggestion
) -> str | ValueNotFound:
```
Not a big thing, but might be worth 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
