-
Notifications
You must be signed in to change notification settings - Fork 6
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(awards): add slack notifications #31
Conversation
- icon_emoji and username fields seem to not actually work for the incoming webhook bot - i imagine we may have a `slack.bot` configuration, so isolating `webhook` to its own object
|
||
export interface RouterOptions { | ||
identity: IdentityApi; | ||
database: PluginDatabaseManager; | ||
logger: Logger; | ||
// todo: make required in next breaking change | ||
config?: Config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i started here by trying to not make a breaking change (and then in the following two fields it was going to be more complicated so i just broke it for now.)
curious what’s the best approach here. right now i imagine we’re the only ones using these packages but we should have a plan for versioning. ideally we would have started major version zero but the semantic release tool explicitly doesn’t allow this: https://semantic-release.gitbook.io/semantic-release/support/faq#can-i-set-the-initial-release-version-of-my-package-to-0.0.1.
some options:
- state that we’re treating version 1 as unstable, breaking changes may come in minor versions and we’ll note it in the release notes
- just push out a few breaking changes now before anyone’s using the package and hope we’re stable enough after that
- switch to using prereleases
curious about your thoughts here @colinodell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or, of course, just follow semver and be ok doing a major version release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh if we wrap this in with #32 i’m okay with it being a major release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we would have started major version zero but the semantic release tool explicitly doesn’t allow this
I was initially disappointed by this, but their reasoning does make sense. I'd strongly prefer that we avoid intentional BC breaks, since breaking BC tends to break trust. I think we should follow semver (and do a major version release), optionally with some pre-releases first if we need time to test and refine the changes before cutting that stable release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to following semver. It's more important for releases to be reliable (and changes expected) than for major releases to include a lot of changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok this all sounds good. once these branches are merged i'm going to set up a prelease flow and won't cut a proper release until we've run locally a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, might just try to cut the release after shipping these changes and save prereleasing for later. seems like it'll be a good amount of work to get right
plugins/awards-backend/README.md
Outdated
slack: | ||
webhook: | ||
# https://api.slack.com/messaging/webhooks | ||
url: <my_slack_webhook_url> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the webhook url contain a secret? i think in the documentation here we should advise that it be included as an env var and referenced as such in the config (from what I understand about dynamic includes in backstage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done here 8d2d2b2
plugins/awards-backend/src/awards.ts
Outdated
notifications: NotificationsGateway, | ||
catalogClient: CatalogClient, | ||
tokenManager: TokenManager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three new dependencies are only used by the notifyNewRecipients
function, but now the entire class is untestable unless these are all injected/mocked. Should we decouple this with one additional abstraction layer (perhaps called AwardsNotifier
or something) between the awards and the gateway so we can inject a single AwardNotifier
and keep that logic isolated from awards management? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep i think that makes sense. AwardsNotifier
could also have some smarter logic that i kept out from bloating the Awards
class like fanning out to multiple notification gateways and skipping catalog fetching if there are no notification gateways (rather than the nullnotificationgateway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in fb4f4f0.. how's that look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
like fanning out to multiple notification gateways and skipping catalog fetching if there are no notification gateways (rather than the nullnotificationgateway)
I often like to use this design pattern for those types of things (I don't know what it's called):
export class MultipleGateways implements NotificationsGateway {
constructor(private readonly gateways: NotificationsGateway[]) {}
async notifyNewRecipientsAdded(award: Award, newRecipients: UserEntity[]): Promise<void> {
this.gateways.forEach((gateway) => {
gateway.notifyNewRecipientsAdded(award, newRecipients);
});
}
}
But because you're also skipping the catalog fetching when no gateways are added I think it totally makes sense to implement AwardsNotifier
exactly how you did - nicely done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol yeah i love that. i think it's the composite pattern?
Award logos are now stored via s3 or local filesystem storage. BREAKING CHANGE: Running the awards-backend plugin now requires a `awards.storage.<s3|fs>` configuration to run. BREAKING CHANGE: The awards-backend router now requires the following three fields from the plugin environment `{ config: Config, discovery: DiscoveryService, tokenManager: TokenManager }`. (Missed note from #31)
🎉 This PR is included in version 1.0.2 🎉 The release is available on Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.0.0 🎉 The release is available on Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.0.0 🎉 The release is available on Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.0 🎉 The release is available on Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.2 🎉 The release is available on Your semantic-release bot 📦🚀 |
adds slack notifications when new users receive an award