phallstrom / slackistrano

Slack integration for Capistrano deployments.
MIT License
373 stars 74 forks source link

Add an easy way to change web hook name and icon? #68

Closed mszyndel closed 7 years ago

mszyndel commented 7 years ago

Before I spend time writing code for that I wanted to ask what is the reason to make altering the web hook name and icon so hard in the current version?

I don't see any practical reason to pollute my codebase with extra classes to achieve something that could be done with two lines of config file.

If I prepare a pull request which adds following, will it be accepted?

set :slackistrano, {
  channel: '#deployments',
  webhook: 'https://hooks.slack.com/services/asdf/asdf/asdf',
  username: 'deployer',
  icon_url: 'https://example.com/image.gif'
}
phallstrom commented 7 years ago

@hajder Mostly because the number of options were starting to spiral out of control and folks wanted to do things that weren't possible just via options so moving to custom classes seemed like a better solution.

I don't think I want to go back to this even though I totally get your point. However...

If you submitted a PR that added a custom messaging class that took these two arguments that would be fine. So the config would look something like:

set :slackistrano, { channel: '#deployments', webhook: 'https://hooks.slack.com/services/asdf/asdf/asdf', klass: Slackistrano::SomeReasonablySaneNameForTheClass, username: 'deployer', icon_url: 'https://example.com/image.gif' }

That would keep the custom class out of your application and for those not interested in customizing anything but those two elements (imagine there are a good number of these) would simplify their life too.

Thoughts?

grappler commented 7 years ago

This is what I ended coming up with. I hope this makes sense.

module Capistrano
  module ExtendedTools
    class SlackistranoMessagingBot < Slackistrano::Messaging::Base

      def initialize(env: nil, team: nil, channel: nil, token: nil, webhook: nil, icon_url: nil)
        super(env: env, team: team, channel: channel, token: token, webhook: webhook)
        @icon_url = icon_url
      end

      def icon_url
        if @icon_url.nil?
          super
        else
          @icon_url
        end
      end

    end
  end
end

Let me know if this could be improved.

phallstrom commented 7 years ago

How would you invoke this?

grappler commented 7 years ago

Like this. Why?

set :slackistrano, {
    channel: '#deployments',
    webhook: 'https://hooks.slack.com/services/asdf/asdf/asdf',
    klass: Capistrano::ExtendedTools::SlackistranoMessagingBot,
    icon_url: 'https://example.com/image.gif'
}

I hope I understood your question correctly.

grappler commented 7 years ago

FYI - I improved the code above by using super

chrisvanpatten commented 7 years ago

@phallstrom any reason this was closed? It's really killing me that these options aren't available by default.

phallstrom commented 7 years ago

@chrisvanpatten See my comment above on Apr 5. No reason you can't do what OP has done though.

grappler commented 7 years ago

If you don't want to write your own code. The team I work with have create our own gem that you can install https://rubygems.org/gems/capistrano-wearerequired

phallstrom commented 7 years ago

@hajder @chrisvanpatten Hackoberfest made me revisit some things... realized I can do this while denying future options... so... 3.8.2 was just pushed to rubygems.

See: https://github.com/phallstrom/slackistrano#optional-configuration--overrides

chrisvanpatten commented 7 years ago

Thanks so much @phallstrom!