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(barman): Adjust slot-creation #631

Merged
merged 4 commits into from
Apr 11, 2022
Merged

fix(barman): Adjust slot-creation #631

merged 4 commits into from
Apr 11, 2022

Conversation

ggggut
Copy link
Contributor

@ggggut ggggut commented Apr 7, 2022

Description

Hello, creating a slot failed on our side because ERROR: Unknown server 'pg'.
I found it hardcoded in the cronjob.
Wouldn't it be better to use the postgres.host value for creating the db slot?
Let me know what you think!

Best

Issues

barman.server ERROR: Check 'replication slot' failed for server 'xxxxxxx'
	replication slot: FAILED (replication slot 'barman' doesn't exist. Please execute 'barman receive-wal --create-slot xxxxxxx')
	directories: OK
	retention policy settings: OK

Checklist

  • This PR contains a description of the changes I'm making
  • I updated the version in Chart.yaml
  • I updated applicable README.md files using pre-commit run
  • I documented any high-level concepts I'm introducing in docs/
  • CI is currently green and this is ready for review
  • I am ready to test changes after they are applied and released

@ggggut ggggut requested review from a team and michaelimfeld as code owners April 7, 2022 15:41
@ggggut ggggut requested review from inisitijitty and vmaillot and removed request for a team April 7, 2022 15:41
@github-actions github-actions bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 7, 2022
@hairmare
Copy link
Contributor

hairmare commented Apr 7, 2022

postgres.host does feel much better than having a hardcoded pg string. It should probably also get surfaced in values.yaml.

For the barman.backups thing to work it would probably also need some if hasKey/$.Values stuff in the template (like what is already there for backupSchedule).

@ggggut
Copy link
Contributor Author

ggggut commented Apr 7, 2022

thank you! i will look into it tomorrow

@ggggut
Copy link
Contributor Author

ggggut commented Apr 8, 2022

The barman.backups key works now. But..

It should probably also get surfaced in values.yaml.

Can you elaborate?

@github-actions github-actions bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 8, 2022
@ggggut
Copy link
Contributor Author

ggggut commented Apr 8, 2022

I also added the possibility to add annotations. This could be useful in a few cases.
In our case we would need it to annotate velero backups.

@hairmare
Copy link
Contributor

It should probably also get surfaced in values.yaml.

Can you elaborate?

I didn't realise that .postgres.host is already defined in values.yaml 🤦

Copy link
Contributor

@hairmare hairmare left a comment

Choose a reason for hiding this comment

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

LGTM

@hairmare hairmare merged commit 2fa2220 into adfinis:main Apr 11, 2022
@hairmare hairmare mentioned this pull request Jun 20, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants