Improve helmchart template adding fields #4952
Conversation
Add template for nodeSelector, affinity, topologySpreadConstraints, additional annotations, volumes and imagePullSecrets. Signed-off-by: Ronaldo Saheki <[email protected]>
Signed-off-by: Ronaldo Saheki <[email protected]>
Signed-off-by: Ronaldo Saheki <[email protected]>
k8s-ci-robot
added
the
cncf-cla: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ronaldosaheki The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @ronaldosaheki! |
k8s-ci-robot
added
size/XL
|
Hi @ronaldosaheki. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ronaldosaheki
changed the title
k8s-ci-robot
added
cncf-cla: yes
| @@ -44,12 +44,37 @@ spec: | |||
| # values: | |||
| # - amd64 | |||
| # - arm64 | |||
| # - ppc64le | |||
| # - s390x | |||
| # imagePullPolicy for the manager container (e.g., Always, IfNotPresent, Never) | ||
| # imagePullPolicy: IfNotPresent |
There was a problem hiding this comment.
I think we can add that as default
Could we have a PR only for the image
| # Extra volumeMounts for the manager container | ||
| # volumeMounts: | ||
| # - name: extra-config | ||
| # mountPath: /etc/extra |
There was a problem hiding this comment.
I do not know if that will work with the patches but is a good call I think we need a PR for that and e2e tests to ensure that we do not break the behaviour
| # - key: kubernetes.io/os | ||
| # operator: In | ||
| # values: | ||
| # - linux | ||
| # TODO(user): Uncomment the following code to configure the nodeSelector expression | ||
| # annotations, nodeSelector, tolerations, topologySpreadConstraints. | ||
| # nodeSelector: |
There was a problem hiding this comment.
Should it be really added there or via kustomize? Is there a better way to configure it?
We will need to check out and either validate the scenario with e2e tests.
Could we have a PR for those and keep this one only for the imagePull ?
| @@ -1,10 +1,29 @@ | |||
| # [MANAGER]: Manager Deployment Configurations | |||
| controllerManager: | |||
| replicas: 1 | |||
| pod: | |||
| # imagePullSecrets for pulling images from private registries | |||
| imagePullSecrets: [] | |||
There was a problem hiding this comment.
I think I liked it 👍 less the pod above since all here would be only about controller-manager
See : #4912
The person made a valid feedback about how we are exposing the values
I think we will need to change that we might need a PR just to change what we have today on that
but I want to share it with you.
| container: | ||
| image: | ||
| repository: controller | ||
| tag: latest | ||
| # imagePullPolicy for the manager container (e.g., Always, IfNotPresent, Never) |
There was a problem hiding this comment.
Comments are not required.
it is auto-explanatory
And the default as the default avlue IfNotPresent seems the great one
This includes most of changes also discussed in both of this PRs:
ImagePullPolicy: #4932
Align generated helmcharts witth most common helmcharts: #4912
Added fields, changes done in the kustomize, helmchart and values scaffold, following the same style, for kustomize the added fields were added commented out and ready to edit similar to existing fields in kustomize scaffold.
Added fields:
Also updated testdata and docs to pass branch github pipeline.