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

Added KatoIM support #755

Merged
merged 1 commit into from
Dec 13, 2014
Merged

Added KatoIM support #755

merged 1 commit into from
Dec 13, 2014

Conversation

kzaitsev
Copy link

Added KatoIM support

notify:
  katoim:
    room_id: $$KATOIM_ROOM_ID
    on_started: true
    on_failure: true
    on_success: true

@bradrydzewski
Copy link

Awesome! Two very minor comments

  1. Instead of using booleans, how do you feel about strings, similar to this:
    https://github.com/drone/drone/blob/master/plugin/notify/email/email.go

The thought is that if we omit on_started it will default to false. This would allow you to omit on_started and assume that means you want to send messages. Just an idea. Let me know if you like it.

  1. could we put this in a sub folder notify/kadoim? I'm starting to move all plugins into their own separate folders.

Thanks again!

@kzaitsev
Copy link
Author

@bradrydzewski nice idea

i set Started, Success, Failure as strings, now by default enabled only Success and Failure

Also we need to share this code with other notifications plugins
https://github.com/Bugagazavr/drone/blob/katoim/plugin/notify/katoim/katoim.go#L111-L139

Any idea when this can be placed?

@bradrydzewski
Copy link

@Bugagazavr awesome. It would be great to share this logic in the notify package but that would create a circular dependency. I'd like to treat all plugins like the remote package where they get registered at runtime, however, this complicates the yaml parsing.

I'll open an issue where we document and discuss some design ideas.

bradrydzewski added a commit that referenced this pull request Dec 13, 2014
@bradrydzewski bradrydzewski merged commit e70c8d5 into harness:master Dec 13, 2014
bot-harness pushed a commit that referenced this pull request Nov 1, 2023
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.

None yet

2 participants