-
Notifications
You must be signed in to change notification settings - Fork 413
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
Send more advanced slack messages v2 #324
Conversation
if attachments doesnot contain fallback set message as fallback JSONArray converted to String ReadMe modified ReadMe modified test fix
Improved readme
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.
looks good to me. let's do this!
Can someone review this? |
Hey @gurumaia, I'll take a look as well and try bootstrapping it with https://github.com/samrocketman/jenkins-bootstrap-slack. Please also refer to #320 as I generally am attempting to distance myself from this plugin specifically. |
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.
Looks good, no apparent issues.
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.
Doc looks good to me, test looks good.
Alright, great @samrocketman. I did see your commits about removing yourself as a maintainer. I can try to step up and help maintain the plugin if you'd like. |
@gurumaia you're welcome to step up and maintain. Feel free to check in with @kmadel who, as far as I know, has not relinquished being the other maintainer. I'm sure he wouldn't mind bringing you on board. Feel free to send a message to the developers mailing list, mention Kurt, and this discussion. |
Would be amazing to have this reviewed and hopefully approved. Is @gurumaia taking it over? |
Hey @djk I'll contact @kmadel to see if I can get in. This has been on the back of my mind lately as I've had quite a few rushed days these past few weeks. |
Thanks @gurumaia, I'll check your repo! Wasn't hassling you, hope it doesn't feel that way. Thanks for the great work. |
Absolutely no hassling =). btw, I didn't actually code this, I just took @ahmetrehaseker's old PR and performed @samrocketman's suggestions. |
Ah right, well thanks to @ahmetrehaseker and @samrocketman too!
EDIT: Got it to work with help of a friend, like:
And repeating for more fields etc. EDIT: The above doesn't seem to work in regard to actually doing the interpolation of variables. Is there some escaping required? Thanks again! |
@djk sorry, I missed your edit.
@kmadel If you could review this PR, I can go ahead and merge it. |
Decent doc update, it has tests, looks ok to me. I so go for it @gurumaia and thanks. |
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.
Looks good, has tests, new features are much needed in a lot of cases.
Is this being merged? |
+1 for this feature getting merged now that it appears to have tests and looks good. |
I've merged and now I'm waiting for approval on my request for upload access to actually release it: jenkins-infra/repository-permissions-updater#399 |
@gurumaia Any idea on timing to release this? I noticed that your above referenced PR giving you access has been merged. Thanks again for your help taking this over. |
Hey @brianespinosa I've finally released it to the update center yesterday. Should be available now. Please test it and let me know if it works well for you. |
Doesn't this revert the fix in 3905e00#diff-282db0eb13a24d1729df3e194a35f6d7 #274 made by @p00j4 ? |
Based on @ahmetrehaseker's implementation.
This is a change to support slack complex messaging.
See #285 for more details.