nemccarthy / stash-pullrequest-builder-plugin

A Jenkins plugin for Building Stash Pull Requests
https://wiki.jenkins-ci.org/display/JENKINS/Stash+pullrequest+builder+plugin
Other
64 stars 130 forks source link

Manage history of builds internally to reduce comments sent to Stash #52

Closed blaffoy closed 2 years ago

blaffoy commented 8 years ago

Adds options to disable messages sent to Stash, as normal operation of this plugin results in a large number of automated messages swamping development work.

As the plugin depends on Stash messages to manage state, also added a class to manage history of builds internally. This collects a Set of all merges (branch commit + target commit) and comments (comment ID number) that have triggered a build. If a build has already been triggered for a given PR or comment ID, then it will not be rebuilt.

nemccarthy commented 8 years ago

How does this work between Jenkins restarts?

blaffoy commented 8 years ago

Hmmm... good point. Right now, it does not. It'll rebuild all eligible PRs on a restart.

The implementation in the git-scm plugin uses Serializable to persist between restarts. I'll copy that.

nickbroon commented 8 years ago

:+1: It would be great to be able to restrict comments made to Stash, in particular disabling the 'start' comment. It would also be great to be able to choose to only have it comment on failure.

blaffoy commented 8 years ago

@nickbroon. Yes, it would be nice, but @nemccarthy's point about restarts is completely valid. Using the branch I've developed in production means that a restart of Jenkins will cause every single pull request to rebuild.

Unfortunately, I haven't found the time to investigate serializing the build history in between restarts. The Jenkins git plugin achieves this with the Serializable trait, but I haven't completely understood how. I would like to follow the practises of that plugin as much as possible.

nickbroon commented 8 years ago

Adding serializing between restarts would be great, but I don't think it's blocker on this work. In most case the the number of open pull request is usually small, and rebuilding them wouldn't too much of an issue, only resulting in an additional comment being posted, if the comment is configured on.

nemccarthy commented 8 years ago

@nickbroon @blaffoy I'd like to see the serialisation working first as the number of jobs likely depends on your setup. We have Jenkins serving a few larger teams here and we currently have 12 open PRs. Each PR build takes about 10 mins. SO about 2 hours on build time and would fill up all our slaves after a restart. So I'd really like to see some serialisation of the history. Or a database of history.

nickbroon commented 8 years ago

@blaffoy @nemccarthy either you had any luck with looking at serialising the build history?

blaffoy commented 8 years ago

@nickbroon, I have not. I can't even predict when I might have the time to do so. The non-persistent version of this is good enough for my purposes, though I fully understand why it's not suitable for merging, so I can't really take the time to fix it.

nemccarthy commented 8 years ago

@nickbroon likewise, it would be great if someone could help out with this one

nickbroon commented 8 years ago

Just a FYI for myself and others. A link to the relevant bit of the Git plugin that implements persistence of build state: https://github.com/jenkinsci/git-plugin/blob/master/src/main/java/hudson/plugins/git/util/BuildData.java

gthicks commented 8 years ago

Can we merge this into a branch? I want to give implementing persistence a shot but I cannot build on what is already here.

nemccarthy commented 8 years ago

@gthicks you can just pull and merge with master manually to get going;

git checkout -b blaffoy-optional-messages-to-stash master
git pull https://github.com/blaffoy/stash-pullrequest-builder-plugin.git optional-messages-to-stash

then merge the changes;

git checkout master
git merge --no-ff blaffoy-optional-messages-to-stash

and you can then start working on the serde implementation.

gthicks commented 8 years ago

I looked into how the the Git Plugin manages and stores state. That plugin stores the entire history of whats been built up to that point on the latest build using the Action mechanism provided by Jenkins.

The plugin then reads that BuildData, compares it against the current state of the repository, triggers a build if necessary, adds the new build info to the previous build info, and persists that in the latest build.

This logic is implemented here

I will continue to work on this, but I am not sure how much time I will be able to dedicate to it. Hopefully this analysis helps.

nemccarthy commented 8 years ago

What is the status of this one? Did we get history being persisted?

