Modernize this plugin #31
Conversation
I did not manage to get this going with the 1.409 POM. And anyway newer Jenkins versions have some changes that this plugin should account for. POM changes: * Increase minimum Jenkins version to 1.565.3 (LTS from October 2014) and adapt POM as needed to get tests to run successfully in my Eclipse/m2e setup. Also clean out some obsolete stuff relating to easymock, which isn't used anymore. * Add a license (MIT). * Include some Eclipes m2e-specific "lifecycle mappings". Unfortunately that's needed for Eclipse users, otherwise nothing will work. * Fix some versions. * Resolve the sisu-vs.-plexus problems by using a resent sisu-inject-plexus and making sure that comes first and thus wins. Exclude the very old plexus container stuff pulled in via maven-scm-manager-plexus; sisu-inject-plexus provides a newer one. Didn't touch the SCMManagerFactory, though. Code changes: * Mock Jenkins, too, not just Hudson. Miscellaneous: * Add the Eclipse m2e annotation processor file to .gitignore
It's the minimum requirement for Jenkins since 1.520.
Since PR jenkinsci#19 at GitHub (commit 141cc2b), we don't get a commit message dialog anymore for changes to job configurations. The regexp looks broken with the initial slashes and only a single [^/]. With the Cloudbees Folders plugin jobs can be not only at jobs/Some_Job but also at jobs/Folder1/jobs/Folder2/jobs/Some_Job This commit simplifies the regexp and fixes it.
Having an ant pattern starting with "**" was a _very_ bad idea. It makes the plugin search way too many places, including even its own SCM workspace. Which then leads to recursively including ever deeper SCM workspace hierarchies in the SCM workspace and in the repository. Also, such an ant pattern makes the plugin scan all jobs' workspaces, which can take a long time and may discover bogus matches. (They're by default at $JENKINS_HOME/workspace.) The Cloudbees Folders plugin has all jobs under $JENKINS_HOME/jobs, and possibly nested as in jobs/Folder1/jobs/SomeProject Both Jobs and Folders are Saveables; folders also have a config.xml. Source changes: * Introduce a new ConfigurationEntityMatcher that matches and traverses only exactly this jobs/XXX/jobs/YYY/jobs... hierarchy. * To avoid problems with user-defined inclusions starting with * or even **, exclude our own SCM working directory always in ant pattern matching. * Also exclude the war directory in $JENKINS_HOME. * Undo more changes from commit 141cc2b. Since we test new for AbstractItem, the tests should not mock TopLevelItem. Better mock Job. * Add two little test cases for config file matching for a job nested in a folder. * Add minimal test cases for page URL matching for jobs.
This plugin can only put things under $JENKINS_HOME into SCM. If workspaces are located elsewhere, it failed with an exception. Newly, we just ignore such files. In all likelihood, users won't want to sync workspace files anyway. The same goes for the builds directory. People who really might want to put stuff from there into SCM can try symlinking to their desired workspace/build roots from $JENKINS_HOME. Restoring from SCM may, however, then produce unexpected results. Should fix JENKINS-18401 and related issues such as JENKINS-13593 and JENKINS-19984. Includes two tests for buildPathRelativeToHudsonRoot(), and use org.junit.Assert.assertNotNull etc.instead of the hamcrest matchers.
Use Collections.emptyList() of ImmutableList.of().
With the new Jenkins baseline, we not only have hudson.*.xml but also jenkins.*.xml.
When the plugin is dynamically loaded into a running Jenkins, it would not properly initialize and thsu refuse to do anything until Jenkins was restarted. Let's initialize early, but don't do so in tests where we do manually call init() at appropriate times.
1. JENKINS-15218: Maven SCM 1.9.1 _does_ use `git rm -r` to remove a directory. (At least on a machine with git 1.8.3) So I was not able to reproduce that particular problem. 2. JENKINS-16378: The plugin mistakenly tried to copy the full job directory after a job rename. That's Github issue jenkinsci#8[1]. This may fail catastrophically if the build directory is in the job's directory and contains broken symlinks, for instance for new jobs or a job that never failed (lastFailed will point to a non-existing subdirectory named "-1") Even when it doesn't fail, it may end up putting way too many things into SCM, especially if the workspaces should also be located in the job directories. 3. JENKINS-24881 is a duplicate of JENKINS-16378. 4. Jenkins now does have a more general "move", the plugin has to account for that and use onLocationChanged instead of onRenamed. (Also in tests!) [1] jenkinsci#8 Changes: * Refactored rename handling to submit only the stuff that is selected by some strategy. * ConfigurationEntityMatcher: removed unused method getIncludes(), added a new method to match deleted paths. Added a FileSelector to matchingFilesFrom() to be able to restrict the traversal to specific subdirectories under $JENKINS_HOME. * ChangeSet: removed registerRenamedPath (handled by transaction now). * Transaction: change rename handling to include only matched stuff under the new directory * AbstractScmSyncStrategy: createInitializationSynchronizedFileset() changed to account for FileSelector * ScmSyncConfigurationPlugin: fix the need to restart Jenkins to get going, and provide new operations to collect files. Tests: * The tests were broken. Moved two tests that cannot possibly succeed on git but per chance can succeed on svn to the svn-specific tests. These tests "succeeded" in the test runs so far only because they never did anything--they faked a job as Item.class, which never triggered anything, neither before nor after commit 141cc2b. These tests modify the repository from outside the plugin-managed SCM workspace. In git, this will always require a pull. In svn, these two test happen to work because the outside modification is on different paths. It should fail if there was a path conflict, so these two tests are dubious anyway. * Also removed a spurious mocking of the available strategies that was unused.
With all these warning markers it's a bit hard to see what's going on.
Otherwise a mistakenly typed blank before or after may break things and leave the user puzzled about the reason. Also give that git repository URL a form validator. Yes, if this plugin is ever refactored to use git-plugin/git-client and Jenkins' proper Credential handling, this will have to change or be removed again, but I don't see this plugin being anywhere near that goal. The validation checks input for syntactic validity and also whether git actually can access the repository. It's still possible that pushing something to that repository fails later on (for instance, if Jenkins' OS user has no push permission), but at least this verifies the basic setup and gives the user a hint that user.name and user.email should be set, and that Jenkins' OS user may need to have a ssh key for repository access. I've included the validation in the plugin class itself and not in some git specific class because the corresponding jelly form is also the responsibility of the plugin class. Also some minor grammar things in help texts.
In fact; I had been trapped by Eclipse's call hierarchy: getDefaultIncludes() appeared unreferenced. In reality, it is called via jelly from the manaualSynchronizationIncludes help text.
Add a test case for a job name starting with a dash. Is ignored; tests will fail if it is enabled. (Bug in maven-scm: GitRemoveCommand does not use -- to separate options from file names.)
The problem here is that this maven SCM library omits -- for git rm. Factored out the nested class from a previous similar hack into its own file and added a new override for GitRemoveCommand. Enabled the test case for job name starting with a dash.
Add two more test cases for adding/deleting/renaming a job with blanks in the name. Ignored because of maven-scm bug SCM-772.[1] [1] https://issues.apache.org/jira/browse/SCM-772
The basic problem here is SCM-772.[1] maven-scm cannot parse the output from git status if it contains quoted/escaped file names. There's at least two pull requests, one attached to SCM-772 and one at [2] aimed at fixing this; both are erroneous and don't look like they'd go in anytime soon. (The first one would at least replace \r by \f, and the second one only strips quotes but doesn't de-escape.) So we fix this by providing our own commands in our own gitexe provider and making sure that our implementation of these commands can deal with quoted/escaped filenames. Enabled the two test cases for job names with blanks. Also, since we're rewriting part of maven-scm here anyway, I've also included a minimal and proper fix for SCM-695.[3] [1] https://issues.apache.org/jira/browse/SCM-772 [2] apache/maven-scm#26 [3] https://issues.apache.org/jira/browse/SCM-695
The latestCommitFuture is global, but was not protected against concurrent modifications. Code changes: * Make sure that adding the commit to the queue and queueing the new job on the executor happen synchronized. * Eliminate the global latestCommitFuture altogether. It's good enough to wait for the current transactions' commit future.
Same problem as in maven-scm's SCM-695: to check whether one path refers to a parent directory of another path, it behooves one well to handle file separators, otherwise a directory path "foo/bar" may be seen incorrectly as a parent to be stripped from the file path "foo/barbar/someFile.txt".
|
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'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. WDYT ? |
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
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:
As for "young blood": I'll take that as a compliment. I did program on VAXen, though :-)). |
|
You're right about enhancements. I captured my vision about some of your points in JENKINS-18129 and JENKINS-18124 |
* ScmSyncGitCheckInCommand: typo in a code path that is never taken in this plugin (it always calls checkIn with only a directory, no files). * Commit: check for user != null. Is that null check needed at all? * FixedGitStatusConsumer: use simpler & more efficient File.replace() instead of File.replaceAll(). File.separator is guaranteed to be the single character File.separatorChar per its contract.
No real code changes. Try to get a consistent indentation on all files touched in PR jenkinsci#31. Looks like these files used 4 spaces for indentation originally, but my Eclipse was set to use tabs. Fixed by IDE settings to use spaces instead of tabs and reformatted all Java files I touched. This also added some missing @OverRide annotations, and made some single-line if-statements use blocks -- I *never* use the short form.
In fact, this override without the @test annotation effectively switched off this test for git. It's re-enabled now.
| if (matcher.match(pattern, pathRelativeToRoot)) { | ||
| return true; | ||
| } else if (isDirectory) { | ||
| // pathRelativeFromRoot is be a directory, and the pattern end in a file name. In this case, we must claim a match. |
There was a problem hiding this comment.
Can you drop an eye on this please: https://github.com/jenkinsci/scm-sync-configuration-plugin/pull/28/files#diff-8ea250ea1ccf94b3ce908b8029599565R26
In case jobs directory is a link to a mounted volume so nextBuildNumber stays in sync with buildHistory and symlinks we don't want the scm-sync-configuration plugin to throw exceptions.
Such cases are usual when running Jenkins in Docker as a Phoenix Docker Container or simply when rehydrating the build history an keeping the nextBuildNumber in sync with rehydrated build history.
There was a problem hiding this comment.
Hi. I must say I don't quite understand the change you've referenced: I would have thought that getAbsolutePath() was a pure path operation. If we used getCanonicalPath() then that test might indeed detect symlinks, but I doubt it does with getAbsolutePath().
But anyway, check line 20 in JenkinsFilesHelper and the calls to this matches() operation here. I think that's the same test you did, just in a different place.
Also, with these changes the plugin will not synchronize build history or nextBuildNumber by default. A user would have to include those explicitly. Maybe you could explain better over at PR #28 what exactly the setup is and what exceptions were logged, and what the plugin tried to synchronize. (Note that the plugin 0.0.8 had a bug (JENKINS-16378, or issue #8 here) that mistakenly started synchronizing everything in a job's directory when a job was renamed. That should be fixed here with commit 28d4aee2 in this PR.) Finally, PR #28 appears to be based on grzegorz7's work on branch specification, which greatly confuses matters.
But I see that something went wrong with indentation. Probably my Eclipse used tabs to indent. I'll have to fix that so that the diff gets more readable.
There was a problem hiding this comment.
I would like to see your PR merged then I can replay the scenario I mentioned and see whether it is still the case. It may be that the problem is not there anymore once your PR gets merged. The functionality is there and everything worked, there was just an odd log on one of the exhaustive pattern matchers, which, you may have removed now altogether.
I will go with your PR!
|
👍 great stuff @tomaswolf |
| Liste de chemin \u00e0 la Ant, permettant de sp\u00e9cifier des fichiers suppl\u00e9mentaires qui seront synchronis\u00e9s avec le repository | ||
| Includes\ should\ be\ case\ sensitive\ paths\ starting\ from\ your\ JENKINS_HOME\ directory,\ without\ /\ in\ the\ beginning;\ Use\ of\ wildcards=\ | ||
| Les chemins doivent \u00eatre sensibles \u00e0 la casse, bas\u00e9s depuis votre r\u00e9pertoire JENKINS_HOME, et ne d\u00e9marrant pas par un /; L''utilisation des wildcards | ||
| List\ of\ ant-like\ includes\ allowing\ to\ specify\ additional\ files\ to\ synchronize\ with\ the\ repository=Liste de chemin \u00E0 la Ant, permettant de sp\u00E9cifier des fichiers suppl\u00E9mentaires qui seront synchronis\u00E9s avec le repository |
There was a problem hiding this comment.
The only change here is actually this key, which needs to match the new English text in manualSynchronizationIncludes.jelly above.
I hadn't noticed the other changes; looks like eclipse automatically reformatted the properties upon saving. Sorry about that.
D'ailleurs, cela ne devrait pas être "Liste de chemins ..."?
|
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? |
|
Checking out the version in this pull request, is it the correct behavior for the plugin to perform a |
|
@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. |
|
+1 |
|
👍 |
|
+1 |
|
Any progress on getting this merged? Good stuff here! |
|
+1 |
|
Why has this not been merged yet? I'm also running into this issue. |
|
Please merge this :( +1 |
|
+1 |
|
@tomaswolf: This is good stuff. I use SCM sync heavily, and have hit JENKINS-24686 before. |
|
@tomaswolf: fair enough. This plugin is too important, and you did a lot of good work, |
|
@tomaswolf: just to verify, are you using your patch to SCM Sync in a production environment? |
|
@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. |
|
I sent a request to add myself as a plugin maintainer so that I can help get your fixes in. |
|
@rodrigc thanks for taking the lead on this! |
|
Bumping because I'd love to hear when this is merged, 👍 |
|
+1 |
|
@rodrigc: I see that you were accepted as a maintainer. Do you have any updates regarding this PR? |
|
Christmas does come early. |
No real code changes. Try to get a consistent indentation on all files touched in PR #31. Looks like these files used 4 spaces for indentation originally, but my Eclipse was set to use tabs. Fixed by IDE settings to use spaces instead of tabs and reformatted all Java files I touched. This also added some missing @OverRide annotations, and made some single-line if-statements use blocks -- I *never* use the short form.
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.