-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[RAM] Add error for action interval shorter than check interval #148919
Conversation
@elasticmachine merge upstream |
Pinging @elastic/response-ops (Team:ResponseOps) |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
action.frequency?.notifyWhen === RuleNotifyWhen.THROTTLE && | ||
parseDuration(action.frequency.throttle!) < scheduleInterval | ||
); | ||
if (actionsWithInvalidThrottles.length) { |
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.
Question: Should we fail all the actions if one failed this validation? or should we allow the persistence of the other actions? what do you think?
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.
validateActions
is used to validate API requests. It would be more confusing for the user if their update
or create
request only applied 2 out of 3 of the actions they wanted it to, as opposed to just throwing an error and asking them to send the whole request again.
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.
ok make sense but can we validate all the actions at once so we can return a fully detailed msg with the ruleId and ruleName, so they do not have to call it each time to figure out what's wrong in one step.
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 would be a larger change that modifies part of validateActions
out of scope for #148569. Can you create a new issue with a full spec for what you'd like error messages to look like and I can tackle that in another PR?
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.
Ok I will!
…kibana into 148569-throttle-warning
@XavierM We don't currently have API integration tests for any other inner workings of Rules Client methods. These changes are covered in the additions to |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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.
LGTM, #149415 for a follow up PR
Summary
Closes #148569
Adds form error and API error for action intervals shorter than rule check intervals
Checklist
Delete any items that are not applicable to this PR.