-
Notifications
You must be signed in to change notification settings - Fork 27
refactor: update alerts approach to newest splunk approach #1668
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
Conversation
Code Coverage 🎉
|
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.
Looks good, just please add expand_ctrl_props(param) props as for textarea etc. there might be some additional props in TAs that will modify entities.
Can we extend config everything to have those modifications in final build like maxLength for text area
type="text" | ||
name="action.{{ mod_alert.short_name }}.param.{{ param.name }}" | ||
id="{{ mod_alert.short_name }}_{{ param.name }}" | ||
/> |
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.
shouldn't we add {{ expand_ctrl_props(param) }}
<splunk-radio-input | ||
name="action.{{ mod_alert.short_name }}.param.{{ param.name }}" | ||
id="{{ mod_alert.short_name }}_{{ param.name }}" | ||
value="{{ param.default_value }}" |
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.
shouldn't we add {{ expand_ctrl_props(param) }}
<textarea | ||
name="action.{{ mod_alert.short_name }}.param.{{ param.name }}" | ||
id="{{ mod_alert.short_name }}_{{ param.name }}" | ||
></textarea> |
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.
shouldn't we add {{ expand_ctrl_props(param) }}
it might be useful for other text area properties like max length
b"".join(actual_file_lines) | ||
if actual_file_lines and isinstance(actual_file_lines[0], bytes) | ||
else b"" | ||
) |
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.
nice catch, I assume here was issue, right?
As before there was comparision of the same variables
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.
yess, now it would be checking the bytes so that extra whitespace issue will resolved.
No further requirement. |
Issue number: ADDON-78947
PR Type
What kind of change does this PR introduce?
Summary
Changes
Introduce new Splunk approaches to create custom alerts actions.
User experience
Users can see the new Splunk based component for custom alerts actions.
Checklist
If an item doesn't apply to your changes, leave it unchecked.
Review
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear