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

Fix #96 double start message when build is manually started #137

Merged
merged 1 commit into from
Oct 27, 2015

Conversation

tterrag1098
Copy link
Contributor

This isn't technically tested but I think the change is pretty obvious

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@samrocketman
Copy link
Member

I've marked this as a candidate to merge. I'll test the behavior and if it behaves as expected then I'll merge it. If not, I'll respond why.

@tterrag1098
Copy link
Contributor Author

Just a note, I built my branch myself and installed it on my jenkins, and I could not see any of the slack options. Did something large change that I could have missed?

@samrocketman
Copy link
Member

Seems some information was lost in the patch. Here's a snapshot of behavior before/after.

snapshot of before and after

Seems there's some information lost when combining the two messages. @chrylis does this make sense or is your desire to have all contents combined?

I don't actively develop this plugin. I only maintain pull requests and releases.

@tterrag1098
Copy link
Contributor Author

The behavior I was testing against was the behavior of the current release, which is evidently different than the behavior of the master branch. Though I don't really see the purpose of the "Starting..." message either. Isn't it a bit redundant?

@samrocketman
Copy link
Member

The master branch significantly differs currently from the latest stable release. Therefore, it should be tested from master. It is redundant which is why this is being address by your pull request, correct? The missing information I'm referring to is "started after 2.1 sec". If you have a lot of jobs queued up and the job spent time in the Jenkins build queue then this time would be longer. So I don't think it's necessarily bad to have the information.

That's why I'm waiting on @chrylis to respond with what they originally intended. I have no qualms either way as I do not currently use the slack plugin. I maintain it for others who wish to contribute to its code base.

@samrocketman
Copy link
Member

Since not much feedback has been received from @chrylis I'm going to go ahead and approve this pull request as a candidate to merge.

@samrocketman samrocketman modified the milestone: slack-2.0 Oct 22, 2015
@samrocketman samrocketman merged commit f62088d into jenkinsci:master Oct 27, 2015
samrocketman added a commit that referenced this pull request Oct 27, 2015
build is manually started

closes #96
@MilesRoberts
Copy link
Contributor

@samrocketman: the test you posted looks like it dumps and/or ignores the custom message from the start messaging. Am I reading that right? edit: in looking at the code it looks like it's ignoring custom message if it passes scmCause == null which is a bug from my standpoint. We rely on the custom messaging to notify build parameters and some misc other info on start

MilesRoberts pushed a commit to MilesRoberts/slack-plugin that referenced this pull request Nov 30, 2015
@GuiSim
Copy link

GuiSim commented Feb 16, 2016

Any plans on merging this?

@samrocketman
Copy link
Member

@MilesRoberts feel free to open an issue and perhaps a PR to address the issue.

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.

5 participants