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

Gw/chart fix for external postgres #275

Closed

Conversation

guidowalter
Copy link

@guidowalter guidowalter commented Feb 8, 2023

Proposed changes

When updating Weblate using an existing Postgresql Instance, the Helm chart will fail / Weblate will not start due to missing Posgresql Enviroment variables.
This is due to the fact, that postgresql.enabled must be set to false to not having a postgresql pod created during deployment, but the env. variables willl only be set, if the postgresql.enabled is set to true (see current deployment.yaml).

To avaid this, I extended the check in the deployment.yaml to not only look at postgresql.enabled but also if postgresql.postgresqlHost is set. If the one or the other condition is true, the env. variables must be set.

I also extended the secrets.yaml to include the before exting postgresql.auth.postgresqlUsername. For an existing Postgresql Instance it would be helpfull to either use an own username or to use the standard one defined in secrets.yaml. So I check if enablePostgresUser is set to true, then use the "postgres" username, otherwise the value is read from postgresqlUsername. For this I put postgresqlUsername back into values.yaml

Last but not least, the value for the Postgresql port is read from postgresql.service.port in depolyment.yaml. In the values.yaml file, the port information is in postgresql.service.port.postgresql. I chnaged the position in the values .yaml for the port, as this would prevent the deployment to function.

Checklist

  • Lint and unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added documentation to describe my feature.
  • I have squashed my commits into logic units.
  • I have described the changes in the commit messages.

Other information

Sorry, something went wrong.

| postgresql.auth.secretKeys.userPasswordKey | string | `"postgresql-password"` | |
| postgresql.enabled | bool | `true` | |
| postgresql.postgresqlHost | string | `None` | External postgres database endpoint, to be used if `postgresql.enabled == false` |
| postgresql.service.ports.postgresql | int | `5432` | |
| postgresql.service.ports | int | `5432` | |
Copy link
Member

Choose a reason for hiding this comment

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

The template uses postgresql.service.port, shouldn't this match?

Copy link
Author

Choose a reason for hiding this comment

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

Hi,
you mean it should match the original Bitnami Postgresql Template? Only, if it should be inherited.
If so, then we need to change Line 54 in deployment.yaml (Values.postgresql.service.port) ](value: "{{ .Values.postgresql.service.port }}"), because this would create a map entry in the Postgres_Port env variable, which prevents Weblate from starting. Then we can revert the change in the values.yaml.

That said, if we want to inherit all the variables, we need to change more, as for example postgresql.auth.postgresPassword does not match with postgresql.auth.password from the Bitnami template.
I can take care of that and sync the templates :-)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant this:

value: "{{ .Values.postgresql.service.port }}"

I have no clue whether it's related, though.

My helm knowledge is nearly zero, I just try to avoid messing up the chart more than what we've already done (see #226 or #268).

Copy link
Author

Choose a reason for hiding this comment

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

Hi, exactly this is what I described. If we use

value: "{{ .Values.postgresql.service.port }}"

in the deployment.yaml, but have
service:
ports:
postgresql: 5432

in the values yaml, that result in a ENV entry for that Weblate Container somethin like this:
POSTGRES_PORT: map[postgresql: 5432]. Weblate does not start with this port information. I ran into this problem myself.
I decided not to change the deployment.yaml, rather than make the change in the values.yaml.

The problem with #226 is, that the structure of the postgres template changed. The only solution is to delete the deployment and to reinstall. But this results in data loss IF the persitant volume is not set to RETAIN and after the reinstallation manually bound again to the PVC of Weblate. I had to do this also manually but all data relevant PVs are set to retain. This could be fixed by having a pv.yaml and creating a cross reference between pv and pvc in the first place.

#268 might has something to do with the newly added

enabled: true

I ran into this as well when upgrading when disabling the auth for redis.

Long story short: I'll take care of the chart issues tomorrow and have look how to best sync the template values without breaking existing deployments. I will also have a look at the PV/PVC combination, so that even people need to reinstall, they can decide to keep their existing data in the future. I can also write some documentation for existing deployments that might help people out if they run into that problem. I would update this PR and then we can discuss this futher.
Would that be Ok with you?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds great!

@github-actions
Copy link

This pull request has been automatically marked as stale because there wasn’t any recent activity.

It will be closed soon if no further action occurs.

Thank you for your contributions!

@github-actions github-actions bot added the wontfix Nobody will work on this. label Mar 12, 2023
@github-actions github-actions bot closed this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix Nobody will work on this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants