Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added missing attributes and improvements to deployment yaml file and… #14

Closed
wants to merge 2 commits into from

Conversation

robece
Copy link
Contributor

@robece robece commented Jan 29, 2020

… normalized yaml file names

  • Added features to deployment: nodeSelector, securityContext, tolerations, affinity

… normalized yaml file names

- Added features to deployment: nodeSelector, securityContext, tolerations, affinity
- name: {{ .Values.operatorName }}-metrics-apiserver
image: {{ .Values.image.metricsAdapter }}
image: "{{ .Values.image.metricsAdapter }}:{{ .Chart.AppVersion }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for you if we add this @ahmelsayed? It's fine for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if there is a way to override the full image name because it enables using helm for deploying a custom keda. like this

@tomkerkhove
Copy link
Member

LGTM but prefer to have @ahmelsayed his sign-off as well

@tomkerkhove
Copy link
Member

Let's merge #16 first so we have the CI validation as well.

@ahmelsayed
Copy link
Contributor

Thanks @robece. I think we need to merge #10 first which will cause a conflict with your changes, but #10 is part of kedacore/keda#506 which I merged yesterday in the other repo but missed the one here.

Would you mind merging the changes in master again? sorry about that

@robece
Copy link
Contributor Author

robece commented Jan 29, 2020

No worries, please let me know once the first merge is done and I can do the second merge with my changes today.

@tomkerkhove
Copy link
Member

Closing since #17 is now open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants