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

Refactor slack and teams notifications #1069

Merged
merged 3 commits into from Jun 17, 2022

Conversation

kingzacko1
Copy link
Contributor

@kingzacko1 kingzacko1 commented Jun 15, 2022

refactor of the Slack and Teams notification types code to more closely align with use of AutoValue across the project. There should be no functional difference in the two notifications.

Notes for Reviewers

  • The commit history must be preserved - please use the rebase-merge or standard merge option instead of squash-merge
  • Sync up with the author before merging

@kingzacko1 kingzacko1 marked this pull request as ready for review June 16, 2022 18:12
Copy link
Contributor

@ryan-carroll-graylog ryan-carroll-graylog left a comment

Choose a reason for hiding this comment

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

Everything looks good to me code wise (minus one possible log message update in comment). The original functionality is working in testing for both notification types.

This is not in the original scope of this PR, but I did notice an existing bug where you can use a teams webhook for a slack notification and vice versa. If you think it's worth fixing in this PR I believe modifying the nested if statements in the validate() methods of following files should fix the issue:

if (webhookUri.getHost().toLowerCase().contains("slack")) {
if (!SLACK_PATTERN.matcher(webhookUrl()).find()) {
validation.addError(FIELD_WEBHOOK_URL, INVALID_SLACK_URL_ERROR_MESSAGE);
}
} else if (webhookUri.getHost().toLowerCase().contains("discord")) {
if (!DISCORD_PATTERN.matcher(webhookUrl()).find()) {

if (webhookUri.getHost().toLowerCase().contains("office")) {
if (!TEAMS_PATTERN.matcher(webhookUrl()).find()) {

@kingzacko1
Copy link
Contributor Author

Fixed the log message and updated URL validation logic in latest commit @ryan-carroll-graylog

@kingzacko1 kingzacko1 force-pushed the refactor-slack-and-teams-notifications branch from 502e5b9 to fccc9c0 Compare June 17, 2022 16:49
@roberto-graylog roberto-graylog self-assigned this Jun 17, 2022
Copy link
Contributor

@roberto-graylog roberto-graylog left a comment

Choose a reason for hiding this comment

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

Looks good.

@kingzacko1 kingzacko1 merged commit c058de3 into master Jun 17, 2022
@kingzacko1 kingzacko1 deleted the refactor-slack-and-teams-notifications branch June 17, 2022 21:19
@danotorrey
Copy link
Contributor

Thank you @kingzacko1!

kingzacko1 added a commit that referenced this pull request Oct 19, 2022
 Refactor MS Teams and Slack message objects to use AutoValue
kingzacko1 added a commit that referenced this pull request Oct 25, 2022
…1205)

* Handle fields containing colon chars correctly

* Refactor slack and teams notifications (#1069)

 Refactor MS Teams and Slack message objects to use AutoValue

* Fix log message payload
dennisoelkers pushed a commit to Graylog2/graylog2-server that referenced this pull request Aug 29, 2023
…rations#1069)

 Refactor MS Teams and Slack message objects to use AutoValue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants