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

Don't publish twice, while the build isn't finished yet #446

Merged
merged 1 commit into from
Dec 17, 2018

Conversation

KreAch3R
Copy link
Contributor

Without this patch:

test - #23 Started by user admin (Open)
test - #23 Success after 12 sec and counting (Open)
test - #23 Success after 13 sec (Open)

With this patch:

test - #27 Started by user admin (Open)
test - #27 Success after 12 sec (Open)

Without this patch:
```
test - jenkinsci#23 Started by user admin (Open)
test - jenkinsci#23 Success after 12 sec and counting (Open)
test - jenkinsci#23 Success after 13 sec (Open)
```

With this patch:
```
test - jenkinsci#27 Started by user admin (Open)
test - jenkinsci#27 Success after 12 sec (Open)
```
@@ -104,7 +104,7 @@ public void finalized(AbstractBuild r) {
AbstractProject<?, ?> project = r.getProject();
Result result = r.getResult();
AbstractBuild<?, ?> previousBuild = project.getLastBuild();
if (null != previousBuild) {
if (null != previousBuild && !r.isBuilding()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when does this happen?
I tested master and this branch, (with a very simple job) and both had the same output

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also troubled by this, I noticed it only now that I was working with #445 . My build job is a simple shell execution of:

sleep 2s
exit 0

And I keep getting 2 notifications when the build completes, which (I assume) are because finalized is fired very early, while jenkins hasn't marked up the build as finished. I looked into the jenkins source code, Run.java:
https://github.com/jenkinsci/jenkins/blob/bcc4e4fe9c3dccc5ca74299d24f8f992c9d5cd71/core/src/main/java/hudson/model/Run.java#L737-L745

    public @Nonnull String getDurationString() {
        if (hasntStartedYet()) {
            return Messages.Run_NotStartedYet();
        } else if (isBuilding()) {
            return Messages.Run_InProgressDuration(
                    Util.getTimeSpanString(System.currentTimeMillis()-startTime));
        }
        return Util.getTimeSpanString(duration);
    }

And Messages.properties:
https://github.com/jenkinsci/jenkins/blob/584c34cda584b98b71d82d038e7d5ebe38534e18/core/src/main/resources/hudson/model/Messages.properties#L233

Run.InProgressDuration={0} and counting

And confirmed that the output I get, test - #23 Success after 12 sec and counting (Open) comes up because the isBuilding() is still true.

My jenkins env is:

Virtual box machine
Debian GNU/Linux 9.6 (stretch)
jenkins: 2.155

I get the duplicate messages in our current master as well:

test - #41 Started by user admin (Open)
test - #41 Success after 4.1 sec and counting (Open)
test - #41 Success after 4.9 sec (Open)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KreAch3R I use JobDSL for creation job. -there is no duplicate.
Manually created : also only 2 messages in slack room;

test_misc » test_manuakl_slack - #1 Started by user Denis Shvedchenko (Open)
custom general mssage

test_misc » test_manuakl_slack - #1 Success after 0.3 sec and counting (Open)
custom success message

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on my test job with either version, let's try it, can back it out if it causes regressions

@timja timja merged commit e66caf6 into jenkinsci:master Dec 17, 2018
timja added a commit to timja/slack-plugin that referenced this pull request Dec 20, 2018
timja added a commit that referenced this pull request Dec 20, 2018
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.

3 participants