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/telegram): add telegram notification #19135

Merged
merged 5 commits into from
Aug 12, 2020

Conversation

sranka
Copy link
Contributor

@sranka sranka commented Jul 29, 2020

This PR takes back the telegram notification (#18218) that was reverted by #19088 + adds a new UI feature flag notification-endpoint-telegram that turns on/off telegram notifications (in 04b188a), off by default

  • 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 sranka requested review from ebb-tide, 121watts and desa and removed request for ebb-tide July 29, 2020 13:50
@sranka sranka marked this pull request as ready for review July 29, 2020 13:50
@sranka sranka requested a review from a team as a code owner July 29, 2020 13:50
@desa desa requested a review from ebb-tide August 4, 2020 20:48
@sranka sranka force-pushed the 17899/telegramNotification2 branch 2 times, most recently from baaadc2 to a2891ab Compare August 6, 2020 04:19
@glinton
Copy link
Contributor

glinton commented Aug 6, 2020

@sranka what is the status of this? have you looked into why the initial revert was made in order to avoid another? My assumption is that you will need to split this work into separate prs for the frontend and backend work, but i'm not certain

@sranka sranka force-pushed the 17899/telegramNotification2 branch from a2891ab to d6f3c33 Compare August 7, 2020 04:20
@sranka
Copy link
Contributor Author

sranka commented Aug 7, 2020

@glinton it was reverted because of deployment problems, a new feature flag that turns off telegram UI is the solution, the name of the flag was suggested by @desa on July, 27th :

"I’d name the feature-flag something like notification-endpoint-telegram
[9:19 PM] next steps would be to re-open the telegram pr with the feature-flag"

Copy link
Contributor

@ebb-tide ebb-tide left a comment

Choose a reason for hiding this comment

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

I made a suggestion in comments, but after that is changed this should be good to merge.

@sranka sranka force-pushed the 17899/telegramNotification2 branch from d6f3c33 to 3bb7cc5 Compare August 12, 2020 09:36
@sranka sranka merged commit a431ca5 into master Aug 12, 2020
@sranka sranka deleted the 17899/telegramNotification2 branch August 12, 2020 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants