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

Provide capability to specify annotations in Helm chart #27

Merged
merged 2 commits into from
Mar 16, 2020
Merged

Provide capability to specify annotations in Helm chart #27

merged 2 commits into from
Mar 16, 2020

Conversation

tomkerkhove
Copy link
Member

Signed-off-by: Tom Kerkhove kerkhove.tom@gmail.com

Closes #26

Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
@ncrothe
Copy link
Contributor

ncrothe commented Mar 11, 2020

That should work. Comparing this to the image values, I would suggest using annotation.keda and annotation.metricsAdapter for consistency. Also, I would suggest podAnnotation instead of plain annotation as the chart might end up with serviceAnnotations etc. at some point.

On a more general note: Looking at the structure of the values section, the multi-component nature of the chart and other charts I have seen, I do, however, think you might want to consider switching from a property.component structure to component.property eventually.

I.e. from

annotations:
  operator:
  apiserver:

etc. to something like

operator:
  image:
  replicaCount:
  podAnnotations:
metrics:
  image:
  replicaCount:
  podAnnotations:

Though this does go well beyond the scope of an MR like this and would also break existing deployments. But something to consider for a future version bump.

@ncrothe
Copy link
Contributor

ncrothe commented Mar 11, 2020

Sorry, did not realise github ate the angular brackets in last line of second paragraph. Fixed my suggestion to reflect. @tomkerkhove

@ahmelsayed ahmelsayed merged commit d9419d2 into kedacore:master Mar 16, 2020
@tomkerkhove tomkerkhove deleted the deployment-annotations branch March 16, 2020 22:36
@ncrothe
Copy link
Contributor

ncrothe commented Mar 18, 2020

@ahmelsayed Is it possible to make a timely 1.3.1 release of the chart? Because this fix would go really well with the annotation-based podIdentity stuff in the 1.3.0 release of the operator.

@ahmelsayed
Copy link
Contributor

@ncrothe Done :)

@ncrothe
Copy link
Contributor

ncrothe commented Mar 31, 2020

As pointed out in kedacore/keda#706 the annotations ended up in the wrong place. The are currently in the deployments' metadata, but need to go into the spec's metadata, i.e. to spec.template.metadata.

viperKiran pushed a commit to PaytmLabs/kedacore-charts that referenced this pull request Mar 31, 2020
* Provide capability to specify annotations in Helm chart

Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>

* Use podAnnotations for metricsAdapter / keda

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>
tomkerkhove added a commit that referenced this pull request Apr 2, 2020
* Provide capability to specify annotations in Helm chart (#27)

* Provide capability to specify annotations in Helm chart

Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>

* Use podAnnotations for metricsAdapter / keda

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

* Release 1.3.1 chart

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

* Add priorityClassName to pods specs (#32)

* Add priorityClassName to pods specs

Signed-off-by: Amir Schwartz <amschwar@microsoft.com>

* Increment chart version

Signed-off-by: Amir Schwartz <amschwar@microsoft.com>

Co-authored-by: Amir Schwartz <amschwar@microsoft.com>
Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

* Fixed affinity and added pod distruption budget for operator and metric-apiserver

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

* Fixed CR comments

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

* Changed pdb template filename

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

* Removed the changes caused by merge conflict

Signed-off-by: kiran <kiran.sundaravarathan@paytm.com>

Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Co-authored-by: Ahmed ElSayed <ahmed@elsayed.io>
Co-authored-by: amirschw <24677563+amirschw@users.noreply.github.com>
Co-authored-by: Amir Schwartz <amschwar@microsoft.com>
Co-authored-by: kiran <kiran.sundaravarathan@paytm.com>
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.

Allow pod annotations via Chart
4 participants