Skip to content

Conversation

@segator
Copy link

@segator segator commented Sep 18, 2016

I added the ${JNLP_SECRET} to be used by the cloud template, this is very usefull if you want to execute jenkins slave on windows machine auto deployed and you are using secured slave connections.

And also The UUID generated to have random VM name I reduced using base MAX_RADIX, same random security but shorter name, this is because maybe you want to use the vmname to create directories, and in windows we have a limit of chars for the path

segator added 3 commits September 18, 2016 17:15
allow start in interactive mode  slave in windows machine on Guest Start
This change cause the UUID is shorter and you work with shorter VM names
this change cause vm name are shorter, and have same posibilities of
random
Copy link
Owner

@pjdarton pjdarton left a comment

Choose a reason for hiding this comment

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

OK, I see what you're doing here.
I can't "accept" the changes because (a) you've asked me to accept your changes into my own fork, rather than into the actual main code, (b) you're asking me to merge it into my "master" and not a branch, (c) I'm not sure how to accept your changes into a branch because I don't fully understand git and (d) there's a lot of formatting changes that drown out the actual change.
However, I get the idea and I'll see what I can do to make this happen.
See comments in https://issues.jenkins-ci.org/browse/JENKINS-20743

@segator
Copy link
Author

segator commented Sep 19, 2016

Well I pulled the request to your main branch because it's releated with your actual pull request on the main jenkins branch, you added this new feature of "guest properties" but it's not integrated yet to the main branch.

So I expected you accept my pull request and add this changes to your actual pull request.

I have planed to pull and other request to you, I added this "guest properties" new feature to the build step vsphere clases clone and deploy.

And I want to tell you, thank you for your work, It's just what I needed!!

@pjdarton
Copy link
Owner

The pull request which adds the GuestInfo functionality (which your
JNLP_SECRET pull request adds to) was
jenkinsci#49 (and
jenkinsci#53) and that's
already merged in. That's why it was easy for me to create a pull request
against the main code with just that enhancement.

I do have a larger enhancement awaiting merge (
https://issues.jenkins-ci.org/browse/JENKINS-38269), but the guestinfo
functionality is already there. Unfortunately, while I do have code which
will shorten the slave names a lot "ready to go", I want to wait until that
larger enhancement is merged before I issue another pull request, otherwise
it will cause merge hell.

As for future work regarding guestinfo - I think that it would be nice to
make that functionality available for static slaves as well as for the
dynamic cloud slaves, and to include that functionality in the Launcher
code, but that's functionality I don't need at work right now so I suspect
that I'm not going to get to do that unless I work on it in my spare time
(and I prefer to spend my free time doing other things :-)

I agree that it would also be useful as a build step - hopefully the way I
wrote the code will make that fairly easy to implement.
I would suggest that you create a JIRA issue for the work you want done,
then put your changes on a branch in your own github fork from the main
repository (not mine! :-) and once you're happy, issue a pull request for
your branch to request that the plugin maintainers merge your changes into
the master branch of the plugin, and mark your issue as "being reviewed".
That's all I'm doing - I don't own the plugin, I'm merely issuing pull
requests, mentioning those pull requests in the JIRA issues (and mentioning
the JIRA issues in the pull requests) and then Jason (one of the two
maintainers listed on the plugin's wiki page) is helpfully merging them in.

Regards,

Peter

On 19 September 2016 at 18:58, segator [email protected] wrote:

Well I pulled the request to your main branch because it's releated with
your actual pull request on the main jenkins branch, you added this new
feature of "guest properties" but it's not integrated yet to the main
branch.

So I expected you accept my pull request and add this changes to your
actual pull request.

I have planed to pull and other request to you, I added this "guest
properties" new feature to the build step vsphere clases clone and deploy.

And I want to tell you, thank you for your work, It's just what I needed!!


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA8F2WzrbZHEOJC6FyAmU-xmiHzWlzaVks5qrs1jgaJpZM4J_7Tv
.

Copy link
Owner

@pjdarton pjdarton left a comment

Choose a reason for hiding this comment

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

A UUID, as used by the original code, is 128 bits of data. A single long, which is what you're using here, is only 64 bits, and not all of that will be random due to the use of toString.

However, as I've also battled Windows path length issue in the past, I understand the concept :-)
I've got code-changes locally that solve this "VM name too long" issue differently - once my other PR is merged, I'll be able to issue that one too.
The technique I've got locally is:
a) For templates which have an instance cap of N machines, I use names of prefix_1, prefix_2 ... prefix_N (and ensure that it doesn't use one that's currently in use). This reduces the length of the "unique" bit down from 36 characters (the length of UUID's toString()) to 1-3 characters.
b) For templates which have no cap, I use a UUID to provide the random number, and then render that 128-bit number using BigInteger's toString(MAX_RADIX). This drops the length from 36 characters to 26 characters.
If 26 characters is still too long then I guess we could use the (64bit long) milliseconds since 1970, which should drop it to around 13 characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants