Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[alerting] Adds Action Type configuration support and whitelisting #44483
[alerting] Adds Action Type configuration support and whitelisting #44483
Changes from 18 commits
6c48bc1
4113bd2
c03104c
e46f616
a12f7b9
ea4580f
f169c0a
4855bb0
cd87387
057a6ec
fad46ab
7d8ed08
a93f2f0
7375745
d5baa69
ac21af9
b277445
2699dd3
f73fb00
481da21
9a84b8b
8a9ddc9
72f70b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think alternatives was here when the property was
string | string[]
? So, I think we can remove thealternatives()
andtry()
wrappers, just make it aJoi.array()
? Or maybe there's some joi sneakiness I don't yet understand :-)It clearly works now, so fine with shipping this way, create an issue / project card if it should be fixed later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's not needed anymore, I'll fix in a small PR next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the name and semantics of this to be strange. I expect
is...
methods to return booleans.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, I could rename it to
ensureIsWhitelisted...
or something like that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this starts to read as a valuable piece of function, but there's no verbs, or something. Would something like
isHostnameWildcard()
work instead? Or is this some kind of functional pattern I'm not familiar with?I also noticed you used
const <functionName> = <lambda>
as is the style for a lot of code these days, but I think we've generally just used functions in this area of Kibana at least; ie,function doesValueWhitelistAnyHostname(...) {...}
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding
doesValueWhitelistAnyHostname
it's not about FP, it's about communicating intent.Implementation is easier to communicate (they read the code) but intent is much harder (read the code, and the surrounding code, and how they fit together). I find it helpful to communicate intent in the name of functions rather than implementation.
Hope that helps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why doesn't this just throw instead of returning an error and then throwing based on instanceof? Seems overcomplicated to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't typecheck thrown errors in Typescript, so the general recommendation there seems to be to return errors.
I agree it's over complicated- that's why the Result type is a much more elegant solution, but there were concerns about Result type being used on external APIs.