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

feat(charts)!: helm support for custom annotations and serviceAccount #1741

Merged
merged 27 commits into from
Jul 4, 2024

Conversation

misba7
Copy link
Contributor

@misba7 misba7 commented Jun 3, 2024

closes #1740

This introduces breaking changes to the helm chart as it:

  1. Changes the ingress annotations and labels location.
  2. Introduces a separate Service account.

PLEASE BE ADVISED.

@misba7 misba7 requested review from moabu and iromli as code owners June 3, 2024 05:08
@mo-auto mo-auto added comp-charts-flex Touching folder /flex-cn-setup/pygluu/kubernetes/templates/helm comp-casa Component affected by issue or PR comp-admin-ui Component affected by issue or PR kind-feature Issue or PR is a new feature request labels Jun 3, 2024
Copy link

sonarcloud bot commented Jun 21, 2024

Copy link
Member

@moabu moabu left a comment

Choose a reason for hiding this comment

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

I think there is a misunderstanding here. The custom annotations are per resource per service. You shouldn’t be applying the same custom annotations for every resource.

Per deployment, per configMap, per secrets, per service. For the gluu-all-in-one this is easier since its one service. For the microservices chart you need to add them per service under the global key depending on what resources are there for each service. Forexample, taking auth-server you have 7different resources that need to have custom annotations. Each service(sub chart) might have a different number of resources that need custom annotations.

global:
  auth-server:
   # — Add custom annotations for kubernetes resources for the auth server service
  customAnnotations:
    destinationRule:
    podDisruptionBudget:
    virtualService:
    deployment:
    horizontalPodAutoscaler:
    service:
    secret:
  

{{ toYaml .Values.additionalAnnotations | indent 4 }}
{{- end }}
{{- if .Values.customAnnotations.configMaps }}
Copy link
Member

Choose a reason for hiding this comment

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

only the configMap . These annotations are per resource so here you would just add the annoatation for the configmap only not both secrets and configmaps otherwise the additionalAnnotations would suffice

{{- if .Values.additionalAnnotations }}
{{ toYaml .Values.additionalAnnotations | indent 4 }}
{{- end }}
{{- if .Values.customAnnotations.configMaps }}
Copy link
Member

Choose a reason for hiding this comment

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

same comment as before but this time this shouuld be customAnnotations.cronjobs not certmanager or secret. Per resource

Comment on lines 18 to 24
{{- if .Values.customAnnotations.configMaps }}
{{ toYaml .Values.customAnnotations.configMaps | indent 4 }}
{{- end }}
{{- if .Values.customAnnotations.secrets }}
{{ toYaml .Values.customAnnotations.secrets | indent 4 }}
{{- end }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

same comment as before . it should be customAnnotations.deployment

Copy link
Member

@moabu moabu left a comment

Choose a reason for hiding this comment

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

charts/gluu-all-in-one/values.yaml Show resolved Hide resolved
destinationRule: {}
horizontalPodAutoscaler: {}
gateway: {}
ingress: {}
Copy link
Member

Choose a reason for hiding this comment

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

Please review all ingresses as per ingress additional annotations exist which means this is a duplicate. Same goes for the microservices chart

Comment on lines 13 to 15
{{- if index .Values "auth-server" "ingress" "authServerLabels" }}
{{ toYaml (index .Values "auth-server" "ingress" "authServerLabels") | indent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

this is a duplicate and wrong entry that should be removed. This is not the auth -server service .

Comment on lines 19 to 21
{{- if index .Values "auth-server" "ingress" "webfingerAdditionalAnnotations" }}
{{ toYaml (index .Values "auth-server" "ingress" "webfingerAdditionalAnnotations") | indent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

should be removed.

charts/gluu-all-in-one/values.yaml Show resolved Hide resolved
charts/gluu-all-in-one/values.yaml Show resolved Hide resolved
@moabu moabu changed the title feat: helm support for custom annotations and serviceAccount feat(charts)!: helm support for custom annotations and serviceAccount Jul 2, 2024
Copy link

sonarcloud bot commented Jul 2, 2024

@moabu moabu merged commit fd7e9e5 into main Jul 4, 2024
2 checks passed
@moabu moabu deleted the feat-flex-helm-customization branch July 4, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-admin-ui Component affected by issue or PR comp-casa Component affected by issue or PR comp-charts-flex Touching folder /flex-cn-setup/pygluu/kubernetes/templates/helm kind-feature Issue or PR is a new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: customize helm chart to support custom annotations and serviceAccount
3 participants