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

feat: Added Microsoft Teams Notifier #2633

Closed
wants to merge 24 commits into from

Conversation

martinvibes
Copy link

Description

Closes issue(s)

Resolve #2556

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for go-feature-flag-doc-preview canceled.

Name Link
🔨 Latest commit 396033c
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/672bb669e9bd2a0008dc7795

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 92.96875% with 9 lines in your changes missing coverage. Please review.

Project coverage is 85.24%. Comparing base (a1572ee) to head (b399264).

Files with missing lines Patch % Lines
notifier/microsoftteamsnotifier/notifier.go 94.87% 4 Missing and 2 partials ⚠️
cmd/relayproxy/config/notifier.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2633      +/-   ##
==========================================
+ Coverage   85.04%   85.24%   +0.20%     
==========================================
  Files         102      103       +1     
  Lines        4720     4847     +127     
==========================================
+ Hits         4014     4132     +118     
- Misses        561      567       +6     
- Partials      145      148       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martinvibes
Copy link
Author

hello @thomaspoignant kindly review :)

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

@martinvibes we are super close to be able to merge this PR.
I've made a few suggestions, when done we will be able to merge the PR.

Headers map[string][]string `mapstructure:"headers" koanf:"headers"`
WebhookURL string `mapstructure:"webhookUrl" koanf:"webhookurl"`
SlackWebhookURL string `mapstructure:"slackWebhookUrl" koanf:"slackWebhookUrl"`
MicrosoftTeamsWebhookURL string `mapstructure:"microsoftteamsWebhookUrl" koanf:"microsoftteamsWebhookUrl"`
Copy link
Owner

Choose a reason for hiding this comment

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

🎯 suggestion: ‏Instead of using a new field here I would suggest to re-use WebhookURL that is generic enough to be used in association of the microsoftteams kind field.

Short bool `json:"short"`
}

type ByTitle []Field
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
type ByTitle []Field
type ByTitle []Field

🎯 suggestion:ByTitle does not have to be public I would suggest to put this struct as private.

Copy link

sonarcloud bot commented Nov 6, 2024

@martinvibes
Copy link
Author

hello @thomaspoignant kindly review :)

@martinvibes martinvibes closed this Nov 6, 2024
@martinvibes martinvibes deleted the microsoft_notify branch November 6, 2024 19:57
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.

(feature) Microsoft Teams Notifier
3 participants