enlivenhq / teamcity-slack

Configurable TeamCity notifier plugin for Slack.
137 stars 51 forks source link

Use slack-webhook library #35

Closed eljobe closed 4 months ago

eljobe commented 7 years ago

I'm not 100% happy with this change. It definitely simplifies the creation of slack messages from the data that is generated by the TeamCity build status events. However, I notice that the SlackWrapper class is basically a useless abstraction in the current way the code is organized.

For each event the SlackNotificator receives and each user in the set of users who need to be notified for this event, we instantiate a new SlackWrapper and then call send on that wrapper instance. But, we never need to reuse any of the state on the SlackWrapper object, so, we might as well just be passing the data straight through instead of storing it on an object.

I'd like to get this much of the code integrated into the project as it provides some benefit (no longer need the SlackPayload class.) But, then I'm going to continue restructuring the SlackNotificator such that it's job is basically to:

  1. Create a TeamCityEvent object which describes what happened from TeamCity's point of view (e.g. The build failed.)
  2. For each user who needs to be notified, create a SlackTarget object which describes the data we currently keep on SlackWrapper and then passes that Set<SlackTarget> and TeamCityEvent to a new version of SlackWrapper.send() which will just be responsible for creating a SlackMessage and sending it to the SlackApi for each SlackTarget and TeamCityEvent This way, we can construct the SlackWrapper only once and cache instances of SlackApi based on the slackUrl from which they are constructed.

If you'd prefer, I can go ahead and work on those changes as part of this pull request, but this code is working, so I'd prefer to get it checked in, and then add these other pieces incrementally.

eljobe commented 4 months ago

I'm guessing this is not going to be merged. Closing.