[stable/Jenkins] Merge Master.SidecarContainers: into Master.Sidecars

[holmesb]

Re:

charts/stable/jenkins/values.yaml

Line 238 in 9084ce5

SidecarContainers:

I deliberately added Master.Sidecar in the JCasC PR rather than Master.JCasC.Sidecar, so that other sidecars could be placed here. Can't see any logic behind having both Master.SidecarContainers and Master.Sidecar. @torstenwalter , do you have any objection if I move your commented example to Master.Sidecar and then the configAutoReload one will be underneath it?

My PR referred to this: "Hierarchy of sidecar values allows further sidecars to be added for other purposes in future" (I accept this PR became a bit wordy at times!). For clarity, I'll also rename this key to Master.Sidecars.

[torstenwalter]

No objections

[torstenwalter]

Merging both is a good idea, but we need to pay attention that people know what they are configuring. e.g. .Values.Master.Sidecar.image becomes confusing once there are more sidecars.

This is how the yaml looks at the moment:

  Sidecar:
    image: shadwell/k8s-sidecar:0.0.2
    imagePullPolicy: IfNotPresent
    resources:
    configAutoReload:
      enabled: false
      sshTcpPort: 1044
      label: jenkins_config
      folder: /var/jenkins_home/casc_configs

and in addition the SidecarContainers property I introduced.

What about renaming "Sidecar" to "Sidecars" and moving the configAutoReload settings below that config key. SidecarContainers could become others. So the result would be:

  Sidecars:
    configAutoReload:
      enabled: false
      image: shadwell/k8s-sidecar:0.0.2
      imagePullPolicy: IfNotPresent
      resources:
      sshTcpPort: 1044
      label: jenkins_config
      folder: /var/jenkins_home/casc_configs
    others:

That way it becomes clear which sidecar you are configuring. I could also add another one (in a separate PR) for the GitHub webhook proxy. e.g:

  Sidecars:
    configAutoReload:
      ...
    webhookproxy:
    ....
    others:

others would still provide people with the option to configure whatever other sidecar containers they would like to have,

Issue with renaming would be that it's a breaking change. No clue how you would deal with that in this chart.

[holmesb]

All sounds good Torsten. Yes image, imagePullPolicy, & resources belong under configAutoReload. Also agreed leaving Sidecar and not renaming it to Sidecars will be less disruptive.

[torstenwalter]

Just seen my mistake. I forgot to rename Sidecar to Sidcars. Corrected that now. I think that's clearer and we face backward incompatibilities anyhow when moving the other properties.

@holmesb Do you want to create the PR or should I? Just want to avoid double work while both of us start working on this independently.

@maorfr Any opinions how to deal with these breaking changes?

[maorfr]

These are new features (as you know), and its probably better to do now rather then later.

I say break away ;)

[torstenwalter]

PR is ready. @holmesb would be great if you could test your part and provide your feedback.
Thanks for pointing out that we ended up with different sidecar config fields.

[holmesb]

LGTM @torstenwalter, nice work. No problems according to my testing. Thanks for this.