buildStep Clone: expose all options of cloneOrDeployVm() method, allow to use namedSnapshot and extraConfigParameters
#137
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
VSphere.cloneOrDeployVm()method is wrapped bycloneVm()(used for aClonestep) anddeployVm()(used for aDeploystep).These wrappers set up null-ness of
namedSnapshotstring andextraConfigParametersmap, and fiddle with false or true value ofuseCurrentSnapshot; however thebuilders/...step classes seem to be more considerably different by logic and purpose.The "Clone" related classes/methods seem to hard-code use of a snapshot (and "Deploy" classes/methods conversely seem to use the current state of the VM).
This PR extends the exposed "Clone" buildStep class (of
vSpherestep) with an ability for the caller to optionally provide thenamedSnapshotand the other two fields to fully customize the created clone. Caused by a practical need to spawn some VMs from an older snapshot of the template :)The expectation is that by default it would behave as it did before.
SIDE NOTE:
mvn packageseems to fail with Java 17 during test initialization, but works with JDK of Java 8. Also happens on master branch, and per https://stackoverflow.com/questions/68974753/unable-to-make-field-private-final-java-util-comparator-java-util-treemap-compar seems to be due to Cucumber evolution.For "legacy"/freestyle jobs, I did extend jelly markup partially, to include the new fields, but would welcome further iterations from someone more knowledgeable about it to:
extraConfigParametersmap (dynamic table of two columns for adding the key/value pairs?);linkedCloneoption vs. presence or absence of EITHER ONE OR NONE ofnamedSnapshotoruseCurrentSnapshotsettings (they are still optional, althoughuseCurrentSnapshotis implied for linked clones if a named snapshot is not requested).useCurrentSnapshotcheckbox especially iflinkedCloneis not set (default) so the "Clone" step behaves as it did before - unless told otherwise. Is there a tri-state checkbox to facilitate a Boolean null value?Looking at earlier PR reviews like #103 I am also concerned if it is a good idea to change the
Cloneclass constructor, or to keep it as it was and introduce non-final properties and@DataBoundSettermethods instead? Got no opinion on that. All I can say is nothing blew up in the limited local dev-testing :)Testing done
Did not test "legacy"/freestyle jobs much - just the appearance of the fields for the interactive build step setup in Web-UI. Could not really test the sanity-checker nor the resulting job, it complained a lot about the "cluster" and "resource pool" to use, something it auto-detects with a pipeline step. Also not sure if it did consider the added field checks at all, did not complain about any combos. The original code seems broken, e.g. asks for a
serverNamebut does not set one in jelly...For practical purposes, on a fresh Jenkins added this plugin (custom build) and created the credential for vCenter, a cloud, and a scripted pipeline to play with, similar to the following:
...and fiddled with parameters for the latter part.
namedSnapshotnoruseCurrentSnapshotis specified, it uses the newest snapshot (as expected per legacy behavior) for "deep" or "shallow" clones (depending onlinkedClonepresence/value):useCurrentSnapshot=falseis explicit (and nonamedSnapshotis passed), it seems to copy the current state of the (template) source VM (UPDATE: Also tested successfully with a source VM that had no snapshots):useCurrentSnapshot=trueis explicit and anamedSnapshotis passed, it fails (as expected):useCurrentSnapshot=falseandlinkedClone=true(and nonamedSnapshotis passed), it fails (as expected):