Modernize this plugin - Githubissues

tomaswolf commented 9 years ago

This is a fairly large pull request, but I believe if you go through it commit by commit, you'll be able to follow what I did. I did try to have a clean and understandable commit history here...

The most fundamental change is a switch on the Jenkins base version from 1.409 and Java 5 to 1.565.3 and to Java 6 (minimum requirement for Jenkins since 1.520). I just didn't get the original version from master to build and run the tests successfully with the 1.409 POM. 1.565.3 is an LTS version from October 2014.

After that, I started fixing things that prevented me from running this plugin in our in-house setup. The individual commit messages explain it all.

Making this work for jobs with blanks in the name (commit cb2f319b) required rewriting more of maven-scm since the root cause was in there.

The last two commits attempt to fix JENKINS-26204. I never could reproduce that problem, but in looking through the code for a possible cause I stumbled upon one possible race condition (though that should not affect an installed plugin since it doesn't use "synchronous" transactions) and discovered that the path handling in the plugin suffered from the same problem as maven-scm.

Not dealt with is the "poisoned commit" problem. Currently the plugin leaves a failing commit in its queue and will retry it forever again (until Jenkins is restarted). If these retries also fail, no further commits to the SCM repository will ever succeed. It's unclear to me what the best strategy would be; just skipping the "bad" commit is no solution as it might make subsequent commits also fail. Also, such a "bad" commit may leave behind an inconsistent file and repository state (some paths added/staged, some not). Probably the best strategy to deal with failing commits is to stop the plugin (no more recording), require the user to fix it, and then let the user restart the plugin. At least that would not require the user to stop and restart the whole Jenkins.

fcamblor commented 9 years ago

Hi, it sounds great !

I'm ashame that I didn't worked on the plugin since a long time, and don't think I will have lot of time to spend on it for maintenance. I'll try to review the PR during my holidays (in 2 weeks), but I'll understand if you want it to be merged fast before that time.

I'm looking forward to find new maintainers on the plugin, if you want to take this responsibility, I will greatly be happy to share it given the hard work you did. It may infuse some young blood on this plugin.

WDYT ?

jenkinsadmin commented 9 years ago

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

tomaswolf commented 9 years ago

There's no hurry. The bugs fixed by this PR are all open for quite some time already, so a few more weeks won't hurt. And I can build that thing myself and install that self-built plugin :-)

I understand that you may not have much time to maintain this old plugin. That happens. As for maintaining it myself: I fear I also won't have much time for it. I already spent too much time on this :-(, even though I refrained from any major refactorings to improve separation of concerns. But also: I still see three large things to be done in this plugin, and at the moment I have no clear idea on how to tackle either of them:

move from maven-scm to the Jenkins plugins (SCM API, subversion plugin, git & git-client plugin). In order to not force people to have to install the git stuff if they only have svn repos, the git plugins would need to be an optional dependency, which may complicate matters further. Also,I've taken a brief look at the SCM API, and it's not entirely clear to me yet how I'd go about using that instead of maven-scm. But maven-scm also is in a sorry state and doesn't look well maintained -- lots of old patches and pull requests that got never applied...
the "restore" functionality needs, besides the POST issue JENKINS-19131, functionality to let the user choose from the repo history what he wants to restore. Implementing that looks like a lot of work.
that "poisoned commit" problem mentioned above. Evidently there's even users who _do_modify the repo directly: JENKINS-24811.

As for "young blood": I'll take that as a compliment. I did program on VAXen, though :-)).

fcamblor commented 9 years ago

You're right about enhancements.

I captured my vision about some of your points in JENKINS-18129 and JENKINS-18124

cornelf commented 9 years ago

:+1: great stuff @tomaswolf

bdkoepke commented 9 years ago

I've actually got a fairly large commit as well that I was almost finished working on before I saw this. @fcamblor would you be interested in adding another 'collaborator' to the project? The company I work for is very interested in the continued maintenance of this project?

bdkoepke commented 9 years ago

Checking out the version in this pull request, is it the correct behavior for the plugin to perform a git add but not a git commit && git push? When is git commit && git push supposed to be called? I would think that would happen once you hit 'Submit comment'.

tomaswolf commented 9 years ago

@bdkoepke: all the actual repository operations happen in ScmSyncConfigurationBusiness.processCommitsQueue(). Line 208 commits and pushes (in git) or checks in (in svn) the changes. The plugin maintains its own commit objects, queues those and then processes this queue asynchronously.

trevoro commented 9 years ago

+1

cornelf commented 9 years ago

:+1: Loads of bug-fixes and improvements in this PR, it deserves a 1.0.0 release.

jessesanford commented 9 years ago

+1

kleini commented 9 years ago

+1

kyounger commented 9 years ago

+1

svvitale commented 8 years ago

Any progress on getting this merged? Good stuff here!

cmarquezrusso commented 8 years ago

+1

clarkstuth commented 8 years ago

Why has this not been merged yet? I'm also running into this issue.

kpmueller commented 8 years ago

Please merge this :(

+1

cmarquezrusso commented 8 years ago

+1

rodrigc commented 8 years ago

@tomaswolf: This is good stuff. I use SCM sync heavily, and have hit JENKINS-24686 before. Do you want to become a maintainer for this plugin so that you can merge this stuff in? If not I can ask @fcamblor if I can be added as a maintainer for this plugin.

tomaswolf commented 8 years ago

@rodrigc, @fcamblor: as I had written above, I fear I also won't have much time for maintaining this plugin, so it wouldn't make much sense to add me as a maintainer.

rodrigc commented 8 years ago

@tomaswolf: fair enough. This plugin is too important, and you did a lot of good work, to just let sit there. I will step up to get your stuff in. @fcamblor: is it OK if I ask to become a maintainer for this plugin?

rodrigc commented 8 years ago

@tomaswolf: just to verify, are you using your patch to SCM Sync in a production environment? I heavily use your plugin and have hit some of the issues you have fixed in your patch, so just want confirm.

tomaswolf commented 8 years ago

@rodrigc: yes, I'm using this in a production environment. Jenkins 1.620, using a Gerrit-managed git repo. (Gerrit 2.11.4) Works fine for us. Apart from the problems mentioned above and explicitly not handled by this pull request (poisoned commit, no support in the plugin to restore, no way to stop and restart just the plugin, plus a few minor things such as no way to specify the branch to push to -- but there's another PR by somebody else who took a stab at that) we encountered no problems with the plugin itself.

We do occasionally run into a problem with pushes, but that's unrelated to the plugin. It's some compatibility problem with thin packs between the old cgit version our Jenkins uses (1.9.4) and the JGit in Gerrit; when it happens once in a while, we commit manually using git push --no-thin.

rodrigc commented 8 years ago

I sent a request to add myself as a plugin maintainer so that I can help get your fixes in. https://groups.google.com/forum/#!topic/jenkinsci-dev/jSxy6Llr2is

jessesanford commented 8 years ago

@rodrigc thanks for taking the lead on this!