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 #2636

Merged
merged 39 commits into from
Nov 8, 2024

Conversation

martinvibes
Copy link
Contributor

Description

Closes issue(s)

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

Sorry, something went wrong.

Copy link

netlify bot commented Nov 6, 2024

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

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

@martinvibes martinvibes changed the title Notify microsoft feat: Added Microsoft Teams Notifier Nov 6, 2024
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.00%. Comparing base (1ddb559) to head (fa14bd1).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
notifier/microsoftteamsnotifier/notifier.go 81.25% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2636      +/-   ##
==========================================
+ Coverage   84.96%   85.00%   +0.04%     
==========================================
  Files         102      103       +1     
  Lines        4721     4763      +42     
==========================================
+ Hits         4011     4049      +38     
- Misses        564      566       +2     
- Partials      146      148       +2     

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

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.

I still see some issue in this PR, could you fix it?

CONTRIBUTING.md Outdated
@@ -35,17 +36,17 @@ If you encounter any bugs or issues with the project, please [create a new issue

### Feature Requests

If you have a feature idea that you would like to see implemented, please [create a new issue](../../issues/new?assignees=&labels=enhancement%2Cneeds-triage&projects=&template=feature.yaml&title=(feature)+<title>) with the following information:
If you have a feature idea that you would like to see implemented, please [create a new issue](<../../issues/new?assignees=&labels=enhancement%2Cneeds-triage&projects=&template=feature.yaml&title=(feature)+<title>>) with the following information:
Copy link
Owner

Choose a reason for hiding this comment

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

🔨 issue: ‏Why have you changed this file? Your changes are not necessary here.

Comment on lines 8 to 9
SlackWebhookURL string `mapstructure:"slackWebhookUrl" koanf:"slackWebhookUrl"`
EndpointURL string `mapstructure:"endpointUrl" koanf:"endpointUrl"`
Copy link
Owner

Choose a reason for hiding this comment

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

🔨 issue: koanf annotation expect a lower case name here, why have you changed it for slack and url endpoint? It has nothing to do with this PR.

Comment on lines 23 to 25
if c.Kind == MicrosoftTeamsNotifier && c.WebhookURL == "" {
return fmt.Errorf("invalid notifier: no \"webhookURL\" property found for kind \"%s\"", c.Kind)
}
Copy link
Owner

Choose a reason for hiding this comment

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

👏 praise: ‏It is way better to use WebhookURL here 👏

martinvibes and others added 2 commits November 6, 2024 21:21
@martinvibes
Copy link
Contributor Author

hello @thomaspoignant kindly review :) @thomaspoignant

@thomaspoignant
Copy link
Owner

hello @thomaspoignant kindly review :) @thomaspoignant

I still see changes in the contributing.md any reason why?

@martinvibes
Copy link
Contributor Author

@thomaspoignant i didn't touch the file

@thomaspoignant
Copy link
Owner

@thomaspoignant i didn't touch the file

Changes seems to come from you merge in this commit 7058977

BTW why did you merge the branch from @abdegenius and not the one from this repo? This is probably the reason why you have the issue.

abdegenius and others added 4 commits November 6, 2024 21:42
@martinvibes
Copy link
Contributor Author

hey @thomaspoignant i have fix the issues please check

my vscode was auto formatting on save

@martinvibes
Copy link
Contributor Author

hello @thomaspoignant kindly review :)

@martinvibes
Copy link
Contributor Author

hello @thomaspoignant any update

@thomaspoignant
Copy link
Owner

hello @thomaspoignant any update

I will do some clean up and merge the PR.

@martinvibes
Copy link
Contributor Author

alright sir anticipating :)

Copy link

sonarqubecloud bot commented Nov 8, 2024

@kodiakhq kodiakhq bot merged commit 3525e70 into thomaspoignant:main Nov 8, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants