Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[JENKINS-42724] - Restore the windows-service/jenkins.xml resource to restore compatibility with windows-slaves 1.2 #2803
Conversation
Windows Slaves plugin performs a direct access to the resources bundled into the core. Hence the file removal was a bad idea though I have not seen the issue in automatic tests and ATH. This change also was a last-minute change in #2765 in order to address suggestions from @daniel-beck, hence I didn't test it properly
reviewbybees
commented
Mar 13, 2017
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| In the case WinSW gets terminated and leaks the process, we want to abort | ||
| these runaway JAR processes on startup to prevent "Slave is already connected errors" (JENKINS-28492). | ||
| --> | ||
| <extensions> |
This comment has been minimized.
This comment has been minimized.
jglick
Mar 13, 2017
Member
So this is copied from jenkins.xml, and was not in the deleted jenkins-slave.xml, right?
This comment has been minimized.
This comment has been minimized.
oleg-nenashev
Mar 13, 2017
Author
Member
This is copied from https://github.com/jenkinsci/windows-slave-installer-module/blob/master/src/main/resources/org/jenkinsci/modules/windows_slave_installer/jenkins-slave.xml .
I cannot enable the automatic update easily due to the Copy-paste of the old stuff in Windows Slaves plugin, but at least I can pick the new sample for other options
| className="winsw.Plugins.RunawayProcessKiller.RunawayProcessKillerExtension" | ||
| id="killOnStartup"> | ||
| <pidfile>%BASE%\jenkins_agent.pid</pidfile> | ||
| <stopTimeout>5000</stopTimeout> |
This comment has been minimized.
This comment has been minimized.
|
@reviewbybees done |
|
Also created a workaround PR to the plugin: jenkinsci/windows-slaves-plugin#6 . With that change we may avoid the out-of-order release |
|
Should I merge it to formally close the regression? Likely yes, because there will be more people updating to 2.50+ without updating to Windows Slaves 1.3.1+ |
|
Isn't that just a matter of upgrading the bundled version? |
Well, maybe. I doubt there are any other usages of this file. And Windows Slaves Changelog does not contain potential fatal flaws. Only minor changes since the decoupling |
|
In that case I would favor that variant. |
oleg-nenashev
changed the title
oleg-nenashev
dismissed stale reviews from
abayer and jglick
The implementation has been reworked
|
@reviewbybees @daniel-beck I need another review iteration since I have changed the implementation after the release of 1.3.1 and feedback from @daniel-beck |
|
Assuming this fixes the issue for people who had an older version installed, i.e. it automatically is upgraded. Also, should be squashed. |
Will check it, but I have some concerns about that |
|
@daniel-beck Checked, it does not work OOTB. So I will restore the resource file |
oleg-nenashev
force-pushed the
oleg-nenashev:bug/JENKINS-42724-missing-jenkins-slave-xml
branch
from
41994c6
to
6428c94
|
@daniel-beck fixed |
|
As discussed, |
|
Note: I've just got an issue during the upgrade from 1.642. The restart should have fixed the issue, but still a potential case. As discussed with @daniel-beck , we agreed not to bump the dependency now since the detached plugin needs to be bumped accordingly in https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/ClassicPluginStrategy.java#L413...L427 . And @daniel-beck thinks it is not trivial in this case |
oleg-nenashev
changed the title
oleg-nenashev
changed the title
|
Merging to get it in the release |
oleg-nenashev commentedMar 13, 2017
•
edited
Background:
See JENKINS-42724 and JENKINS-42746.
No autotests, it should be somehow caught by PCT or ATH.
Full diff
Changelog entries
Proposed changelog entries:
windows-service/jenkins.xmlresource to restore the compatibility with Windows Agents Plugin (FKA Windows Slaves Plugin) 1.2 and belowSubmitter checklist
Desired reviewers
@daniel-beck @jenkinsci/code-reviewers @reviewbybees