AMBARI-21008 Integrate AlertTargetService and subresources with swagger #619
Conversation
Change-Id: Icdbc0221f7d66fc27c051b0cacf04534f3875ae4
|
Refer to this link for build results (access rights to CI server needed): |
| * @param ui uri info | ||
| * @return alert target resource representation | ||
| */ | ||
| @GET @ApiIgnore // documented below |
There was a problem hiding this comment.
I think /alert_targets and /alert_targets/{targetId} should be documented separately, since they have a few differences.
| @Path("{targetId}") | ||
| @Produces("text/plain") | ||
| @ApiOperation(value = "Returns a single alert target", response = AlertTargetSwagger.class, responseContainer = "List") |
There was a problem hiding this comment.
List container is appropriate only for the "all targets" endpoint.
| * Request / response schema for AlertTarget API Swagger documentation generation. The interface only serves documentation | ||
| * generation purposes, it is not meant to be implemented. | ||
| */ | ||
| public interface AlertTargetSwagger extends ApiModel { |
There was a problem hiding this comment.
To more accurately reflect the JSON document, the outer AlertTarget map should be modelled, too.
"AlertTarget" : {
"name" : "asdf",
"notification_type" : "test",
...
}
Example:
| @ApiModelProperty(name = AlertGroupResourceProvider.ID_PROPERTY_ID) | ||
| Long getId(); | ||
|
|
||
| @ApiModelProperty(name = "cluster_id") |
There was a problem hiding this comment.
You can use ClusterResourceProvider.CLUSTER_ID.
| @ApiResponse(code = HttpStatus.SC_FORBIDDEN, message = MSG_PERMISSION_DENIED), | ||
| @ApiResponse(code = HttpStatus.SC_INTERNAL_SERVER_ERROR, message = MSG_SERVER_ERROR), | ||
| @ApiResponse(code = HttpStatus.SC_BAD_REQUEST, message = MSG_INVALID_ARGUMENTS), | ||
| }) | ||
| public Response getTargets(@Context HttpHeaders headers, |
There was a problem hiding this comment.
Ideally this method should be getTarget, since it returns a single item. If you want to keep the existing name to avoid mixing documentation and refactoring, you can provide a nickname for Swagger.
There was a problem hiding this comment.
renamed to getTarget
Change-Id: I255a77bae1c5239ded10793e2c997bb4ea0783e2
|
applied the requested changes, thanks for the suggestions |
|
Refer to this link for build results (access rights to CI server needed): |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): |
|
@benyoka Can you please review? |
|
|
||
| /** | ||
| * The {@link AlertTargetService} handles CRUD operation requests for alert | ||
| * targets. | ||
| */ | ||
| @Path("/alert_targets/") | ||
| @Api(value = "/alerts", description = "Endpoint for alert specific operations") |
There was a problem hiding this comment.
I think @Api value should be "Alerts"
There was a problem hiding this comment.
thanks @benyoka. fixed by 0ad81d7
swagger API resource values are not consistently capitalised, probably that confused me. thanks for checking.
ambari-server/docs/api/generated/swagger.json:
{
"name" : "User Authentication Sources",
"description" : "Endpoint for user specific authentication source operations"
}, {
"name" : "Users",
"description" : "Endpoint for User specific operations"
}, {
"name" : "Views"
}, {
"name" : "clusters",
"description" : "Endpoint for cluster-specific operations"
}, {
"name" : "hosts",
"description" : "Endpoint for host-specific operations"
},
Change-Id: Ie4ffc4d30eec202f179a147c4a7f93cdca28df7e
| @@ -54,7 +54,7 @@ | |||
| * targets. | |||
| */ | |||
| @Path("/alert_targets/") | |||
| @Api(value = "/alerts", description = "Endpoint for alert specific operations") | |||
| @Api(value = "/Alerts", description = "Endpoint for alert specific operations") | |||
| public class AlertTargetService extends BaseService { | |||
There was a problem hiding this comment.
I think the leading slash is also not needed, so it should be just "Alerts" (other @Api declarations don't use slashes).
Change-Id: I800427c0fd04ea6c10ad96bbd15b52faa5c4e4c1
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
Change-Id: Icdbc0221f7d66fc27c051b0cacf04534f3875ae4
What changes were proposed in this pull request?
Add swagger documentation to AlertTarget API endpoints
How was this patch tested?
Generate swagger docs by
then manually checking Swagger output at
ambari-server/docs/api/generated/index.htmlcheckstyle using:
API endpoint calls at
api/v1/alert_targetshave also been tested manuallyplease review @Jetly-Jaimin @adoroszlai @benyoka