Fixed JENKINS-36952 and added timeOutInSeconds to Clone and Deploy#65
Fixed JENKINS-36952 and added timeOutInSeconds to Clone and Deploy#65MarcelMue wants to merge 3 commits intojenkinsci:masterfrom MarcelMue:master
Conversation
|
Is there a chance that this can be merged? @jswager |
elordahl
left a comment
elordahl
left a comment
There was a problem hiding this comment.
Can you fix the formatting issues?
| private final String folder; | ||
| private final String customizationSpec; | ||
| private final boolean powerOn; | ||
| private Integer timeoutInSeconds = new Integer(TIMEOUT_DEFAULT); |
There was a problem hiding this comment.
• Why don't you declare this as final and set it in the constructor if null?
• Why are you using an Integer over a primitive int? Are you giving the user a field to provide an override?
|
Can this be fixed and merged please? The plugin is extremely broken in the pipeline with hard-coded timeout and the exceptions it causes. This PR would fix all of it, but it's been sitting here stale for 6 months. If formatting for adding timeout is the issue, I created another pull request that will at least fix exceptions in Jenkins pipeline. #79. If this gets merged first feel free to close that PR. |
|
@phucyall FYI at present this plugin is in a state of statis because the sole maintainer who has write access (which is required to merge a PR) is no longer able to spend time on this. There are efforts being made (see #76 for discussion) to transfer access so we can get things moving again. Note: at present this PR isn't in a good state (the issues raised above have not been dealt with) so it isn't presently in a state where I'd merge it even if I did have write-access and I expect that Eric would be like minded. However, you could help... |
|
Wonderful. I did already create my own PR with just the fix for the exception (there is a workaround that doesn't require updated timeout). Looking forward to my own build of the plugin 😃 |
|
Just as a heads up: I am currently unable to implement any changes as I no longer have a setup to test on. The feedback from Eric came so much later, that I had given up on the hopes of ever getting something merged. My workaround was to simply switch away from the plugin in the end. |
|
If the development on this plugin picks up again (at least the merging starts happening) I would gladly re-do your changes with the feedback addressed. It would for sure be nice to be able to set the timeout right in the clone task. That's more of an enhancement though. The real bug was lack of null check before setting "VSPHERE_IP" env. variable. I already made a separate PR for that fix |
|
@MarcelMue That's pretty much the situation that Jason (currently the only person with write-access) is in - he no longer has a setup to test on. Also, the notifications (to Jason) about activity on the plugin stopped getting through, so he had no idea everyone was waiting for him. He is now aware ;-) @phucyall I'd suggest that you "subscribe" to the issues that I raised that aren't merged yet (71 to 78). If you see a series of mergers happening there, that's your queue to get coding and submit a tidier PR that we can merge before the flurry of activity ceases ;-) |
ghost
mentioned this pull request
|
As mentioned in #76, I now have permission to process pull requests on this plugin. As a result, I've been reviewing lots of outstanding pull requests. I've not merged this one because the concerned raised earlier hadn't been addressed. |
3 participants
All changes were done for pipeline support only.