-
Notifications
You must be signed in to change notification settings - Fork 36
Renaming all Actions to distinguish between internal and external facing #284
Conversation
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
============================================
+ Coverage 70.03% 70.07% +0.04%
- Complexity 1815 1820 +5
============================================
Files 190 190
Lines 8867 8900 +33
Branches 756 757 +1
============================================
+ Hits 6210 6237 +27
- Misses 2301 2306 +5
- Partials 356 357 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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. Thanks for the change.
@@ -23,7 +23,7 @@ | |||
public class ADResultBulkAction extends ActionType<BulkResponse> { | |||
|
|||
public static final ADResultBulkAction INSTANCE = new ADResultBulkAction(); | |||
//Internal Action which is not used for public facing RestAPIs. | |||
// Internal Action which is not used for public facing RestAPIs. | |||
public static final String NAME = "cluster:admin/opendistro/adinternal/write/bulk"; |
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.
Could we put the internal and external action common prefix to CommonValue so that it is harder to have typo errors and easier to maintain in case that we want to change in the future?
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.
Sure, thats a nice idea.
Will make a change.
@@ -19,7 +19,7 @@ | |||
|
|||
public class AnomalyResultAction extends ActionType<AnomalyResultResponse> { | |||
public static final AnomalyResultAction INSTANCE = new AnomalyResultAction(); | |||
//External Action which used for public facing RestAPIs. | |||
// External Action which used for public facing RestAPIs. |
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.
should this be internal since only background job runs it?
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.
It is also used in one of our Rest APIs which I wanted it to be external action :
Line 127 in 129d249
client.execute(AnomalyResultAction.INSTANCE, getRequest, new RestToXContentListener<>(channel)); |
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.
I see.
import org.elasticsearch.action.ActionType; | ||
|
||
public class DeleteModelAction extends ActionType<DeleteModelResponse> { | ||
public static final DeleteModelAction INSTANCE = new DeleteModelAction(); | ||
// Internal Action which is not used for public facing RestAPIs. | ||
public static final String NAME = "cluster:admin/opendistro/adinternal/model/delete"; | ||
public static final String NAME = CommonValue.EXTERNAL_ACTION_PREFIX + "model/delete"; |
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 is internal.
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.
Yeah I did take care of it : )
This is breaking tests. |
Reordered INSTANCE and NAME statements to fix them, thanks @kaituo. |
*Issue #195 *
Description of changes:
Renaming all actions to distinguish between Internal facing vs external facing.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.