Kaned1as commented 8 years ago

No. History is static and stored in memory.

chingcodes commented 8 years ago

Has anyone made any progress on the history persistence?

I am willing to help or take a stab at it myself.

mandrizzle commented 8 years ago

Just throwing out an idea, rather than using the comments, we can use the build status for the commit in stash....not 100% ideal but better than leaving a comment each time.

See https://developer.atlassian.com/static/rest/stash/3.11.3/stash-build-integration-rest.html

We can check if there is a build status for a commit, if there is one, we don't build. And adding a status is a simple post, and there is already a pr open (PR #77) that adds support sending the notification

nemccarthy commented 8 years ago

This sounds like a good idea - when a new commit is made to a PR we can tell stash to clear the build status.

Seems like a much smarter way of doing this!

On Thursday, 12 May 2016, Mandeep Singh notifications@github.com wrote:

Just throwing out an idea, rather than using the comments, we can use the build status for the commit in stash....not 100% ideal but better than leaving a comment each time.

See

https://developer.atlassian.com/static/rest/stash/3.11.3/stash-build-integration-rest.html

We can check if there is a build status for a commit, if there is one, we don't build. And adding a status is a simple post, and there is already a pr open (PR #77 https://github.com/nemccarthy/stash-pullrequest-builder-plugin/pull/77) that adds support sending the notification

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/nemccarthy/stash-pullrequest-builder-plugin/pull/52#issuecomment-218688401

Cheers,

NW

nickbroon commented 8 years ago

Build statuses are stored on commits not PRs. So clearing build status does not make sense for this. You simply have to check if the commit at head of PR branch has a build status associated with it.

mandrizzle commented 8 years ago

I can't think of a scenario where this would not work. Even if a commit has multiple jobs that run and they each notify stash, it wouldn't matter since it saves a list of statuses in the commit. We just find the one we left to see if we built it already

blaffoy commented 8 years ago

@mandrizzle Could you clarify if this would work in the scenario where I'm using Jenkins to merge and build each PR with master? In this case, I would want to log a build as having happened against both the git SHA of the current head of the branch, and the current head of master. So, for my purposes, it wouldn't be sufficient to save a build status against a Git SHA on Stash, but I'd also need to save the SHA of master so that it would be rebuilt when master changes.

mandrizzle commented 8 years ago

@blaffoy I think in your scenario, you will still see the build status in your master branch commit history since you are merging commits that contain build status. I am not sure if we also need to send a build status for the commit found in /merge to indicate the merge is building successfully, but that is sorta out of scope of what we are trying to do.

blaffoy commented 8 years ago

@mandrizzle Interesting. I use a workflow where PRs are merged back to master locally on the Jenkins server and run through a CI pipeline. The results of that pipeline are sent back to the PR on Stash in order to prevent merging a bad PR. Having Jenkins manage its own record of what merges it has completed works better for me in that case.

Of course, it's on me to implement that, and six months later I still haven't done it. :-\

mandrizzle commented 8 years ago

We can also use SQLight like this plugin

https://github.com/jenkinsci/dbCharts-plugin/blob/master/src/main/java/hudson/plugins/dbcharts/SqliteJDBCConnection.java

nemccarthy commented 7 years ago

Would be great to get something like this working if anyone has time...

Kaned1as commented 7 years ago

I have a patch for that (uses stash commit-build system as @mandrizzle noted above), but it's from heavily modified plugin for our very specific needs. If you have an hour or two to adapt it, I'd be glad to share.

nemccarthy commented 7 years ago

@Adonai would be great if you could share/make a PR. Then someone could come along and extend what you have done to make it a bit more generic. Thanks!

Kaned1as commented 7 years ago

Sorry it took so long. My employer prohibits github access so I couln't save history from original repo.

Was finally able to upload my work here: https://gitlab.com/Kanedias/StashPrBuilder

Codebase is mostly same, I renamed DTOs a bit, all logic with build-commits resides in StashApiClient

willwolfram18 commented 6 years ago

Will conflicts for this be resolved so that it can be merged and released?