Support initContainers and {image, tag} and {image: {repository, tag}} Helm values #1258
Conversation
To wit:
- update initContainers as well as containers
- read and write {image, tag} format in FluxHelmRelease values
|
Helm charts typically use: Are you intentionally using a different format than is commonly used? See #1226 for an example |
Argh, no, I think I just made it up in my head :-( |
- read and write to initContainers as well as containers in manifests, so we can update them; - report on the initContainers of Kubernetes resources
Some charts use an encoding for images of
image: repo/name
tag: tag
rather than putting the whole image ref in one field. Support that
(after kubeyaml) by interpreting the values fields, or its values,
depending on whether a "tag" entry is present.
squaremo
force-pushed
the
enhance/new-kubeyaml
branch
from
6c224aa to
8a00438
Compare
squaremo
changed the title
|
Hey @justinbarrick, if you get a chance, can you have a glance at this (code and or trying out)? I do not trust my head to get it right ... |
|
Your code and tests look good! You have a test that covers: Maybe you could cover the really basic case as well: And I’ll give this a try on my cluster tonight! |
|
So I tried this out in my personal Kubernetes cluster, which is primarily managed using charts and FluxHelmReleases. Some notes:
Overall, looks great! |
|
This is a crappy patch that fixes the issue I described above: oliviabarrick@acdc513 It makes it work correctly in my cluster, but I'm not sure if that's how you want to fix it. |
|
Another case to test might be: |
Ahh yeah I see the mistake I've made, thanks for puzzling that out. I wish I could think of a better way of dealing with the different types .. I'm tempted to just recursively transform the |
|
Thank you very much for checking it over @justinbarrick, that's great feedback. I'll do another round to fix up the things you pointed out.
At present I expect one or other of the formats, whichever is seen first -- I don't think there's any reason not to accept all entries that match any format, though. |
The output of `helm create` uses an encoding of the image to put in
the deployment template which looks like this:
values:
image:
repository: weaveworks/flux
tag: 1.4.3
This commit adds support for interpreting FluxHelmRelease resources
and manifests that use that format, as well as its derivative:
values:
containerA:
image:
repository: weaveworks/containerA
tag: 0.1
containerB:
image:
repository: weaveworks/containerB
tag: 0.1
Support for updating FluxHelmRelease YAMLS using the {image:
{repository, tag}} format comes with kubeyaml 0.4.1.
squaremo
force-pushed
the
enhance/new-kubeyaml
branch
from
8a00438 to
77c388e
Compare
and rewrote the FluxHelmRelease values "interpretation" code to cover the missing case shown in oliviabarrick/flux@acdc513. |
|
Just tried it in my cluster and it's working well! |
When intepreting the `values` part of a FluxHelmRelease, _all_ structures than can be interpreted as specifying a container (something mentioning an image, essentially) should be returned, since that is both more useful and more in line with expectations. Prior to this commit, the implementation would return _either_ a container defined at the top level (i.e., an `image` field), _or_ a set of containers defined in other fields. But this is not adequate even for our own flux chart.
This has the matching change to the previous commit, that is to interpret all containers in `values` rather than a top-level `image` field _or_ subfields.
Right; as it's implemented now, fluxd doesn't have access to the charts, so it won't know that the chart has a You can put the repository value in the
We could extend the interpretation of FluxHelmRelease to cover these -- I'm sure there will always be exceptions, whatever we do. I've open an issue for these varieties anyway (#1262).
@justinbarrick Did this problem still appear the more recent time you tried it? EDIT: remove todo for dealing with tag values that aren't strings; see comment below. |
|
The problem I was referring to there was the one uncovered by the missing test case. So, yes, I could reproduce it but it’s resolved now 😀 |
Arp, nope. You just have to quote values, at least those that look like floats or ints. The reason is this: YAML will parse those number-like values as ints or floats, and the actual string cannot be reliably recovered. (OK, it can if it's an int, but not if it's a float and I'd rather have no special cases). |
|
@aaron7 With Justin's thumbs up, I reckon this is ready for code review -- sorry for the premature review request |
|
Oh good point in regards to floats! Octal and hex integers might have been an issue if you tried that too. |
cluster/kubernetes/resource/spec.go
Outdated
| @@ -27,6 +27,11 @@ func (t PodTemplate) Containers() []resource.Container { | |||
| im, _ := image.ParseRef(c.Image) | |||
| result = append(result, resource.Container{Name: c.Name, Image: im}) | |||
| } | |||
| for _, c := range t.Spec.InitContainers { | |||
| // FIXME(michael): account for possible errors here | |||
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Much better idea than naming myself.
manifests, so we can update them;
rather than putting the whole image ref in one field. Support that
(after kubeyaml) by interpreting the values fields, or its values,
depending on whether a "tag" entry is present.
including those generated by
helm create(and not substantially altered afterwards). Support that too.Fixes #1226.