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.

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

Adds a slack action #39221

Merged
merged 4 commits into from
Jun 25, 2019
Merged

Adds a slack action #39221

merged 4 commits into from
Jun 25, 2019

Conversation

pmuellr
Copy link
Member

@pmuellr pmuellr commented Jun 18, 2019

Summary

Adds a Slack action to the nascent Actions sub-framework that's part of the nascent Alerting framework.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@pmuellr pmuellr added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:Stack Services labels Jun 19, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services

@pmuellr pmuellr force-pushed the alerting/slack-action-2 branch from 7be1efc to ecdc4b4 Compare June 19, 2019 16:04
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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, just one nitpick.

@pmuellr pmuellr force-pushed the alerting/slack-action-2 branch from ecdc4b4 to 4797237 Compare June 21, 2019 22:50
@pmuellr
Copy link
Member Author

pmuellr commented Jun 21, 2019

Just forced push this branch to get the code up to the new x-pack/legacy/plugin level.

Let's see how CI goes; once green, will un-draft ...

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@pmuellr pmuellr changed the base branch from feature/alerting to master June 24, 2019 13:21
@pmuellr pmuellr added the review label Jun 24, 2019
@pmuellr pmuellr marked this pull request as ready for review June 24, 2019 13:23
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few questions / comments.

*/

// a function which returns a promise with a resolve() and reject() method
export function rPromise() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unable to find where this function is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes, it appears to be unused now, will delete

let actionTypeRegistry: ActionTypeRegistry;
let actionType: ActionType;

const mockEncryptedSavedObjectsPlugin = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I created a new mock that we can now re-use x-pack/legacy/plugins/encrypted_saved_objects/server/plugin.mock.ts.

@elasticmachine
Copy link
Contributor

💔 Build Failed

package.json Outdated
@@ -119,6 +119,7 @@
"@kbn/pm": "1.0.0",
"@kbn/test-subj-selector": "0.2.1",
"@kbn/ui-framework": "1.0.0",
"@slack/webhook": "^5.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be in x-package/package.json, not the root package.json

@elasticmachine
Copy link
Contributor

💔 Build Failed

pmuellr added 2 commits June 25, 2019 12:26
- move @slack/webhook from package.json to x-pack/package.json
- remove unused r_promise module
- refactor the way slack mock works
- rename id of slack action from `kibana.slack` to `.slack`
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

This is great! LGTM 👍

@pmuellr pmuellr merged commit 9ac2139 into elastic:master Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants