Added missing attributes and improvements to deployment yaml file - Merged with PR #10 #17
Conversation
- Added features to deployment: nodeSelector, securityContext, tolerations and affinity
- Added normalized yaml files names
- Disabled CRD template installation for Helm 3, to prevent: info:
skipping unknown hook: "crd-install"
to install in Helm 2 add the parameter: --set customResourceDefinition.create=true
robece
requested review from
ahmelsayed,
jeffhollan and
tomkerkhove
as code owners
robece
changed the title
|
Looks like a flag is required for Helm 2.X now, would you mind sending a PR on https://github.com/kedacore/keda-docs to update https://keda.sh/deploy/ please? |
| - name: {{ .Values.operatorName }}-metrics-apiserver | ||
| securityContext: | ||
| {{- toYaml .Values.securityContext | nindent 12 }} | ||
| image: "{{ .Values.image.metricsAdapter }}:{{ .Chart.AppVersion }}" |
There was a problem hiding this comment.
As per @ahmelsayed we'd prefer not to do this: #14 (comment)
There was a problem hiding this comment.
sure, let me add the changes for custom keda deployments! :)
|
All our files have been numbered as of #10 so I think it's best to stick with that. I'm adding @zroubalik in case you have any questions on that. Feel free to make the required changes to this branch so that we can keep the history of this feature change. |
|
You've mentioned this:
Can you elaborate on what has changed please? |
|
thanks @tomkerkhove , let me add the numbers , for the second question about "Disabled CRD template installation..." since Helm 3 removed the hooks and crds folder works for the CRD installation, the change was in the flag (customResourceDefinition.create) value to prevent CRD hook installation if you are using Helm 3, this avoid the some warning messages, using this flag only in case you want to use Helm 2. |
|
Btw. this is a good question, in my first commit I removed the hooks for Helm 2 support to keep the use only for Helm 3 CRD installation folder files without hooks, but in the merge I saw there are pull request validations for Helm 2 and I prefer to keep the hooked CRD files in the template folder. |
|
Hi @tomkerkhove @ahmelsayed , just to comment that I sent a PR for the documentation to add the parameter in the Helm 2 installation, kedacore/keda-docs#76, thanks! |
|
I guess it's a side effect of us trying to have the same chart for both versions. I'm not sure how other charts manage that? separate helm repos? |
|
Completely agree @ahmelsayed , originally I was planning to let the changes just for Helm 3, what do you think @tomkerkhove if we remove the hooks from this chart and create another chart for version 2? if everyone agree we can remove the lint validation for Helm 2 and commit the new changes to support only Helm 3 in this chart. |
|
Tried to poke around and find another project that creates CRDs that has a helm 2 and helm 3 chart to see if any "best practices" were around. Couldn't find any. May ping helm team tomorrow Edit: https://cloud-native.slack.com/archives/CGF207RB5/p1580440634001200 |
|
I've opened #20 which is the culprit here. Since we don't have proper support for publishing older chart versions we have to be backwards compatible. Once we have that one we just bump the major version and remove Helm 2.x support. What do you guys think? Happy to take this one up if we can get an Azure Container Registry or so. |
|
Hi, I was wondering if there is anything else pending to move this change forward or if there is any pending conversation we want to check, if you need any help let me know, cheers! |
|
The change looks good to me. My only reservation is around requiring the extra flag for helm 2, but if @jeffhollan and @tomkerkhove are okay with it, then it's fine. |
|
I'd suggest to keep the same experience for Helm 2.x (and thus not use the flag) until we have resolved #20 |
|
Let's do that, I will remove the flag for Helm 2 in order to push the rest of the changes, in addition, I'll update the documentation to remove the flag, cheers!! |
|
Thanks @robece! |
|
Things turn out to be properly merged so we might be able to bit the bullet and fix this |
Added features to deployment: nodeSelector, securityContext, tolerations and affinity
Added normalized yaml files names
Disabled CRD template installation for Helm 3.x, to prevent the message: info: skipping unknown hook: "crd-install", to install in Helm 2.x just add the parameter: --set customResourceDefinition.create=true
Relates to #18