improve 'started' message with proper cause #37

jackbearheart · 2014-12-18T23:34:45Z

Before, the started message would say the build had been 'started
by changes ...' if a user started a build when there were changes
to be pulled in.

Now if a user starts the build, it will always say 'started by user',
but if a user didn't start the build, it will still show the changes.

jenkinsadmin · 2014-12-19T01:02:09Z

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

samrocketman · 2015-01-30T06:23:56Z

Can you please expand upon what this is for?

I guess I'll start maintaining this plugin. I don't have any experience developing Jenkins plugins but if I can get my own environment set up and learn to release then I'll do it. This may take about a week.

jackbearheart · 2015-01-30T15:58:46Z

I want the actual cause to be in the notification, but before this changeset the cause is incorrect. The notification message would say that the build was "started by changes" even if it was actually started by a human clicking "build". So now the cause is accurate.

samrocketman · 2015-01-31T01:02:40Z

Thanks for elaborating.

samrocketman · 2015-01-31T01:03:17Z

Candidate for merge means I will first merge your change locally, compile locally, and test locally in Jenkins before merging.

samrocketman · 2015-01-31T19:58:30Z

pom.xml

@@ -14,7 +14,7 @@
    <parent>
        <groupId>org.jenkins-ci.plugins</groupId>
        <artifactId>plugin</artifactId>
-        <version>1.447</version>
+        <version>1.567</version>


What does updating this do? Does it specify the minimum version of Jenkins?

I realize this is upgrading the version in which unit tests are run against. Can you confirm my assumption?

https://wiki.jenkins-ci.org/display/JENKINS/Choosing+Jenkins+version+to+build+against

Thanks for clarifying @daniel-beck.

samrocketman · 2015-02-01T16:48:49Z

I merged this and tested it; I couldn't see the difference between before and after. Can you give me a scenario where I can reproduce the old behavior and see the new behavior post-merge?

samrocketman · 2015-02-01T22:30:01Z

@jackbowman I've merged so many other PRs that the current master conflicts pretty hard with your changes. Can you pull the latest changes from master into your request? Here's how I attempted to do it. The following commands are from the slack plugin git repository on my system.

#pull in latest changes
git fetch
#a5d6d04 is a point before I performed dos2unix
git checkout a5d6d04
#download PR #37 and merge it
git ls-remote origin | grep 37
git fetch origin refs/pull/37/head
git merge FETCH_HEAD
#resolve the conflicts in ActiveNotifier.java and then run dos2unix
dos2unix src/main/java/jenkins/plugins/slack/ActiveNotifier.java
#all conflicts have been resolved so add all files
git add .
#conclude the merge
git commit
#I am assuming origin/master is this upstream repository https://github.com/jenkinsci/slack-plugin
git merge origin/master
#at this point there's significant merge conflicts that I can't resolve them myself.

I had to merge this from an earlier point because I performed dos2unix on ActiveNotifier.java when I merged PR #25 into master. dos2unix changes windows line endings to unix line endings in a file.

Run mvn test after you've resolved the conflicts and push an update to this merge request.

samrocketman · 2015-02-04T06:40:58Z

@jackbowman I plan to release slack plugin 1.7 this weekend. If you would like your change to make it into that release then please resolve the conflicts before the weekend so I can merge it.

jackbearheart · 2015-02-04T18:51:35Z

I'll try to do this before the weekend.

samrocketman · 2015-02-04T21:56:46Z

If not then it can make it into the next release. I'll try to maintain a monthly release schedule if there's any pull requests that have been made.

jackbearheart · 2015-02-06T21:24:59Z

Ok, I don't think I'll have time to merge before you release 1.7. Thanks for keeping me up to date.

peergum · 2015-02-06T22:06:11Z

Jack, you can make it much easier if you convert all CRLF's in your conflicting files to simple LF's first, then pull from master.

peergum · 2015-02-07T05:27:57Z

I think that even if the build is started manually, the list of pending changes - if any - should still be shown in the start notification, because you may want to know what is being built anyway... I'd remove the return statement at the end in your (scmCause == null) test.

samrocketman · 2015-02-15T15:42:06Z

Since the release is delayed due to #47 if you rebase and resolve the conflicts you might still make it in before the 1.7 release.

Before, the started message would say the build had been 'started by changes ...' if a user started a build when there were changes to be pulled in. Now if a user starts the build, it will always say 'started by user', but if a user didn't start the build, it will still show the changes.

