Minor fixes for adding timeOutInSeconds to Clone and Deploy by MarcelMue#81
Conversation
|
This is a clone of #65 with feedback addressed |
| public Clone(String sourceName, String clone, boolean linkedClone, | ||
| String resourcePool, String cluster, String datastore, String folder, | ||
| boolean powerOn, String customizationSpec) throws VSphereException { | ||
| boolean powerOn, int timeoutInSeconds, String customizationSpec) throws VSphereException { |
There was a problem hiding this comment.
"int" should be "Integer" in order to cope with old configuration data where this parameter did not exist.
|
|
||
| public class Clone extends VSphereBuildStep { | ||
|
|
||
| private final int TIMEOUT_DEFAULT = 60; |
There was a problem hiding this comment.
Indentation incorrect - use spaces, not tabs (tabs are evil - they never display the same as some people set them to 4, some 2, some 8 etc).
There's indentation issues lower down too - check your editor's settings and ensure that the file is free of tabs (and then check the indentation again) before re-uploading.
| this.customizationSpec=customizationSpec; | ||
| this.powerOn=powerOn; | ||
| if (timeoutInSeconds != null) { | ||
| this.timeoutInSeconds = timeoutInSeconds; |
There was a problem hiding this comment.
Once the timeoutInSeconds is an Integer, you should use ... = timeoutInSeconds.intValue(); to explicitly un-box the value.
|
|
||
| public class Clone extends VSphereBuildStep { | ||
|
|
||
| private final Integer TIMEOUT_DEFAULT = 60; |
There was a problem hiding this comment.
In this case, Integer should be int - this one isn't nullable.
(Sorry, I should have spotted this earlier)
| private final String folder; | ||
| private final String customizationSpec; | ||
| private final boolean powerOn; | ||
| private final Integer timeoutInSeconds; |
There was a problem hiding this comment.
I think that, unless we want to store a null/empty timeoutInSeconds field, this should be "int".
Pros/Cons...
If we have this field as an "int" then, if the user opens and presses "Save" on the config without setting the value, this field will be added with the default value.
If we have this field as an "Integer" then the field can be persisted as having no value set, meaning that the persisted data is basically saying "we want the default timeout to be used, whatever that is". In this case, the code should be modified to do the "if this is null, use the default value" at execution-time instead of at construction time.
Personally I think it'd be simpler to have this as an int, and have the get method also return an int.
(if you want it as an Integer then you'll have to delay the "if null..." stuff until the perform(...) method.
There was a problem hiding this comment.
Yeah sorry, I'm pretty new to all this code so bear with me a bit.
I like the idea of keeping everything except the constructor as int so I'll do that.
There was a problem hiding this comment.
I'm only just one step ahead of you at present - it's a "learning experience" for both of us :-)
...unfortunately, this means that I'm sometimes pointing you in the wrong direction :-(
I've just been reading https://wiki.jenkins.io/display/JENKINS/Hint+on+retaining+backward+compatibility and that's reminded me that, when Jenkins loads the configuration off disk, it doesn't actually call the constructor at all.
This means that we can't rely on the constructor setting the field to a value before perform() gets run because, where a user has an old job (i.e. no timeoutInSeconds field), and their job runs with the new plugin code (which is looking for a timeoutInSeconds field), then perform() will find that field is set to zero.
So I think that we need to decide what we want the default to be (should it wait forever, or one minute, or what? e.g. In my experience, one minute isn't enough for a Windows VM!), and then defer deciding what to until within the perform() method (which isn't what I'd do if I could rely on the constructor being called!).
I'm not familiar with what this code did before there was a timeout - did it wait "forever"? If so, I'd suggest that this is what the default behavior should be as, that way, existing customers will see no difference, but can decide to take advantage of the new flexibility if they choose to.
(I do apologize - I was expecting this part to be way simpler than it's turning out to be!)
1 participant
I ported the changes created by MarcelMue into a new PR with the code review feedback addressed.