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(notification/teams): add teams notification #18916

Closed
wants to merge 6 commits into from

Conversation

sranka
Copy link
Contributor

@sranka sranka commented Jul 10, 2020

Closes #17939

This PR adds support for Microsoft Teams in InfluxDB notification endpoints and notification rules. It depends on influxdata/flux#2866 that introduces a new teams package to send notifications.

What is in this PR

  • implementation and tests of teams endpoint and rule
  • swagger updated with teams notification rule and endpoint
  • updated UI to use the new swagger + new UI for teams endpoint and rule:

What is not in this PR

  • teams endpoint and rule in =pkger=
  • new UI tests
  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

@sranka
Copy link
Contributor Author

sranka commented Jul 10, 2020

The PR could be merged after #18218, small conflicts are expected.

@timhallinflux timhallinflux requested review from desa and removed request for desa July 17, 2020 15:42
@sranka sranka force-pushed the 17939/teamsNotification branch 2 times, most recently from 581fea8 to 3c1f2c0 Compare July 25, 2020 11:09
@sranka sranka marked this pull request as ready for review July 25, 2020 17:05
@sranka sranka requested a review from a team as a code owner July 25, 2020 17:05
@sranka sranka requested review from glinton and removed request for a team July 25, 2020 17:05
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

can you split this into two prs (front/back-end) or feature flag the front-end code as it tends to be deployed first

@sranka
Copy link
Contributor Author

sranka commented Jul 28, 2020

@glinton ... sure, can you please first

  • take back or help with the (f67c3a1) telegram notifications into master. I could possibly do it, but I would need some instructions or code pointers to know of how to name or add a feature flag part, I have a lot of conflicts herein that are now hard to resolve. Teams and telegram notifications change the same files and this PR was done on top of existing telegram changes. Teams must be finished first.
  • approve fix(endpoints): do not reset organization's secrets when an endpoint is created or updated #19082 , it fixes a major defect in the management of endpoints

@glinton
Copy link
Contributor

glinton commented Jul 28, 2020

take back or help with the (f67c3a1) telegram notifications into master

I'm not certain why that was reverted, so i don't want to un-revert it. My guess is that you'll need to split up that work into 2 prs also, one for the backend code, and one for the front for the same reasons as mentioned above. Reach out to those involved in the revert if you'd like to learn more.

approve #19082

I commented on the pr, but can you add tests to show the behavior described and that the new code fixes that. otherwise it looks like a sane change

@sranka
Copy link
Contributor Author

sranka commented Jul 29, 2020

@glinton ... I have to wait herein for the telegram to be re-introduced back into master, the other way would be painful. I changed this PR to draft because of it.

@sranka sranka marked this pull request as draft July 29, 2020 05:10
@sranka
Copy link
Contributor Author

sranka commented Aug 12, 2020

@glinton ... this type of notification can be now enabled with a feature flag "notification-endpoint-teams", the implementation follows already approved #19135

@sranka sranka force-pushed the 17939/teamsNotification branch from 0bc51d6 to 82ca912 Compare August 12, 2020 12:00
@sranka sranka marked this pull request as ready for review August 12, 2020 12:23
@sranka sranka requested a review from glinton August 12, 2020 12:23
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

go code looks good. I'll let someone with more front-end experience approve the pr after the front-end code is reviewed

notification/endpoint/endpoint_test.go Show resolved Hide resolved
notification/rule/teams.go Show resolved Hide resolved
notification/endpoint/teams.go Outdated Show resolved Hide resolved
notification/endpoint/teams.go Outdated Show resolved Hide resolved
notification/rule/teams.go Outdated Show resolved Hide resolved
notification/rule/teams.go Outdated Show resolved Hide resolved
notification/rule/teams_test.go Show resolved Hide resolved
@desa desa requested a review from 121watts August 17, 2020 21:43
@sranka sranka force-pushed the 17939/teamsNotification branch from 567728f to a66387b Compare August 24, 2020 08:24
@sranka
Copy link
Contributor Author

sranka commented Oct 14, 2020

This PR cannot be approved and merged anymore, it contains both UI and server part that are in now different repositories. Two PRs are required now.

@sranka sranka closed this Oct 14, 2020
@jacobmarble jacobmarble deleted the 17939/teamsNotification branch January 2, 2024 22:03
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.

InfluxDB v2 - MS-Teams - Notification Endpoint
2 participants