Jnlp #117
Jnlp #117
Conversation
|
@clockworksoul, @jgangemi, @bparees: We discussed about those changes in #116 So I thought that you may want to have a look or try it out. |
|
@carlossg: its ready for review now |
README.md
Outdated
| and will be the container acting as Jenkins agent. It can ve overridden by defining a container with the same name. | ||
| and will be the container acting as Jenkins agent. | ||
|
|
||
| It can ve overridden by defining a container with the same name, or by explicitly: |
| @@ -396,7 +409,14 @@ private Pod getPodTemplate(KubernetesSlave slave, Label label) { | |||
| volumes.add(new VolumeBuilder().withName(WORKSPACE_VOLUME_NAME).withNewEmptyDir("").build()); | |||
| Map<String, Container> containers = new HashMap<>(); | |||
|
|
|||
| if (template.isCustomJnlpContainerEnabled() && template.getJnlpContainer() != null) { | |||
There was a problem hiding this comment.
this config variable feels misnamed to me.
the behavior is "when true, automatically add a default jnlp container"
so i would expect a name more like either:
- IsDefaultJNLPContainerEnabled
- IsAddJNLPContainerEnabled
or flip the boolean because
IsCustomJNLPContainerEnabled, to me, means "the user has promised to provide the jnlp container in one way or another, so we don't need to do anything"
There was a problem hiding this comment.
getJnlpContainer() doesn't return the default jnlp container but the user specified one or if you prefer the "custom". I can see that being confusing and I'll rename it to customJnlpContainer.
There was a problem hiding this comment.
hm. So is there any way for me to avoid having a jnlp (custom or otherwise) added to my pod? is setting the container name to jnlp the only way?
There was a problem hiding this comment.
No, the pull request does not provide a way to do so. It provides a more clear way to define the which container will act as a jnlp container and also adds the inheritance thing, but no way to explicitly request no jnlp container at all.
Even, if we add an option for that. I think that it wouldn't work, because you can't have a slave pod without a jnlp container.
If you are using a hybrid container (e.g. build tools + jnlp), it is still a jnlp container, so it could be defined in the jnlp container field.
Having no jnlp container at all could possibly make sense only in the case where you already had a slave pod running and for some reason you needed an additional container to execute some steps in the pipeline. Of course, this is still not supported (yet) by the plugin (because we would need to have a way to share the workspace, #114 might help in that regard).
Now, if you still feel there is a demand for such option, I don't mind adding it as part of this pull request.
There was a problem hiding this comment.
Even, if we add an option for that. I think that it wouldn't work, because you can't have a slave pod without a jnlp container.
sure, i just was thinking of the case where i don't want to call my container "jnlp" but it does indeed provide the jnlp client.
but it's not a big deal, if naming the container jnlp is the accepted mechanism i think we can live with it.
There was a problem hiding this comment.
i would request that you add a help item to the container name field that includes this information.
| The name of the pod template to use for providing default values. | ||
| When a value is specified and there is a pod template with a matching name, the template will be | ||
| used as a parent to all other pod templates. | ||
| </div> |
|
They are unnecessary, the feedback we get however, points to the fact that the current approach is not obvious. So, I think that we need to take that into consideration. |
|
@carlossg: rebased and fixed some typos. So what are we going to do with that? I think that its more obvious to ppl to have an explicit field to set jnlp, but I don't have very strong preference. |
|
I have added #132 that will solve the problem for people upgrading from 0.8 and earlier. The only container would be called For people already in 0.9 and 0.10 I'm going to assume they already went through the trouble of changing the container names. I don't see an obvious way to detect whether a template includes the jnlp container twice or not. The other code about having a default template looks good to me and could be useful anyway |
No description provided.