-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conditional init container, default to disabled #132
Conditional init container, default to disabled #132
Conversation
Not everyone has privileged access
…ontainer test doesn't fail.
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really useful. I totally didn't think about environments where this would be set on the node level. Thank you for making it configurable!
Apart from the 2 review comments this PR only needs the new value added into the readme. Apart from that it is all ready to go!
@@ -159,3 +159,6 @@ ingress: | |||
|
|||
nameOverride: "" | |||
fullnameOverride: "" | |||
|
|||
sysctlInitContainer: | |||
enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to put false here? The default value that you added to the test is true
so the template tests are going to fail.
@@ -104,6 +104,7 @@ spec: | |||
imagePullSecrets: | |||
{{ toYaml .Values.imagePullSecrets | indent 8 }} | |||
{{- end }} | |||
{{- if .Values.sysctlInitContainer.enabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to go after the initContainers:
block otherwise this will break extraInitContainers
. There is a template test that catches this use case too which is failing:
________________________________ test_adding_a_extra_init_container _________________________________
def test_adding_a_extra_init_container():
config = '''
extraInitContainers: |
- name: do-something
image: busybox
command: ['do', 'something']
'''
r = helm_template(config)
> extraInitContainer = r['statefulset'][uname]['spec']['template']['spec']['initContainers']
E KeyError: 'initContainers'
It's not bumped automatically. This chart is released with the same versioning and cadence of the rest of the elastic stack. There will also be occasional extra patch releases in between releases too though. |
I would love to use this but there are a couple of reasons why we can't yet.
I would be totally open for a PR that allows the sysctl values to bet set via this method, however we can't make it the default just yet. |
@Crazybus I guess you should close this one :) |
Not everyone can run privileged containers, even though
vm.max_map_count
is set on the platform.${CHART}/tests/*.py
${CHART}/examples/*/test/goss.yaml
It's unclear whether the chart version in
README.md
is bumped automatically