add alertId parameter in get chained alert API and paginate associated alerts if alertId param is set. #1070
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d alerts if alertId param is mentioned Signed-off-by: Surya Sashank Nistala <[email protected]>
Codecov Report
@@ Coverage Diff @@
## 2.9 #1070 +/- ##
============================================
- Coverage 72.75% 68.00% -4.76%
Complexity 119 119
============================================
Files 160 160
Lines 10447 10467 +20
Branches 1572 1576 +4
============================================
- Hits 7601 7118 -483
- Misses 1952 2647 +695
+ Partials 894 702 -192
|
lezzago
reviewed
Aug 2, 2023
| val getAssociatedAlerts: Boolean = request.param("getAssociatedAlerts", "false").toBoolean() | ||
| val workflowIds = mutableListOf<String>() | ||
| if (workflowId.isNullOrEmpty() == false) { | ||
| workflowIds.add(workflowId) | ||
| } | ||
| val alertIds = mutableListOf<String>() | ||
| if (alertId.isNullOrEmpty() == false) { |
There was a problem hiding this comment.
nitpick: use isNotEmpty() or isNotBlank()
There was a problem hiding this comment.
alertId is of type String?
isNotEmpty() and isNotBlank() can be used only if it is not nullable. so the isNullOrEmpty() check takes care of it. There isn't a negated implementation of isNullOrEmpty
lezzago
reviewed
Aug 2, 2023
| try { | ||
| val associatedAlertIds = mutableSetOf<String>() | ||
| alerts.forEach { associatedAlertIds.addAll(it.associatedAlertIds) } | ||
| if (associatedAlertIds.isEmpty()) return | ||
| val queryBuilder = QueryBuilders.boolQuery() | ||
| val searchRequest = SearchRequest(alertIndex) | ||
| // if chained alert id param is non-null, paginate the associated alerts. | ||
| if (getWorkflowAlertsRequest.alertIds.isNullOrEmpty() == false) { |
There was a problem hiding this comment.
nitpick: use isNotEmpty() or isNotBlank()
lezzago
reviewed
Aug 2, 2023
| if (!tableProp.missing.isNullOrBlank()) { | ||
| sortBuilder.missing(tableProp.missing) | ||
| } | ||
| searchRequest.source().sort(sortBuilder).size(tableProp.size) |
There was a problem hiding this comment.
We dont take the from value. How are we paginating associated alerts?
lezzago
requested changes
Aug 2, 2023
Signed-off-by: Surya Sashank Nistala <[email protected]>
|
Closing this PR Will backport #1071 to 2.9 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Get chained alert API will support filtering based on an alertId. The motivation is to fetch associated alerts with support for pagination. The size and sort params would apply to fetch associated alerts in this api when alert id is mentioned