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

Already on GitHub? Sign in to your account

Change primary template identification to be its name, not its AMI. #35

Merged
merged 1 commit into from Feb 12, 2013

Conversation

Projects
None yet
3 participants
Contributor

kpfleming commented Feb 12, 2013

Many useful configurations will have multiple slave templates that use
the same AMI (but differ in other aspects). This patch changes various
parts of the code to properly identify templates by their names (in the
'description' field) instead of their AMI. As an example, the manual slave
provisioning button, before this patch, would allow the user to select a
template from a drop-down list, but then launched the slave using the
template's AMI, which could have resulted in the wrong template being
used for the launch. This patch also changes the global config page so
that the 'description' field is the first field in each template
definition, to emphasize its importance.

Addresses [JENKINS-7960] and [JENKINS-15158].

@kpfleming kpfleming Change primary template identification to be its name, not its AMI.
Many useful configurations will have multiple slave templates that use
the same AMI (but differ in other aspects). This patch changes various
parts of the code to properly identify templates by their names (in the
'description' field) instead of their AMI. As an example, the manual slave
provisioning button, before this patch, would allow the user to select a
template from a drop-down list, but then launched the slave using the
template's AMI, which could have resulted in the wrong template being
used for the launch. This patch also changes the global config page so
that the 'description' field is the first field in each template
definition, to emphasize its importance.

Addresses [JENKINS-7960] and [JENKINS-15158].
803dfea

Jenkins » ec2-plugin #49 SUCCESS
This pull request looks good
(what's this?)

Member

francisu commented Feb 12, 2013

Hey Kevin, I had a look at this, but I'm concerned about compatibility with older versions of the plugin. There is a mechanism that can fix up the configuration when something like this changes. It's called from PluginImpl and you can see an example in InstanceTypeConverter. Can you look into this a bit?

Member

francisu commented Feb 12, 2013

This is a case where it might be helpful to have a version number (just a simple number) associated with the configuration which is incremented on any incompatible change so that it's easy to determine when converters need to be run. See the other outstanding pull request for an example of why this is required. If you wanted to propose and implement a mechanism like that, it would be generally helpful.

Contributor

kpfleming commented Feb 12, 2013

I don't believe this patch changes the persisted configuration at all.

Member

francisu commented Feb 12, 2013

Yes, sorry, you are right, thanks for your contribution. I have been away from the code for a while, so forgive my lapses.

@francisu francisu added a commit that referenced this pull request Feb 12, 2013

@francisu francisu Merge pull request #35 from kpfleming/provision-by-template
Change primary template identification to be its name, not its AMI.
bd975b4

@francisu francisu merged commit bd975b4 into jenkinsci:master Feb 12, 2013

@kpfleming kpfleming deleted the kpfleming:provision-by-template branch Feb 12, 2013

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