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.

Sign up for GitHub

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

Clean up image values for horizon chart #99

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Clean up image values for horizon chart #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/horizon/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
{{- end -}}

{{- define "common.horizonImage" -}}
{{ .Values.global.image.horizon.registry }}/{{ .Values.global.image.horizon.repository }}:{{ default .Chart.AppVersion .Values.global.image.horizon.tag }}
{{ .Values.image.registry }}/{{ .Values.image.repository }}:{{ default .Chart.AppVersion .Values.image.tag }}
{{- end -}}

{{- define "core.config" -}}
Expand Down
6 changes: 3 additions & 3 deletions charts/horizon/templates/ingest-statefulset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ spec:
- {{ $flag | quote }}
{{- end }}
{{- end }}
imagePullPolicy: {{ .Values.global.image.horizon.pullPolicy }}
imagePullPolicy: {{ .Values.image.pullPolicy }}
ports:
- containerPort: {{ .Values.ingest.horizonConfig.port | default 8000 }}
name: horizon
Expand Down Expand Up @@ -82,8 +82,8 @@ spec:
{{- end }}
{{- if .Values.ingest.coreExporter.enabled }}
- name: stellar-core-prometheus-exporter
image: "{{ .Values.global.image.coreExporter.registry }}/{{ .Values.global.image.coreExporter.repository }}:{{ .Values.global.image.coreExporter.tag }}"
imagePullPolicy: {{ .Values.global.image.coreExporter.pullPolicy }}
image: "{{ .Values.ingest.coreExporter.image.registry }}/{{ .Values.ingest.coreExporter.image.repository }}:{{ .Values.ingest.coreExporter.image.tag }}"
imagePullPolicy: {{ .Values.ingest.coreExporter.image.pullPolicy }}
ports:
- containerPort: 9473
name: metrics
Expand Down
2 changes: 1 addition & 1 deletion charts/horizon/templates/web-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ spec:
- {{ $flag | quote }}
{{- end }}
{{- end }}
imagePullPolicy: {{ .Values.global.image.horizon.pullPolicy }}
imagePullPolicy: {{ .Values.image.pullPolicy }}
ports:
- containerPort: {{ .Values.web.horizonConfig.port | default 8000 }}
name: horizon
Expand Down
37 changes: 13 additions & 24 deletions charts/horizon/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,15 @@ global:
# - imagepullsecret1
# - imagepullsecret2

image:
horizon:
Comment on lines -27 to -28
Copy link
Contributor

Choose a reason for hiding this comment

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

The horizon image settings are in the global section to allow parent chart to override the value. There is an initial "stellar-network" chart somewhere that we planned to use to deploy full stacks for testing.
I think we should leave it as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a global chart can still overwrite the values that are not in the global section
however, putting values like this in the global section makes the chart less flexible, because any upstream values that might conflict would break a chart

Copy link
Contributor

Choose a reason for hiding this comment

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

global is a section used to communicate values between charts. The container image horizon should not be overwritten by other charts.

The intention, from what I remember, was for the horizon image to be overwritten by the parent chart. I'll try to remember some more details.
Could you explain a bit more why you think the container image for horizon should not be overwriten?

In addition the core exporter image is only used in one place

It's used in 1 place in this chart. But the exporter is also used by the core chart

registry: docker.io
repository: stellar/stellar-horizon
## Optional tag. Defaults to the appVersion from Chart.yaml
# tag: 1.2.3

## Specify a imagePullPolicy
## Defaults to 'Always' if image tag is 'latest', else set to 'IfNotPresent'
## ref: http://kubernetes.io/docs/user-guide/images/#pre-pulling-images
##
pullPolicy: Always
coreExporter:
registry: docker.io
repository: stellar/stellar-core-prometheus-exporter
tag: latest
## Specify a imagePullPolicy
## Defaults to 'Always' if image tag is 'latest', else set to 'IfNotPresent'
## ref: http://kubernetes.io/docs/user-guide/images/#pre-pulling-images
##
pullPolicy: Always
image:
registry: docker.io
repository: stellar/stellar-horizon
## Optional tag. Defaults to the appVersion from Chart.yaml
# tag: 1.2.3
pullPolicy: Always

ingest:
## See global.image.horizon for image settings
## See image for image settings

## Whether to deploy ingesting instances
enabled: true
Expand Down Expand Up @@ -118,7 +103,11 @@ ingest:
## If you'd like to have prometheus
enabled: true

## See global.image.coreExporter for image settings
image:
registry: docker.io
repository: stellar/stellar-core-prometheus-exporter
tag: latest
pullPolicy: Always
Comment on lines +106 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

We were planning on building full stellar network stacks and for that to work we needed the parent chart to be able to set the exporter. The exporter will also be used by the core chart which deploys validators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the parent chart can still set the exporter here


## Uncomment to set resource limits
#resources:
Expand All @@ -130,7 +119,7 @@ ingest:
# memory: 128Mi

web:
## See global.image.horizon for image settings
## See image for image settings

## Whether to deploy non-ingesting instances
## Useful with read-only DB replica
Expand Down