bito-code-review[bot] commented on code in PR #44514:
URL: https://github.com/apache/superset/pull/44514#discussion_r4124343529
##########
superset/config.py:
##########
@@ -3545,6 +3639,28 @@ def _config_fingerprint(source: bytes | None) -> str:
logger.exception("Found but failed to import local superset_config")
raise
+if _canonical_history_retention_override:
+ VERSION_HISTORY_RETENTION_DAYS = _normalize_version_history_retention_days(
+ _canonical_history_retention_value, legacy=False
+ )
+if (
+ _legacy_history_retention_override
+ and "VERSION_HISTORY_RETENTION_DAYS" not in os.environ
+):
+ _legacy_history_retention_days: int =
_normalize_version_history_retention_days(
+ _legacy_history_retention_value, legacy=True
+ )
+ if not _canonical_history_retention_override:
+ VERSION_HISTORY_RETENTION_DAYS = _legacy_history_retention_days
+ elif VERSION_HISTORY_RETENTION_DAYS == _version_history_retention_seed:
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is valid and addresses a potential
issue where the legacy deprecation warning could be skipped. By normalizing and
warning for the legacy key before the seed comparison, the code ensures that
the deprecation warning is correctly triggered even when a canonical override
is present. Applying this change improves the robustness of the configuration
loading logic.
**superset/config.py**
```
_legacy_history_retention_days: int =
_normalize_version_history_retention_days(
_legacy_history_retention_value, legacy=True
)
if not _canonical_history_retention_override:
VERSION_HISTORY_RETENTION_DAYS = _legacy_history_retention_days
elif VERSION_HISTORY_RETENTION_DAYS == _version_history_retention_seed:
```
##########
superset/config.py:
##########
@@ -1794,31 +1826,59 @@ class ExportStorageConfig(TypedDict, total=False):
def _parse_version_history_retention_days() -> int:
"""Parse the retention window without making invalid input fatal."""
- value: str | None =
os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ value: str | None = os.environ.get("VERSION_HISTORY_RETENTION_DAYS")
+ legacy: bool = False
+ if value is None:
+ value = os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ legacy = value is not None
if value is None:
return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ return _normalize_version_history_retention_days(value, legacy=legacy)
+
+
+def _normalize_version_history_retention_days(value: object, *, legacy: bool)
-> int:
+ """Normalize released legacy values without shortening retention on
upgrade."""
+ name: str = (
+ "SUPERSET_VERSION_HISTORY_RETENTION_DAYS"
+ if legacy
+ else "VERSION_HISTORY_RETENTION_DAYS"
+ )
try:
- retention_days = int(value)
+ if isinstance(value, bool) or not isinstance(value, (str, int)):
+ raise ValueError("Retention must be integer days")
+ retention_days: int = int(value)
except ValueError:
+ logger.warning("Invalid %s=%r; skipping pruning", name, value)
+ return 0
+ if legacy:
logger.warning(
- "Invalid SUPERSET_VERSION_HISTORY_RETENTION_DAYS=%r; using %d",
- value,
- _DEFAULT_VERSION_HISTORY_RETENTION_DAYS,
+ "%s is deprecated; use VERSION_HISTORY_RETENTION_DAYS. "
+ "Legacy nonpositive values disable pruning.",
+ name,
)
- return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ if retention_days <= 0:
+ return 0
+ if retention_days < -1:
+ logger.warning("Invalid negative %s; skipping pruning", name)
+ return 0
if retention_days > _MAX_VERSION_HISTORY_RETENTION_DAYS:
logger.warning(
- "SUPERSET_VERSION_HISTORY_RETENTION_DAYS=%r exceeds the maximum "
- "of %d; using %d",
+ "%s=%r exceeds the maximum of %d; skipping pruning",
+ name,
value,
_MAX_VERSION_HISTORY_RETENTION_DAYS,
- _DEFAULT_VERSION_HISTORY_RETENTION_DAYS,
)
- return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ return 0
+ if retention_days == -1:
+ logger.warning(
+ "VERSION_HISTORY_RETENTION_DAYS=-1 makes history eligible for "
+ "immediate pruning on the next scheduled run; use 0 to disable"
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion to use the `name` variable in the warning message is correct
and has been addressed. The updated code now dynamically uses `name` (which
correctly reflects whether the legacy or canonical setting is being processed)
in all warning logs, ensuring that users receive accurate feedback regardless
of which environment variable triggered the warning.
**superset/config.py**
```
try:
if isinstance(value, bool) or not isinstance(value, (str, int)):
raise ValueError("Retention must be integer days")
retention_days: int = int(value)
except ValueError:
logger.warning("Invalid %s=%r; skipping pruning", name, value)
return 0
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
