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

Add ability to use cards #18

Closed
wants to merge 9 commits into from

Conversation

robert-bo-davis
Copy link

@robert-bo-davis robert-bo-davis commented Apr 30, 2016

This adds rudimentary support for hipchat cards. Cards are much more extensible than the basic message format.

This maintains legacy behavior by default.

Here is the normal message, a collapsed card, an expanded card, and a collapsed failure:
screen shot 2016-05-01 at 10 52 42 am

@tboerger
Copy link
Contributor

tboerger commented May 3, 2016

The code looks good so far, but now I know what you mean. Downstream is not really the correct icon reference

@jackspirou
Copy link
Contributor

I need more time to really review this (I no longer use HipChat) but @mujiburger what do you think of having cards as the default behavior?

@robert-bo-davis
Copy link
Author

robert-bo-davis commented May 3, 2016

I was worried about breaking somebody's custom templates for the original message style without telling them, but it doesn't look like that feature has been out for that long. Providing a disable_cards flag so there is an easy fix if somebody needs their original template and doesn't want to spend time figuring out the card templating may be enough. Let me know if that is the preferred behavior and I'll make it so.

Any input on the default card template would be appreciated. I went for less clutter in the collapsed card, but some may prefer more information without a click. The "activity" which is what shows before the card is expanded supports a pretty full subset of HTML so it is flexible.

@bradrydzewski
Copy link
Contributor

bradrydzewski commented May 9, 2016

If cards have different behavior, and different configuration, should they be two separate plugins? Perhaps a hipchat and hipchat_card plugin? I would rather have two plugins with simple configurations than a mono-plugin with many configuration options that is more difficult to understand.

@bradrydzewski
Copy link
Contributor

btw, the card functionality looks awesome

@robert-bo-davis
Copy link
Author

robert-bo-davis commented May 9, 2016

Since it is buried in the documentation updates of the PR, I wanted to make sure it was clear. As of right now, cards aren't supported on the hipchat mobile apps. The plugin sends both formats and those apps display the default message. A separate cards plugin would need to provide the same functionality as the main plugin in addition to handling cards.

@tboerger
Copy link
Contributor

What should we do now? Migrate the drone config to 0.5 and merge this pr?

@bradrydzewski
Copy link
Contributor

@tboerger I would prefer to accelerate efforts to create plugins.drone.io to allow developers to publish plugins or plugin variations

@tboerger
Copy link
Contributor

@bradrydzewski so you want to close this PR?

@nlf
Copy link

nlf commented Jun 16, 2016

code here lgtm

does need updated with master though, and also pending @bradrydzewski's decision here

@appleboy
Copy link

Any updates?

@jmccann
Copy link
Owner

jmccann commented Dec 7, 2016

@mujiburger I'm interested in supporting this. If you mind updating with the 0.5 updates I made I would totally review. Thanks!

@robert-bo-davis
Copy link
Author

I took a quick look this morning. The conflict is strong with this one and I'm swamped at the moment. I'll take a harder look at it after the holidays.

@jmccann
Copy link
Owner

jmccann commented Dec 8, 2016

Yea, it was a major update to move to supporting Drone 0.5. Take your time!

@jmccann
Copy link
Owner

jmccann commented Dec 16, 2016

Just curious if/how refactoring this plugin to use some hipchat golang libraries would affect this effort? May make it easier?

Looking at potentially using either:

@jmccann
Copy link
Owner

jmccann commented Mar 7, 2017

Closing for now as I don't have time to update it and it seems to have gone stale.

@jmccann jmccann closed this Mar 7, 2017
@jmccann jmccann mentioned this pull request May 17, 2017
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.

7 participants