Stop hard-coding underscore in clone names #93
Conversation
* Underscores complicate things when you want to create DNS records for the ephemeral slaves. * The user can always add the underscore back in their config. This should be documented as a breaking functional change but I don't really see that people would be designing tons of functionality around this underscore. As a result, there is no migration from pre- to post- removal versions of the plugin.
Added plugin-upgrade code to template's readResolve method so that existing users upgrading to new version of the plugin keep the same clone names until they decide otherwise. Corrected Javadoc for CloudProvisioningAlgorithm's findUnusedName method. Corrected online help for template field cloneNamePrefix.
| // Earlier versions of the code may have stored configuration in a | ||
| // different way but in the same kind of fields, so we need an explicit | ||
| // versioning to know how to mutate the data. | ||
| if( configVersion<=0) { |
There was a problem hiding this comment.
nit: inconsistent parens (looks like this project generally goes for the standard-ish if (...) { with space after the if and space after the close paren.)
There was a problem hiding this comment.
I'm quite happy with nit-picking comments; I'll sort those out - I dislike messy/inconsistent code too.
| // In version one, the underscore was removed from the code so we | ||
| // have to put it in the data (the user is then free to change it to | ||
| // something else if they want to). | ||
| this.cloneNamePrefix = this.cloneNamePrefix + "_"; |
There was a problem hiding this comment.
Can this.cloneNamePrefix be empty going into this method, e.g. perhaps if there was no config object at all to begin with? Is readResolve() only called if there is an existing config, and if so, is cloneNamePrefix guaranteed to be non-null and non-empty?
There was a problem hiding this comment.
There are no guarantees.
- If it was empty then the slave node names would previously have been named "_someRandomId" and this code will ensure that this remains the case.
- If it was null then the slave node names would previously have been named "null_someRandomId" and this code will ensure that this remains the case.
i.e. The old behavior is maintained, but a bad config won't be magically made good.
In an ideal world, we'd have some sort of "post deserialization fix-up" at the cloud level that ensured that, within the vSphereCloud, the prefix was non-null, non-empty and unique for each template, but I'm not 100% sure where that code would have to go / get-called-from in order to make it work in all situations.
| + CURRENT_CONFIG_VERSION | ||
| + ". Either CURRENT_CONFIG_VERSION is incorrect or the readResolve method is not setting configVersion when it upgrades the data."); | ||
| } | ||
| if( configVersion > CURRENT_CONFIG_VERSION ) { |
| @@ -1,6 +1,6 @@ | |||
| <div> | |||
| Controls the name by which the virtual machine will be known to vSphere. | |||
| The plug-in will append an underscore and a unique ID to this prefix in order to generate unique names.<p> | |||
| The plug-in will append a unique ID to this prefix in order to generate unique names.<p> | |||
There was a problem hiding this comment.
HTML nit (maybe applies throughout) -- do this
<p>stuff</p>
<p>more stuff</p>
There was a problem hiding this comment.
Personally, I'm not much of a fan of putting formatting/layout elements at the start/end of lines that contain content. I prefer to keep layout and content on their own lines. e.g.
<p>
stuff
</p>
<p>
more stuff
</p>However, I did try wrapping the first line of text (line 2 in the file) in <p> ... </p> and found that this then added an unsightly amount of vertical whitespace in the help text (between the top border of the help box and the top of the first line of text).
There's a border around the existing help-text that's about as wide & tall as an "M" character, and the word "Controls" at the start and the "(from vSphere Plugin)" text at the end are both indented by equal amounts.
If we make the initial text a paragraph then the layout becomes unbalanced and there's "two Ms" worth of vertical whitespace before the first word "Controls".
i.e. it might be "more correct" HTML but it's also more ugly.
I suspect that the HTML generated by Jenkins to wrap the help text was designed more for "simple text" than HTML containing lots of layout formatting. I suspect that it was designed to make things "look right" if they start and end with no vertical whitespace.
I'm not an HTML expert, so I did mvn hpi:run and did some experimentation with the layout ... and I found that if we remove <p> and </p> completely and instead separate the initial text (lines 2 and 3 in this file) from the "Note" (line 4) by <br><br> then that looks good too (in my opinion), and it also uses half an M less vertical whitespace between the "Note" text and the "(from vSphere Plugin)" text, which appeals to my sense of neatness.
...also (a brief google later), doing <p style="margin-top:0;">stuff</p> for the first makes it look acceptable when rendered too, but I suspect that messing with styling here would be considered worse than just using <br><br> would be.
I'm open to ideas but unless we can settle on something that's both "good HTML practice" and also "looks correct when rendered" then I think it may be best to leave that code as-is.
|
@joshk0 A few days ago you said you were going to test this ... What's the verdict? |
|
@joshk0 Nudge. |
|
I'm sorry Peter, I wasn't able to test this. However I really don't see any downside, I was happy with it as is and you've made it even safer and conservative so i feel like I would be happy to see this in a release. Thanks for following up. |
|
@joshk0 FYI release 2.18 was uploaded a few minutes ago; it should get indexed in the next 12 hours or so and thus appear as an available update in your Jenkins server(s) within 48 hours. |
#92 made changes to the functionality to remove the hard-coded underscore between a template's cloneNamePrefix and the random ID in the slave node names.
However, these changes didn't update the online help text, the javadoc, or handle the upgrade path for existing users (who would "suddenly" find their clone names were lacking the underscore after an upgrade).
This PR incorporates #92 but also includes changes to address the above.