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

Support filterBy in update/delete destination/monitor APIs #311

Merged
merged 5 commits into from
Dec 2, 2020

Conversation

lezzago
Copy link
Contributor

@lezzago lezzago commented Dec 1, 2020

Issue #, if available:
#310
Description of changes:
Support filterBy in update/delete destination/monitor APIs

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

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #311 (c205b5f) into master (421960c) will decrease coverage by 0.79%.
The diff coverage is 42.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #311      +/-   ##
============================================
- Coverage     80.06%   79.27%   -0.80%     
- Complexity      199      202       +3     
============================================
  Files           151      151              
  Lines          5198     5322     +124     
  Branches        682      701      +19     
============================================
+ Hits           4162     4219      +57     
- Misses          674      728      +54     
- Partials        362      375      +13     
Impacted Files Coverage Δ Complexity Δ
...troforelasticsearch/alerting/util/AlertingUtils.kt 41.66% <0.00%> (-25.00%) 0.00 <0.00> (ø)
...ting/transport/TransportDeleteDestinationAction.kt 32.69% <23.80%> (-37.31%) 0.00 <0.00> (ø)
...alerting/transport/TransportDeleteMonitorAction.kt 40.00% <28.57%> (-40.00%) 0.00 <0.00> (ø)
.../alerting/transport/TransportIndexMonitorAction.kt 59.34% <44.44%> (-0.66%) 0.00 <0.00> (ø)
...ch/alerting/transport/TransportGetMonitorAction.kt 80.00% <71.42%> (-6.96%) 0.00 <0.00> (ø)
...rting/transport/TransportIndexDestinationAction.kt 61.19% <80.76%> (+4.67%) 0.00 <0.00> (ø)
...relasticsearch/alerting/core/model/ScheduledJob.kt 80.00% <0.00%> (-6.67%) 0.00% <0.00%> (ø%)
...distroforelasticsearch/alerting/core/JobSweeper.kt 71.65% <0.00%> (-0.54%) 35.00% <0.00%> (ø%)
...e/action/node/ScheduledJobsStatsTransportAction.kt 75.00% <0.00%> (ø) 8.00% <0.00%> (ø%)
...asticsearch/alerting/core/schedule/JobScheduler.kt 75.71% <0.00%> (+4.28%) 15.00% <0.00%> (ø%)
... and 3 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 421960c...c205b5f. Read the comment docs.

@lezzago lezzago requested a review from bowenlan-amzn December 2, 2020 17:28
skkosuri-amzn
skkosuri-amzn previously approved these changes Dec 2, 2020

val updatedDestination = updateDestination(destination = destinationV2)
assertEquals("Incorrect destination name", updatedDestination.name, "testUpdate")
assertEquals("Incorrect destination type", updatedDestination.type, DestinationType.SLACK)
Copy link
Contributor

@bowenlan-amzn bowenlan-amzn Dec 2, 2020

Choose a reason for hiding this comment

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

I am wondering the use case for this filterBy feature: another user B with different backend role compared to user A who create the destination, when fillterBy enabled, user B will not be able to update the the destination, even user B has the permission for update destination API

reason to ask is I am expecting to see some test like that here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we will be adding those. Sriram modified the testing framework in this PR, so it will be easy to add a different user to test this use case. I have tested this manually for now.

bowenlan-amzn
bowenlan-amzn previously approved these changes Dec 2, 2020
@lezzago lezzago dismissed stale reviews from bowenlan-amzn and skkosuri-amzn via c205b5f December 2, 2020 17:50
@lezzago lezzago merged commit 7694fd0 into opendistro-for-elasticsearch:master Dec 2, 2020
tlfeng pushed a commit that referenced this pull request Feb 6, 2021
* Support filterBy in update/delete destination/monitor APIs

* cleanup code

* fix message

* add comments to checkUserFilterByPermissions function
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants