optionally allow image resolution by name #59
Conversation
this enables a use case where one would continuously update the image and re-create it as a new snapshot, causing the id to constantly change but the name to be persistent. since ordinarily name is a really poor way to resolve images, in particular also because we always need to talk to the api to get the actual image, this entire feature is limited to user images rather than all images. specifically this also lessens the API work we need to do. the template config has a new checkbox to enable this feature, which then prompts a reload of the image combobox to only include user images (and at the same time the values in the model are changed from slugOrIds to names) at provisioning time we then need to pass a bit more context along so as to resolve accordingly in newImage()
| */ | ||
| @DataBoundConstructor | ||
| public SlaveTemplate(String name, String imageId, String sizeId, String regionId, String username, String workspacePath, | ||
| Integer sshPort, Boolean setupPrivateNetworking, String idleTerminationInMinutes, String numExecutors, String labelString, | ||
| Boolean labellessJobsAllowed, String instanceCap, Boolean installMonitoring, String tags, | ||
| String userData, String initScript) { | ||
| String userData, String initScript, Boolean imageByName) { |
There was a problem hiding this comment.
So I'm doing a quick pass on my phone. Is this actually used in the constructor or just setting?
I think changing the constructor can break loading older configs. I'll have to check. I think in general a @DataBoundSetter is safer. It might only matter if your removing fields.
I'll do a trial run with existing config and jcasc.
There was a problem hiding this comment.
I hadn't given it much thought TBH. Having a constructor that accepts all but one member feels a bit iffy though, so does having imageByName not be final when imageId is. That said, I'm not a fan of ctors with a million arguments of none-descriptive type anyway ^^
So, I drop the ctor argument and replace it with setImageByName(Boolean imageByName)?
|
I really like this. I need to review the code but love the idea. I mostly selfishly want to make sure nothing brwks my installs |
|
any chance to get this merged?? |
this enables a use case where one would continuously update the image
and re-create it as a new snapshot, causing the id to constantly change
but the name to be persistent. since ordinarily name is a really poor
way to resolve images, in particular also because we always need to talk
to the api to get the actual image, this entire feature is limited to
user images rather than all images. specifically this also lessens the
API work we need to do.
the template config has a new checkbox to enable this feature, which
then prompts a reload of the image combobox to only include user images
(and at the same time the values in the model are changed from slugOrIds
to names)
at provisioning time we then need to pass a bit more context along so as
to resolve accordingly in newImage()