jackbearheart · 2015-03-06T15:49:50Z

I rebased the change.

You can observe the changes introduced by this plugin by following these steps:

Add a job named "test"
Add slack notifications, click "notify on build start"
Create an empty repo on your machine.
Add the SCM option, configure polling to "* * * * *".
Make a commit, and before the polling kicks in, click build.
Observe the message "test - Merge in community changes #1 Started by user anonymous"
Make a commit and wait for the polling to pick it up -> observe the message "test - Changing bot name and icon in post-build action #2 Started by changes from [...]"

Before this commit, step 6 has a different result ("test - #1 Started by changes from [...]").

peergum suggested always including the changes (removing the return statement in this change), but for my use case I don't actually want that; the changes are irrelevant to me I just want to know that someone manually started the build.

samrocketman · 2015-03-06T17:23:51Z

I'll compile and test this later. If all is well this will be the next thing I merge.

peergum · 2015-03-06T18:34:05Z

Any specific change to one individual may not necessary be good for others... the idea is to keep compatibility with the existing way things work. In this specific case, someone might start the build manually, but it will still include the changes and so people won't be informed about the changes being built...

For the note, if this gets merged as is, it will be the second time a specific update prevails over the common goal, and I'll start forking and maintaining my own slack-plugin repo.

samrocketman · 2015-03-06T18:51:54Z

@peergum what was the first time? Threatening to fork is not very productive. If there's something you wish to happen then opening a pull request is more productive.

Also, what needs to be changed in this PR that would otherwise break compatibility?

jackbearheart · 2015-03-06T19:18:36Z

I'm all for looking for the common goal. Let's talk use cases. I've no intent to bust things for other people here.

When my build/test servers are running along, everything happens automatically and so no build has to be triggered by a human. But inevitably something goes wrong, or there is an action that is not automated and a human has to decide when to do it. In that case, I think the important thing is always what human has decided to issue the command. Hence this changeset.

Now, I suppose the counter argument is that you still want to know what changes are being built/tested/deployed anyway, when a human takes an action. I kind of see that, but I don't want the slack plugin to produce a lot of spam in chat. There may be a lot of changes that are irrelevant to the overall action (i.e. "Jack ran [Deploy internal tool X]".) Chat does not need a long list of changesets. If someone wants to read them, they can click the link.

It could be an option, to please both of us, and it kind of already is because have this option "Show Commit List with Titles and Authors".

Let me know what you think.

samrocketman · 2015-03-06T23:09:45Z

I'll defer to @peergum on that. I don't use the plugin and only maintain it. So it would be better to get his, and perhaps others' perspectives on what they want to get out of this.

samrocketman · 2015-03-17T23:14:14Z

I'll let this pull request incubate a few more days for feedback. If nobody pipes up then I'll set this to be merged and eventually merge it.

samrocketman · 2015-04-11T17:44:45Z

src/main/java/jenkins/plugins/slack/ActiveNotifier.java

+                MessageBuilder message = new MessageBuilder(notifier, build);
+                message.append(causeAction.getShortDescription());
+                notifyStart(build, message.appendOpenLink().toString());
+                return;


Since this is a public void function I will remove the return statement as @peergum had mentioned earlier post-merge. I realize that it's not actually returning anything but I think it's a little cleaner. I'm doing this as post-merge to expedite merging your change.

cause.

samrocketman · 2015-04-12T02:08:52Z

@jackbowman I realize after reading up that return in a public void statement is valid. So I may have made a mistake by removing the return statement from your patch post-merge. Is there a reason you think I shouldn't have removed the return statement post-merge? Please let me know.

jackbearheart · 2015-04-13T13:12:54Z

Well, it changes the behavior of the function! So you should be careful about doing that in general, we can't just arbitrarily remove return statements.

However, it's what peergum thought was better for everyone anyway, and I don't mind so much. So, maybe he'll be happy and I won't care :)

samrocketman · 2015-04-13T22:39:51Z

Well, it changes the behavior of the function! So you should be careful about doing that in general, we can't just arbitrarily remove return statements.

Yeah, thanks for the advice. I'll be more patient about waiting for feedback. In reality, I didn't see a good reason one way or another.

samrocketman added the feedback needed label Jan 30, 2015

samrocketman added candidate to merge and removed feedback needed labels Jan 31, 2015

samrocketman self-assigned this Jan 31, 2015

