Fail fast on invalid entitlement patches #128071
Conversation
prdoyle
added
auto-backport
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| } catch (PolicyParserException e) { | ||
| // This is more specific and informative than a generalized IllegalStateException | ||
| throw e; | ||
| } catch (IOException | RuntimeException e) { |
There was a problem hiding this comment.
Is the RuntimeException necessary to catch? Could we just convert IOException to UncheckedIOException and leave the rest to propagate on their own? There is something to be said for wrapping any exception, though (not just these) since it includes the layer name? Does the PolicyParserException include that?
There was a problem hiding this comment.
It seems user-friendly to call out, loud and clear, that the patch they're applying is the cause of the problem, and I felt that PolicyParserException was already clear on that point. PolicyParserException may not name the layer, but it does describe the exact problem in detail.
I caught RuntimeException only because the set IOException | RuntimeException covers everything (besides Error) that this method could possibly throw. Happy to tweak it.
There was a problem hiding this comment.
Why not be very clear and convert the old log message (that happened for all Exception) to a runtime exception? The cause will still be included, so the PolicyParserException would still be visible.
There was a problem hiding this comment.
That's what I did. The parts of the old log message I omitted are no longer true.
There was a problem hiding this comment.
Are you saying you don't want me to peel off PolicyParseException? Sure, that's an easy change.
We've decided we'd rather crash at startup if an invalid patch is specified than ignore the patch and continue.