DaanHoogland opened a new issue, #13898:
URL: https://github.com/apache/cloudstack/issues/13898
Found while writing ConfigKey-wiring unit tests for PR #13884 (issue #10752,
phase out enum `Config`).
`ManagementServerImpl.configure()`
(server/src/main/java/com/cloud/server/ManagementServerImpl.java:1141-1148)
still reads two settings through the raw configuration map instead of their
migrated `ConfigKey`s:
```java
_configs = _configDao.getConfiguration();
final String value = _configs.get("event.purge.interval");
final int cleanup = NumbersUtil.parseInt(value, 60 * 60 * 24); // 1 day.
_purgeDelay = NumbersUtil.parseInt(_configs.get("event.purge.delay"), 0);
```
`ManagementServer.EventPurgeInterval` and `ManagementServer.EventPurgeDelay`
both already exist on the interface — three lines below,
`AlertPurgeInterval`/`AlertPurgeDelay` (the exact same purge-scheduling
pattern) were correctly migrated to `.value()` in the same method, so these two
are a clear miss rather than a deliberate choice.
This is a different variant from the pattern in #13893 (redundant/diverging
hardcoded defaults passed to `parseInt`/`parseLong` around an already-migrated
`.value()` call): here the read site was never migrated to `.value()` (or even
to `_configDao.getValue(key.key())`) at all — it still goes through a raw
`Map<String,String> _configs = _configDao.getConfiguration()` lookup by literal
key string, bypassing the `ConfigKey` entirely. The earlier
`.value()`-replacement sweep only searched for the
`_configDao.getValue(key.key())` call shape, so it structurally couldn't catch
this one.
It also happens to carry the same kind of default-mismatch risk as #13893:
`EventPurgeDelay`'s registered default is `"15"`, but the hardcoded fallback
here is `0`, and `0` is the sentinel that disables purging (`if (_purgeDelay !=
0) { schedule... }`). So migrating this site isn't a pure no-op — it needs the
same default-reconciliation judgment call as the #13893 cases before switching
it over.
Only these two sites were found in `ManagementServerImpl`; this was spotted
incidentally while scoping unit tests for the `.value()` migration, not through
an exhaustive sweep — worth a repo-wide grep for other
`_configDao.getConfiguration()`-backed raw map reads
(`_configs.get("literal.key")`) that have a matching `ConfigKey` sitting unused
nearby.
--
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]
