Skip to content

Minor fixes for adding timeOutInSeconds to Clone and Deploy by MarcelMue #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pjdarton merged 5 commits into from Aug 4, 2017
Merged

Minor fixes for adding timeOutInSeconds to Clone and Deploy by MarcelMue #81

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95b0fbd
Minor fixes for adding timeOutInSeconds to Clone and Deploy by MarcelMue
Jul 28, 2017
e6c6be0
Merged up resolved conflicts
Jul 28, 2017
cafd43a
Changing tabs to spaces and int to Integer
Jul 28, 2017
a09b93b
More int related chagnes
Jul 28, 2017
b7625c2
Merge branch 'master' into master
pjdarton Aug 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/main/java/org/jenkinsci/plugins/vsphere/builders/Clone.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@

public class Clone extends VSphereBuildStep {

private final Integer TIMEOUT_DEFAULT = 60;
Copy link
Copy Markdown
Member

@pjdarton pjdarton Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, Integer should be int - this one isn't nullable.

(Sorry, I should have spotted this earlier)


private final String sourceName;
private final String clone;
private final boolean linkedClone;
Expand All @@ -51,12 +53,13 @@ public class Clone extends VSphereBuildStep {
private final String folder;
private final String customizationSpec;
private final boolean powerOn;
private final Integer timeoutInSeconds;
Copy link
Copy Markdown
Member

@pjdarton pjdarton Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@ghost ghost Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@pjdarton pjdarton Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

private String IP;

@DataBoundConstructor
public Clone(String sourceName, String clone, boolean linkedClone,
String resourcePool, String cluster, String datastore, String folder,
boolean powerOn, String customizationSpec) throws VSphereException {
boolean powerOn, Integer timeoutInSeconds, String customizationSpec) throws VSphereException {
this.sourceName = sourceName;
this.clone = clone;
this.linkedClone = linkedClone;
Expand All @@ -66,6 +69,12 @@ public Clone(String sourceName, String clone, boolean linkedClone,
this.folder=folder;
this.customizationSpec=customizationSpec;
this.powerOn=powerOn;
if (timeoutInSeconds != null) {
this.timeoutInSeconds = timeoutInSeconds;
}
else {
this.timeoutInSeconds = TIMEOUT_DEFAULT;
}
}

public String getSourceName() {
Expand Down Expand Up @@ -103,6 +112,10 @@ public String getCustomizationSpec() {
public boolean isPowerOn() {
return powerOn;
}

public Integer getTimeoutInSeconds() {
return timeoutInSeconds;
}

@Override
public void perform(@Nonnull Run<?, ?> run, @Nonnull FilePath filePath, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException {
Expand Down Expand Up @@ -168,8 +181,8 @@ private boolean cloneFromSource(final Run<?, ?> run, final Launcher launcher, fi
vsphere.cloneVm(expandedClone, expandedSource, linkedClone, expandedResourcePool, expandedCluster,
expandedDatastore, expandedFolder, powerOn, expandedCustomizationSpec, jLogger);
if (powerOn) {
VSphereLogger.vsLogger(jLogger,"Powering on VM...");
IP = vsphere.getIp(vsphere.getVmByName(expandedClone), 60);
VSphereLogger.vsLogger(jLogger, "Powering on VM \""+expandedClone+"\" for the next "+timeoutInSeconds+" seconds.");
IP = vsphere.getIp(vsphere.getVmByName(expandedClone), timeoutInSeconds);
}
VSphereLogger.vsLogger(jLogger, "\""+expandedClone+"\" successfully cloned " + (powerOn ? "and powered on" : "") + "!");

Expand Down
Loading