dimas-b commented on code in PR #2472:
URL: https://github.com/apache/polaris/pull/2472#discussion_r2316263598
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java:
##########
@@ -74,7 +74,14 @@ public void warnOnFailedChecks(
@Observes Startup event,
Instance<ProductionReadinessCheck> checks,
ReadinessConfiguration config) {
- List<Error> errors = checks.stream().flatMap(check ->
check.getErrors().stream()).toList();
+ List<Error> errors =
+ checks.stream()
+ .flatMap(check -> check.getErrors().stream())
+ .filter(
+ error ->
+ config.ignoreOffendingProperties().stream()
+ .noneMatch(prop ->
prop.equalsIgnoreCase(error.offendingProperty())))
Review Comment:
This approach LGTM in general. However, WDYT about adding a new "ID"
property to `Error`, e.g. `error.getId()`.
The ID could be a static constant for cases where there could only ever be
one `Error` instance per "check" code (e.g. `checkUserPrincipalMetricTag`) or
it could be a deterministic (hash) function of the "type" plus some parameters
(e.g. `checkInsecureStorageSettings` could produce IDs like `storage-17af38`
and `storage-46fq98`).
The idea is that admin users should suppress specific error instances, but
not "ranges" of errors. This way, if an admin user suppresses one particular
check cases, new checks will still be visible when Polaris adds them. The
value of `error.offendingProperty()` may still be too broad in some cases.
The "hash" part being deterministic will allow admin users to propagate the
same configuration to all their deployment environments. At the same time, it
is not easy to guess, which will force the admin user to review what exactly
needs to be suppressed. Also, if the meaning of the error changes, we can
change the ID, and it will force the admin users to reassess the implications
(and re-suppress).
WDYT?
--
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]
