On 17.09.2015, at 13:19, Thorsten Möllers <[email protected]> wrote:
> if anyone would care to review the plugin for obvious flaws, I would be > happy. Some of the following (especially the one on variable naming) is probably subjective... - I don't think there's a reason for you to subclass Plugin. It doesn't really do anything here. You could move the static stuff into the listener directly. - You're missing the resource file that contains the plugin description for the plugin manager (src/main/resources/index.jelly). - Don't reinvent logging levels (especially if they cannot be changed once compiled), use java.util.logging consistently. - Don't write in the build log of jobs that you're not enabled for. That can break things like log parsing etc., as well as look weird. AFAIU this is added the moment another (upstream) job is aborted. - Don't prefix instance variables with 'i'. - I doubt you really need the RunData, why not query all running jobs for what their (upstream) causes are when you get notified about a run abort? Not sure what's saved/loaded in the Plugin class, but I expect the current design with the static HashMap doesn't handle a Jenkins restart well (the queue is persisted and restored on restart). - Don't rely on the JENKINS_URL env var to be set. - Given how many env vars you check, I think at least ENABLE_HIERARCHY_KILLER can be replaced by a job config option (e.g. a JobProperty). This would also have the advantage that the plugin would have some UI, and could have an inline help text explaining how it works. A non-library plugin without any UI is not a great idea as admins may easily forget it exists, and users don't even know it's there. - It likely doesn't work with Workflow plugin, whose Runs aren't AbstractBuilds. Build Flow plugin probably has the same problem. - While I understand that Parameterized Trigger allows having an upstream project that still runs, I wonder doubt it's always only one, which seems to be the assumption in your code. While that feature may not consider quiet periods, the node may be missing, so two identically parameterized builds could be collapsed in the queue while waiting for the node to free up. Those remarks out of the way: When I first saw the plugin name, I thought of the process tree killing feature of Jenkins (https://wiki.jenkins-ci.org/display/JENKINS/ProcessTreeKiller) -- as in 'process hierarchy'. Wouldn't something like downstream-killer or abort-related-jobs be better? -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/31EF4B0F-947B-40E8-AD7E-2AD2877B06F9%40beckweb.net. For more options, visit https://groups.google.com/d/optout.
