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

Enhance Notification : short/long notification type #127

Merged
merged 1 commit into from Jul 26, 2019

Conversation

@codenio
Copy link
Contributor

@codenio codenio commented Jul 19, 2019

ISSUE TYPE
  • Feature Pull Request
SUMMARY

This commit, (updated)

  • enables sending short/long event notifications to channels
  • adds an optional filed "notiftype" under channel config to change between short (default) and long notif types
  • adds Message() method to event object for creating brief messages (to use across handlers)
  • BugFix: enable/disable recommendations through the config file

Fixes #58

Preview

preview

@codenio codenio force-pushed the feature/notif-enhancement branch from cf65ed3 to e0fa37a Jul 19, 2019
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Jul 19, 2019

Hi @aananthraj,
Thanks for the PR. This looks awesome! Just a minor comment on the message format, can we also highlight cluster name in the notification?

@codenio
Copy link
Contributor Author

@codenio codenio commented Jul 19, 2019

yes, lets highlight that as well. Removing the artical A as well.
Do share if further improvements are required.

@PrasadG193, thanks for the prompt response 👍

pkg/events/events.go Outdated Show resolved Hide resolved
pkg/notify/mattermost.go Outdated Show resolved Hide resolved
@codenio codenio force-pushed the feature/notif-enhancement branch from e0fa37a to 2a42ea6 Jul 19, 2019
@codenio
Copy link
Contributor Author

@codenio codenio commented Jul 19, 2019

any more suggestions..?

@codenio codenio force-pushed the feature/notif-enhancement branch from 2a42ea6 to e239c7d Jul 19, 2019
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Jul 22, 2019

@aananthraj, we need to add configuration related documentation in config files - helm/botkube/values.yaml, deploy-all-in-one*.yaml and ./config.yaml. Otherwise, there is no way for the user to know about this feature.
Also, I think instead of brief, we should set notiftype: long/short. We should set notiftype: short as a default value across all the config files, and if the user wants, they can switch to notiftype: long.

Let me know what you think.

@codenio
Copy link
Contributor Author

@codenio codenio commented Jul 22, 2019

Otherwise, there is no way for the user to know about this feature.

Yes @PrasadG193 , was thinking about updating docs as well. Will do it.

we should set notiftype: long/short. We should set notiftype: short as a default value across all the config files,

Sounds good, will make the required changes and update here.

@codenio codenio force-pushed the feature/notif-enhancement branch from e239c7d to 3ee84b6 Jul 24, 2019
@codenio codenio changed the title Enhance Notification : Brief notification type Enhance Notification : short/long notification type Jul 24, 2019
@codenio codenio force-pushed the feature/notif-enhancement branch from 3ee84b6 to 637a082 Jul 24, 2019
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Jul 24, 2019

@aananthraj, Are we not adding Recommendations in the notifications when configured with short type? We should add it. Otherwise, it will defeat the purpose of having filters

@codenio
Copy link
Contributor Author

@codenio codenio commented Jul 24, 2019

@PrasadG193, doing so there would be no significance between both short/long notif types. Long type will not be used at all (as we have all infos in a short way) which questions the existance of Long NotifType. Is this ok..?

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Jul 24, 2019

@aananthraj with short notification type, we do optimize a lot of space by combining name, kind, namespace and clustername into a single line. If anyone still wants to optimize more, they can always set recommendations: false in the config file.

This commit,
- enables sending short/long event notifications to channels
- adds a optional filed "notiftype" under channel config to change between short (default) and long notif types
- adds Message() method to event object for creating brief messages (to use across handlers)
- BugFix: enable/disable recommendations through config file
@codenio codenio force-pushed the feature/notif-enhancement branch from 637a082 to 7508ba9 Jul 26, 2019
@codenio
Copy link
Contributor Author

@codenio codenio commented Jul 26, 2019

@PrasadG193 requested changes were implemented. I guess it's ready now. Let's get it merged.

@PrasadG193 PrasadG193 merged commit be5fade into infracloudio:develop Jul 26, 2019
1 check passed
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Jul 26, 2019

Great work, @aananthraj !! 🎉

@codenio
Copy link
Contributor Author

@codenio codenio commented Jul 26, 2019

Thanks @PrasadG193 for the extended support 👍

@codenio codenio deleted the feature/notif-enhancement branch Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants