Skip to content

feat(recovery): generalize recovery target config #570

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

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

Conversation

gpothier
Copy link
Contributor

Replace .Values.recovery.pitrTarget.time with .Values.recovery.recoveryTarget, so that we can use any kind of recovery target.

This enables configuration of more advanced recovery options, such as:

  • targetTime
  • targetXID
  • targetLSN
  • targetName
  • etc.

The previous behaviour is actually preserved, but deprecated, so there are no breaking changes.

Resolves #272

Replace `.Values.recovery.pitrTarget.time` with `.Values.recovery.recoveryTarget`,
so that we can use any kind of recovery target.

This enables configuration of more advanced recovery options, such as:
- targetTime
- targetXID
- targetLSN
- targetName
- etc.

The previous behaviour is actually preserved, but deprecated, so there are no breaking changes.

Resolves cloudnative-pg#272

Signed-off-by: Guillaume Pothier <[email protected]>
@gpothier gpothier requested a review from itay-grudev as a code owner April 25, 2025 04:08
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. chart( cluster ) Related to the cluster chart labels Apr 25, 2025
@itay-grudev
Copy link
Collaborator

There is another PR for this, but neither yours nor the others has proper tests for all the parameters.

I only need a non-default-configuration tests that checks if they are passed to the output YAML.

I kind of prefer the other PR though because it's backwards compatible by only adding new options.

@itay-grudev
Copy link
Collaborator

If you have the time to write tests for it, I'll be able to move forward with it.

@itay-grudev
Copy link
Collaborator

Take a look at this PR for reference, as the test case is quite similar - since the options are exclusive there needs to be a separate test case for each option.

@gpothier
Copy link
Contributor Author

Thanks for you comments. I personally think it is easier to simply pass the whole recoveryTarget directly to the CR, instead of having two different schemas (one in the helm chart and one in the CR) to specify the same thing. Additionally, if the CR gets new recovery options, there is nothing else to do here, they will work out of the box.
I'll write tests as requested.

@gpothier
Copy link
Contributor Author

Hi, is there a way to run tests locally? I did not find any info on that in the repo. I tried running the github workflow with Act (https://github.com/nektos/act), but I could not make it work (it fails when creating the Kind cluster apparently). Is it ok to commit my attempts and let Github run the tests until I get them right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart( cluster ) Related to the cluster chart size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for more kinds of recovery targets
2 participants