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

moved message to 'text' instead of 'fields' to resolve truncated message issue #274

Merged
merged 2 commits into from
Mar 2, 2017

Conversation

p00j4
Copy link

@p00j4 p00j4 commented Dec 21, 2016

Change: moved message to 'text' instead of 'fields' to resolve truncated message issue
Reason: As per issue filed on slack api, the "value" of fields has max length of around 4k chars which leads to text truncated post that and since, there is no much usage of huge text, there is no "show more" wrapping link too so the message gets missed. the same problem is with "pretext" as well.

To solve that, changed to "text" which shows wrapped content with "show more" link

Why would someone need large text to send
My Use case: I have my automated tests running and let say there are failures, I have parsed way more huge logs of more than 10k lines and created this text having responsible 's committer name with a short summary for catching all's attention to look at their specific service so that my attachment is not ignored. Yes I could just give link for devs to open and see all details there but thats where the problem is, no one really like to open a link unless it looks like his/her own issue. in some cases, if there are more failures, obviously my text gets bigger and so I need all of them to come posted on slack (its ok to click on "show more" than not getting the content at all)

Note: if this PR sounds good, I'll raise 1 more by next week which will serve the benefits of having "fields" for which originally the plugin was written i.e. there will be 1 more field say "footer" where I will use "fields" to display fixed content upfront (without need to click on "show more")

  • Tests: Yes, Tests Passes

@p00j4 p00j4 changed the title moved message to 'text' instead of 'fields' to resolve truncated mess… moved message to 'text' instead of 'fields' to resolve truncated message issue Dec 21, 2016
@samrocketman
Copy link
Member

Thanks for opening the PR. I have tagged it with a label so others know it needs peer reviewed.

@samrocketman
Copy link
Member

I can't find where the message limits are defined for field and text. Please link to slack documentation so I can verify the limits and better understand the change.

@p00j4
Copy link
Author

p00j4 commented Dec 22, 2016

@samrocketman thanks for the quick attention. Yes, that's the problem. Seems like even Slack team wasn't aware until I filed the issue, so its not documented anywhere. I debugged fully to realize that it wasn't issue with my code and was related to large content support miss.

here is the screen-shot of the issue I have discussed with slack team (they probably, will update the docs or might give fix for having "show more" in fields case as well, but till when is not guaranteed yet ) Please Observe, James's reply:

screen shot 2016-12-22 at 9 00 31 am

screen shot 2016-12-22 at 8 59 09 am

@p00j4
Copy link
Author

p00j4 commented Dec 22, 2016

Note: one edit in my last note where I thought having fields, we can still support footer after truncated text wrapped with "show more" but I tested and found that in case of huge content, even the footer is missed out (considered the limits on whole message all together as it looks). so unfortunately the whole content is going to be missed and there is no way to get it at the moment. However, I really want a footer too. so I'm still trying to figure alternative, will reach you out when found.

@p00j4
Copy link
Author

p00j4 commented Dec 23, 2016

@samrocketman so are we going to merge this PR now? or please let me know if I missed out anything

@samrocketman
Copy link
Member

@p00j4 for the most part; I have taken a back seat role in this plugin and occasionally categorize and review PRs. I leave merging up to the current primary maintainer, @kmadel. He has been merging as they come in order. You may have to rebase this a few times as other possibly breaking changes get merged before this one.

@kmadel
Copy link
Contributor

kmadel commented Feb 1, 2017

Typically we like to see at least two non-maintainers do a review of the code and give a thumbs up before we merge. Anyone?

samrocketman added a commit that referenced this pull request Mar 2, 2017
'fields' to resolve truncated message issue.
@samrocketman
Copy link
Member

I have thoroughly tested this and will be merging it.

@samrocketman samrocketman merged commit 48ed161 into jenkinsci:master Mar 2, 2017
samrocketman added a commit that referenced this pull request Mar 2, 2017
Merge PR #274 moved message to 'text' instead of...
@samrocketman samrocketman added this to the slack-2.2 milestone Mar 6, 2017
@samrocketman
Copy link
Member

Feel free to subscribe to #296 for updates related to Slack Plugin 2.2 release.

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.

4 participants