Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create REST API to fire actions #39463

Merged
merged 10 commits into from
Jun 26, 2019
Merged

Create REST API to fire actions #39463

merged 10 commits into from
Jun 26, 2019

Conversation

mikecote
Copy link
Contributor

In this PR, we're exposing the fire action function as a REST API.

@mikecote mikecote self-assigned this Jun 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services

@mikecote mikecote mentioned this pull request Jun 21, 2019
Closed
3 tasks
@mikecote mikecote force-pushed the alerting/fire-api branch from 7cec0fe to ad6ad69 Compare June 21, 2019 19:37
@elasticmachine

This comment has been minimized.

Sign in to view
@mikecote mikecote marked this pull request as ready for review June 21, 2019 20:07
@mikecote mikecote requested review from a team June 21, 2019 20:07
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@mikecote

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
pmuellr
pmuellr approved these changes Jun 24, 2019
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM but left some comments

x-pack/legacy/plugins/actions/server/routes/fire.ts
export function fireRoute({ server, actionTypeRegistry, getServices }: FireRouteOptions) {
server.route({
method: 'POST',
path: '/api/action/{id}/_fire',
Copy link
Member

Choose a reason for hiding this comment

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

what does the underscore in _fire mean? Not quite public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pattern established by Elasticsearch as well as a way to indicate it doesn't correspond to a restful resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used to denote an action on a resource, though ES seems to be trying to move away from this convention. It also disambiguates in some cases, as index names in ES can't start with an underscore.

x-pack/legacy/plugins/actions/server/init.ts
@@ -64,6 +68,11 @@ export function init(server: Legacy.Server) {
findRoute(server);
updateRoute(server);
listActionTypesRoute(server);
fireRoute({
Copy link
Member

Choose a reason for hiding this comment

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

seems slightly weird that this route takes different params than the others. The others don't need actionTypeRegistry nor getServices? Or was this needed for mocking, perhaps for the "immediate fire" API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah these are functions required by the route (not only for testing). It needs the registry to lookup the action type once loaded from saved objects. And also the getServices to parameterized the execution. This was the only way I was thinking to pass in the singleton instances without exposing anything outside the plugin.

@elasticmachine

This comment has been minimized.

Sign in to view
@mikecote

This comment has been minimized.

Sign in to view
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

bmcconaghy
bmcconaghy approved these changes Jun 25, 2019
View reviewed changes
Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

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

LGTM. I had a comment re: changing the name from fire to execute, but if we want to do that it could be done in a separate PR, as it would be a lot of changes.

x-pack/legacy/plugins/actions/server/routes/fire.test.ts Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/routes/fire.ts
export function fireRoute({ server, actionTypeRegistry, getServices }: FireRouteOptions) {
server.route({
method: 'POST',
path: '/api/action/{id}/_fire',
Copy link
Contributor

Choose a reason for hiding this comment

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

It's used to denote an action on a resource, though ES seems to be trying to move away from this convention. It also disambiguates in some cases, as index names in ES can't start with an underscore.

@azasypkin azasypkin self-requested a review June 26, 2019 11:37
azasypkin
azasypkin approved these changes Jun 26, 2019
View reviewed changes
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, couple of nits.

One general suggestion: the action plugin is a plugin that is or will be a foundational to many other things, is relatively new and will become very complex relatively soon. So it would be awesome if you can use debug logging more frequently, it would be a great help when we debug issues that will happen in production sooner or later (e.g. it was/is super valuable for security plugin) ;)

x-pack/legacy/plugins/actions/server/action_type_registry.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/init.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/lib/get_create_task_runner_function.test.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/lib/execute.test.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/routes/_mock_server.ts
@@ -41,8 +43,21 @@ export function createMockServer(config: Record<string, any> = defaultConfig) {
},
});

server.register({
Copy link
Member

Choose a reason for hiding this comment

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

note: you may have a hard time rewriting tests that rely on this mock and Hapi's expose/register when you decide to migrate to new platform. No need to do anything about that in this PR, just saying 🙈 (experienced that recently, and started to abstract away from Hapi where possible)

x-pack/legacy/plugins/actions/server/routes/fire.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/actions/server/routes/fire.ts Outdated Show resolved Hide resolved
x-pack/test/api_integration/apis/actions/fire.ts Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mikecote mikecote merged commit e522e75 into elastic:master Jun 26, 2019
@mikecote mikecote mentioned this pull request Jun 26, 2019
Merged
mikecote added a commit to mikecote/kibana that referenced this pull request Jun 26, 2019
@mikecote
Create REST API to fire actions (elastic#39463)
1860ef0 
* Create REST API to fire actions

* Add more tests to the fire API

* Remove dead file reference

* Fix broken tests

* Fix broken test

* Update docs

* Apply PR feedback
mikecote added a commit that referenced this pull request Jun 26, 2019
@mikecote
Create REST API to fire actions (#39463) (#39685)
33324a6 
* Create REST API to fire actions

* Add more tests to the fire API

* Remove dead file reference

* Fix broken tests

* Fix broken test

* Update docs

* Apply PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azasypkin azasypkin azasypkin approved these changes

@bmcconaghy bmcconaghy bmcconaghy approved these changes

@pmuellr pmuellr pmuellr approved these changes

Assignees

@mikecote mikecote

Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mikecote @elasticmachine @pmuellr @azasypkin @bmcconaghy