search

cowboysysop / charts

Cowboy Sysop Charts
MIT License
120 stars 70 forks source link

allow VPA Admission controller deployment annotations #576

Closed agrevtsev closed 11 months ago

agrevtsev commented 11 months ago

We are using cert-manager to issue tls certificate for VPA Admission controller. But when certificate renewed, there are no means to use it without restarting VPA Admission controller deployment. We intent to use stakater-reloader to solve such problem, but it requires adding specific annotation to managed deployment. As so, i proposing to alter VPA chart in https://github.com/cowboysysop/charts/pull/575 to allow specify specific annotation for VPA Admission controller deployment. Cheers!

sebastien-prudhomme commented 11 months ago

Hi Alexey, even if it applies to all objects, it should work with the commonAnnotations parameter, no? I'm trying to implement best practices done in Bitnami charts and I don't see any example of such dedicated annotations for Deployments or Statefulsets.

agrevtsev commented 11 months ago

Hi Sebastien! Good point! But if to use commonAnnotations it would apply to all entities, which probably isn’t what we want (at least, in mentioned case with stakater-reloader). And i see at least several component-specific options for defining annotations, like admissionController.podAnnotations or recommender.podAnnotations. Unfortunately i didn't find any specific do's or dont's in bitnami requirements, so totally rely on your decision. Anyway, thank you for work on creation and maintaining such wonderful charts. Cheers, Alexey

agrevtsev commented 11 months ago

@sebastien-prudhomme hi! Please refer https://github.com/bitnami/charts/issues/20983 If i reformat my PR according to this, would it fit?

Br, Alexey

sebastien-prudhomme commented 11 months ago

Great work on the Bitnami template @agrevtsev

Yes you can reformat the same way.

agrevtsev commented 11 months ago

hey @sebastien-prudhomme ! I've managed to reformat PR, and added possibility to set specific annotations for all deployments in chart. Unfortunately, to use common.tplvalues.merge i had to bump bitnami/common dependency for VPA chart. So long, if you'll find this acceptable... Thanks for your time!

Br, Alexey

sebastien-prudhomme commented 11 months ago

Thank again for your great work!