sunank200 commented on code in PR #58188:
URL: https://github.com/apache/airflow/pull/58188#discussion_r2513483702
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -758,10 +758,29 @@ def restore_core_default_configuration(self) -> None:
self._default_values =
create_default_config_parser(self.configuration_description)
self._providers_configuration_loaded = False
+ @property
+ def _validators(self) -> list[Callable[[], None]]:
Review Comment:
I would prefer a method that returns an immutable sequence. Hidden, mutable
lists as a property are brittle. Return a tuple from a method.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -758,10 +758,29 @@ def restore_core_default_configuration(self) -> None:
self._default_values =
create_default_config_parser(self.configuration_description)
self._providers_configuration_loaded = False
+ @property
+ def _validators(self) -> list[Callable[[], None]]:
+ """
+ Return list of validators defined on a config parser class.
+
+ Subclasses can override this to customize the validators that are run
during validation on the
+ config parser instance.
+ """
+ return [
+ self._validate_sqlite3_version,
+ self._validate_enums,
+ self._validate_deprecated_values,
+ self._upgrade_postgres_metastore_conn,
+ ]
Review Comment:
I think it would be good to separate validation from migration work. For
example: _upgrade_postgres_metastore_conn is not a validation.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -758,10 +758,29 @@ def restore_core_default_configuration(self) -> None:
self._default_values =
create_default_config_parser(self.configuration_description)
self._providers_configuration_loaded = False
+ @property
+ def _validators(self) -> list[Callable[[], None]]:
+ """
+ Return list of validators defined on a config parser class.
+
+ Subclasses can override this to customize the validators that are run
during validation on the
+ config parser instance.
+ """
+ return [
+ self._validate_sqlite3_version,
+ self._validate_enums,
+ self._validate_deprecated_values,
+ self._upgrade_postgres_metastore_conn,
+ ]
+
def validate(self):
Review Comment:
```suggestion
def validate(self) -> None:
```
##########
airflow-core/tests/unit/core/test_configuration.py:
##########
@@ -1105,6 +1105,33 @@ 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_default_validators(self):
Review Comment:
It'd be helpful to add a few more tests:
- Verify the exact order and names of the default validators, not just their
count. This helps prevent accidental reordering.
- Consider injecting a dummy validator that raises an exception, then check
that the exception bubbles up and that `is_validated` remains `False`.
- Run `validate` twice and ensure no additional changes occur. If there are
migrations involved, demonstrate that re-running is safe.
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -758,10 +758,29 @@ def restore_core_default_configuration(self) -> None:
self._default_values =
create_default_config_parser(self.configuration_description)
self._providers_configuration_loaded = False
+ @property
+ def _validators(self) -> list[Callable[[], None]]:
+ """
+ Return list of validators defined on a config parser class.
+
+ Subclasses can override this to customize the validators that are run
during validation on the
+ config parser instance.
+ """
+ return [
+ self._validate_sqlite3_version,
+ self._validate_enums,
+ self._validate_deprecated_values,
+ self._upgrade_postgres_metastore_conn,
+ ]
+
def validate(self):
- self._validate_sqlite3_version()
- self._validate_enums()
+ """Run all registered validators."""
+ for validator in self._validators:
+ validator()
+ self.is_validated = True
+ def _validate_deprecated_values(self):
Review Comment:
```suggestion
def _validate_deprecated_values(self) -> None:
```
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -758,10 +758,29 @@ def restore_core_default_configuration(self) -> None:
self._default_values =
create_default_config_parser(self.configuration_description)
self._providers_configuration_loaded = False
+ @property
+ def _validators(self) -> list[Callable[[], None]]:
+ """
+ Return list of validators defined on a config parser class.
+
+ Subclasses can override this to customize the validators that are run
during validation on the
+ config parser instance.
+ """
+ return [
+ self._validate_sqlite3_version,
+ self._validate_enums,
+ self._validate_deprecated_values,
+ self._upgrade_postgres_metastore_conn,
+ ]
Review Comment:
Maybe run validators first, then migrations.
--
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]
