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

Fix postgresql-isready image - adds postgresql.image.registry from values.yaml #471

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

schmittvictor
Copy link
Contributor

@schmittvictor schmittvictor commented Nov 13, 2023

Pull Request

Description of the change

Fix postgresql-isready image

Benefits

Possible drawbacks

Applicable issues

Additional information

Checklist

@jessebot
Copy link
Collaborator

Could you please make sure to document this option in the readme under this section?
https://github.com/nextcloud/helm/tree/main/charts/nextcloud#database-configurations

@jessebot
Copy link
Collaborator

@schmittvictor could you please answer @provokateurin's question and also resolve the conflicts?

@MaximUltimatum
Copy link
Contributor

MaximUltimatum commented Nov 21, 2023

I attempted to push the fix to this PR, but I don't have access rights. (Did test out the linting tool mentioned in the open Contributing doc update though, which is rad). Regardless, @schmittvictor you should be able to pull in the default strategy used here https://github.com/nextcloud/helm/pull/262/files, per @jessebot 's earlier comment to satisfy @provokateurin 's concern

Signed-off-by: Victor S <35772566+schmittvictor@users.noreply.github.com>
Signed-off-by: Victor S <35772566+schmittvictor@users.noreply.github.com>
Signed-off-by: Victor S <35772566+schmittvictor@users.noreply.github.com>
@schmittvictor schmittvictor reopened this Nov 23, 2023
Signed-off-by: Victor S <35772566+schmittvictor@users.noreply.github.com>
@schmittvictor
Copy link
Contributor Author

I resolved the conflicts, I think you can approve

@provokateurin
Copy link
Member

@schmittvictor Would it be possible for you to also fix this for all the other images we use? You can easily search for .image.repository. I found that the mariadb-isalive initcontainer already handles the registry, but not with the fallback (which should be fixed as well).

@@ -297,7 +297,7 @@ spec:
- {{ printf "until mysql --host=%s-mariadb --user=${MYSQL_USER} --password=${MYSQL_PASSWORD} --execute=\"SELECT 1;\"; do echo waiting for mysql; sleep 2; done;" .Release.Name }}
{{- else if .Values.postgresql.enabled }}
- name: postgresql-isready
image: {{ .Values.postgresql.image.repository }}:{{ .Values.postgresql.image.tag }}
image: {{ .Values.postgresql.image.registry | default "docker.io" }}/{{ .Values.postgresql.image.repository }}:{{ .Values.postgresql.image.tag }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
image: {{ .Values.postgresql.image.registry | default "docker.io" }}/{{ .Values.postgresql.image.repository }}:{{ .Values.postgresql.image.tag }}
image: {{ template "postgresql.v1.image" .Subcharts.postgresql }}

We could also do this now that I think about it, which is less wordy. It templates this:

https://github.com/bitnami/charts/blob/0fa3d414009575ed81ff876fd883338afac1ee77/bitnami/postgresql/templates/_helpers.tpl#L44-L49

Which does all the fancy logic here:
https://github.com/bitnami/charts/blob/0fa3d414009575ed81ff876fd883338afac1ee77/bitnami/common/templates/_images.tpl#L6-L30

@jessebot
Copy link
Collaborator

@schmittvictor Would it be possible for you to also fix this for all the other images we use? You can easily search for .image.repository. I found that the mariadb-isalive initcontainer already handles the registry, but not with the fallback (which should be fixed as well).

To move this foward, as this would also close #262, which is one of our oldest PRs, how about we merge this, and then I can create another PR to do mariadb?

@provokateurin
Copy link
Member

Sure. I think we shouldn't go with your suggestion of letting the subcharts handle this. Otherwise we will have even more diverging logic for the images.

@jessebot
Copy link
Collaborator

that seems fine to me :) I was just trying to reduce code as they've already written the handlers here, but I'm not married to this at all. I'll merge this and then create a new PR for mariadb

@jessebot jessebot merged commit f46483b into nextcloud:main Dec 15, 2023
2 checks passed
@jessebot jessebot changed the title Fix postgresql-isready image Fix postgresql-isready image - adds postgresql.image.registry from values.yaml Dec 15, 2023
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.

postgresql image tags not iso for pods "postgresql-isready" and "postgresql"
4 participants