-
Notifications
You must be signed in to change notification settings - Fork 93
Multi Framework Installation Helm Support with Comprehensive document… #1122
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
base: main
Are you sure you want to change the base?
Conversation
|
{{- end }} | ||
containers: | ||
- name: {{ .Chart.Name }} | ||
- name: ku{{ .Chart.Name }} |
Check warning
Code scanning / SonarCloud
Memory limits should be enforced Medium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the readme needs work. it has too much list content and little prose.
{{- end }} | ||
containers: | ||
- name: {{ .Chart.Name }} | ||
- name: ku{{ .Chart.Name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ku?
{{- toYaml . | nindent 8 }} | ||
{{- end }} | ||
{{- end }} | ||
{{- if eq .Values.obaas.framework "SPRING_BOOT" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on line 8 you converted to upper case, here you don't
- name: eureka.client.service-url.defaultZone | ||
value: "http://eureka-0.eureka.{{ .Values.obaas.namespace }}.svc.cluster.local:8761/eureka" | ||
- name: eureka.instance.hostname | ||
value: {{ include "obaas-app.fullname" . }}-{{ $.Release.Namespace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be the key other services use to look up this service in the registry. i think that assuming they would know the namespace may be a bad assumption.
- Building cloud-native, containerized applications | ||
- Prefer explicit configuration over auto-configuration | ||
|
||
### Resource Comparison |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this either needs evidence so people can reproduce/confirm, or delete it. at the very least, it would need to state what the configuration of the test system was
# Liquibase configuration (uses separate admin credentials for migrations) | ||
- name: LIQUIBASE_ENABLED | ||
value: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't LIQUIBAS_ENABLED be in the application.yaml/properties. That's what cbv5 have
- name: hibernate.hbm2ddl.auto | ||
value: {{ $.Values.obaas.helidon.hibernate.hbm2ddl_auto | quote }} | ||
- name: hibernate.show_sql | ||
value: {{ $.Values.obaas.helidon.hibernate.show_sql | quote }} | ||
- name: hibernate.format_sql | ||
value: {{ $.Values.obaas.helidon.hibernate.format_sql | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be in application.yaml/properties. CBV5 have it there
kind: Deployment | ||
metadata: | ||
name: {{ include "obaas-app.fullname" . }} | ||
namespace: {{ .Values.obaas.namespace }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't take out this, value is set in values.yaml file
framework: REPLACE_WITH_FRAMEWORK # Options: SPRING_BOOT or HELIDON | ||
|
||
database: | ||
enabled: true # If true variables with DB secret content will be created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove this parameter
{{- if $.Values.obaas.springboot.enabled }} | ||
- name: SPRING_PROFILES_ACTIVE | ||
value: {{ $obaas.data.SPRING_PROFILES_ACTIVE | quote }} | ||
{{- if $.Values.obaas.database.enabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove this it is a parameter in the values.yaml
{{- if $.Values.obaas.otel.enabled }} | ||
- name: OTEL_EXPORTER_OTLP_ENDPOINT | ||
value: {{ (index $obaasObs.data "signoz-otel-collector") | quote }} | ||
value: {{ index $observability.data "signoz-otel-collector" | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will ned the () surrounding the index index $observability.data "signoz-otel-collector" otherwise it can fail
value: {{ $obaas.data.MP_LRA_COORDINATOR_URL | quote }} | ||
- name: MP_LRA_PARTICIPANT_URL | ||
value: {{ $obaas.data.MP_LRA_PARTICIPANT_URL | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$obaas.data.MP_LRA_COORDINATOR_URL and $obaas.data.MP_LRA_PARTICIPANT_URL doesn't exist in the configmap. it is called $obaas.data.otmm
…ation