samrocketman reviewed Jan 31, 2015
View reviewed changes

samrocketman mentioned this pull request Feb 1, 2015

Updated to show the list of commits (title+author) #30

Merged

samrocketman modified the milestone: slack-1.7 Feb 4, 2015

samrocketman modified the milestones: slack-1.7, slack-1.8 Feb 10, 2015

samrocketman added feedback needed and removed candidate to merge labels Feb 25, 2015

samrocketman removed this from the slack-1.8 milestone Feb 26, 2015

jackbearheart force-pushed the master branch from 26ccdcd to b4d1927 Compare March 6, 2015 15:25

samrocketman added this to the slack-1.8 milestone Mar 6, 2015

samrocketman added candidate to merge and removed feedback needed labels Mar 6, 2015

samrocketman added feedback needed and removed candidate to merge labels Mar 6, 2015

samrocketman added the code review requested label Mar 6, 2015

samrocketman added candidate to merge and removed code review requested feedback needed labels Mar 29, 2015

samrocketman reviewed Apr 11, 2015
View reviewed changes

samrocketman merged commit b4d1927 into jenkinsci:master Apr 11, 2015

samrocketman added a commit that referenced this pull request Apr 11, 2015

Merge PR #37 improve 'started' message with proper

f61cbab

cause.

samrocketman added a commit that referenced this pull request Apr 11, 2015

Removing return as recommended in #37

120fc70

JamesMahy mentioned this pull request May 30, 2019

Slack publish failed, response code 404 #480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve 'started' message with proper cause #37

improve 'started' message with proper cause #37

jackbearheart commented Dec 18, 2014

jenkinsadmin commented Dec 19, 2014

samrocketman commented Jan 30, 2015

jackbearheart commented Jan 30, 2015

samrocketman commented Jan 31, 2015

samrocketman commented Jan 31, 2015

samrocketman Jan 31, 2015

samrocketman Feb 1, 2015

daniel-beck Feb 1, 2015

samrocketman Apr 11, 2015

samrocketman commented Feb 1, 2015

samrocketman commented Feb 1, 2015

samrocketman commented Feb 4, 2015

jackbearheart commented Feb 4, 2015

samrocketman commented Feb 4, 2015

jackbearheart commented Feb 6, 2015

peergum commented Feb 6, 2015

peergum commented Feb 7, 2015

samrocketman commented Feb 15, 2015

jackbearheart commented Mar 6, 2015

samrocketman commented Mar 6, 2015

peergum commented Mar 6, 2015

samrocketman commented Mar 6, 2015

jackbearheart commented Mar 6, 2015

samrocketman commented Mar 6, 2015

samrocketman commented Mar 17, 2015

samrocketman Apr 11, 2015

samrocketman commented Apr 12, 2015

jackbearheart commented Apr 13, 2015

samrocketman commented Apr 13, 2015

improve 'started' message with proper cause #37

improve 'started' message with proper cause #37

Conversation

jackbearheart commented Dec 18, 2014

jenkinsadmin commented Dec 19, 2014

samrocketman commented Jan 30, 2015

jackbearheart commented Jan 30, 2015

samrocketman commented Jan 31, 2015

samrocketman commented Jan 31, 2015

samrocketman Jan 31, 2015

Choose a reason for hiding this comment

samrocketman Feb 1, 2015

Choose a reason for hiding this comment

daniel-beck Feb 1, 2015

Choose a reason for hiding this comment

samrocketman Apr 11, 2015

Choose a reason for hiding this comment

samrocketman commented Feb 1, 2015

samrocketman commented Feb 1, 2015

samrocketman commented Feb 4, 2015

jackbearheart commented Feb 4, 2015

samrocketman commented Feb 4, 2015

jackbearheart commented Feb 6, 2015

peergum commented Feb 6, 2015

peergum commented Feb 7, 2015

samrocketman commented Feb 15, 2015

jackbearheart commented Mar 6, 2015

samrocketman commented Mar 6, 2015

peergum commented Mar 6, 2015

samrocketman commented Mar 6, 2015

jackbearheart commented Mar 6, 2015

samrocketman commented Mar 6, 2015

samrocketman commented Mar 17, 2015

samrocketman Apr 11, 2015

Choose a reason for hiding this comment

samrocketman commented Apr 12, 2015

jackbearheart commented Apr 13, 2015

samrocketman commented Apr 13, 2015