Skip to content
This repository was archived by the owner on Aug 2, 2022. It is now read-only.

Add action to create /_alerting/destinations/ #235

Merged
merged 34 commits into from
Aug 16, 2020

Conversation

skkosuri-amzn
Copy link
Contributor

Issue #, if available:
#232

Description of changes:
#232
Failures responses are also standardized.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

skkosuri-amzn and others added 30 commits June 24, 2020 10:18
* Update to OpenDistro version 1.7.0
* Adds support for ES 7.7.0

* Update the Release Notes

* Update the requirement of JDK 14 in README

* Update the Java version in 'push-notification-jar' workflow
* change default value of the setting 'alert_history_max_age' to 30 days

* change default value of the setting 'alert_history_retention_period' to 60 days
 - Use Codecov to visualize the code coverage result
 - Add Coverage Report Upload action in test-workflow
 - Add several badges in README
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #235 into master will increase coverage by 0.65%.
The diff coverage is 83.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #235      +/-   ##
============================================
+ Coverage     76.30%   76.96%   +0.65%     
+ Complexity      162      157       -5     
============================================
  Files            71       75       +4     
  Lines          2672     2809     +137     
  Branches        422      430       +8     
============================================
+ Hits           2039     2162     +123     
- Misses          415      428      +13     
- Partials        218      219       +1     
Impacted Files Coverage Δ Complexity Δ
...rting/transport/TransportIndexDestinationAction.kt 55.69% <55.69%> (ø) 0.00 <0.00> (?)
...icsearch/alerting/action/IndexDestinationAction.kt 66.66% <66.66%> (ø) 0.00 <0.00> (?)
...ndistroforelasticsearch/alerting/AlertingPlugin.kt 89.23% <100.00%> (+0.16%) 0.00 <0.00> (ø)
...sticsearch/alerting/action/DeleteMonitorRequest.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)
...csearch/alerting/action/IndexDestinationRequest.kt 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...search/alerting/action/IndexDestinationResponse.kt 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...relasticsearch/alerting/model/destination/Chime.kt 64.00% <100.00%> (+6.85%) 0.00 <0.00> (ø)
...search/alerting/model/destination/CustomWebhook.kt 87.14% <100.00%> (+5.51%) 0.00 <0.00> (ø)
...icsearch/alerting/model/destination/Destination.kt 69.82% <100.00%> (+19.82%) 0.00 <0.00> (ø)
...relasticsearch/alerting/model/destination/Slack.kt 64.00% <100.00%> (+6.85%) 0.00 <0.00> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42da6b5...2015ddc. Read the comment docs.

Comment on lines 28 to 33
var destinationId: String
var seqNo: Long
var primaryTerm: Long
var refreshPolicy: WriteRequest.RefreshPolicy
var method: RestRequest.Method
var destination: Destination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be making all the attributes to the request immutable by using val?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made val

Comment on lines 33 to 38
var id: String
var version: Long
var seqNo: Long
var primaryTerm: Long
var status: RestStatus
var destination: Destination
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be immutable as well since we would not want to modify them after the response is generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made val

Comment on lines 67 to 81
} else {
if (!IndexUtils.scheduledJobIndexUpdated) {
IndexUtils.updateIndexMapping(ScheduledJob.SCHEDULED_JOBS_INDEX, ScheduledJob.SCHEDULED_JOB_TYPE,
ScheduledJobIndices.scheduledJobMappings(), clusterService.state(), client.admin().indices(),
object : ActionListener<AcknowledgedResponse> {
override fun onResponse(response: AcknowledgedResponse) {
onUpdateMappingsResponse(response)
}
override fun onFailure(t: Exception) {
actionListener.onFailure(t)
}
})
} else {
prepareDestinationIndexing()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the if block to else if, so we wont have any nested if else statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, changed.

@skkosuri-amzn skkosuri-amzn merged commit 263f55d into master Aug 16, 2020
@bowenlan-amzn bowenlan-amzn added the enhancement New feature or request label Sep 8, 2020
@qreshi qreshi deleted the adding-post-monitor branch December 19, 2020 00:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants