nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
325 stars 264 forks source link

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

Closed schmittvictor closed 9 months ago

schmittvictor commented 10 months ago

Pull Request

Description of the change

Fix postgresql-isready image

Benefits

Possible drawbacks

Applicable issues

Additional information

Checklist

jessebot commented 10 months ago

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 commented 10 months ago

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

MaximUltimatum commented 10 months ago

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

schmittvictor commented 10 months ago

I resolved the conflicts, I think you can approve

provokateurin commented 9 months ago

@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).

jessebot commented 9 months ago

@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 https://github.com/nextcloud/helm/pull/262, which is one of our oldest PRs, how about we merge this, and then I can create another PR to do mariadb?

provokateurin commented 9 months ago

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 commented 9 months ago

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