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

added BackupRetentionPeriod config #173

Merged
merged 1 commit into from
Sep 14, 2023
Merged

added BackupRetentionPeriod config #173

merged 1 commit into from
Sep 14, 2023

Conversation

ssuman2-infoblox
Copy link
Contributor

V1 Story: B-98504

Added BackupRetentionPolicy flag to configure it for prod and non-prod envs

Copy link
Contributor

@bjeevan-ib bjeevan-ib left a comment

Choose a reason for hiding this comment

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

should we account aurora workflow?

Copy link
Contributor

@abalaven abalaven left a comment

Choose a reason for hiding this comment

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

What are we specifying in the backupRetentionPeriod field?

@ssuman2-infoblox ssuman2-infoblox changed the title added BackupRetentionPeriod config for postgresql added BackupRetentionPeriod config Sep 8, 2023
@ssuman2-infoblox
Copy link
Contributor Author

should we account aurora workflow?

@bjeevan-ib I've added this config for DBCluster as well. Usually, backup retention period can't be set to 0 for aurora but the value can be managed with DBCluster.

What are we specifying in the backupRetentionPeriod field?

@abalaven this field is used to set the number of days for which automated backups are retained. More about this is here: https://doc.crds.dev/github.com/crossplane/provider-aws/rds.aws.crossplane.io/DBInstance/[email protected]#spec-forProvider-backupRetentionPeriod

Copy link
Contributor

@abalaven abalaven left a comment

Choose a reason for hiding this comment

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

IIUC, the default value for backupRetentionPeriod is set to 0 in the values file, which I believe will be added to the configMap. Is there any provision to set the value independent of the CM? The CM only holds default values if I'm not wrong.

Copy link
Contributor

@drewwells drewwells left a comment

Choose a reason for hiding this comment

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

Use a string and pass it through time.ParseDuration(). time.Duration is represented as int64 in nanoseconds, so this will be ridiculous to configure if somebody wants 365 days
backup: 3.1536e16

@pjferrell
Copy link
Contributor

pjferrell commented Sep 11, 2023 via email

@ssuman2-infoblox
Copy link
Contributor Author

Use a string and pass it through time.ParseDuration(). time.Duration is represented as int64 in nanoseconds, so this will be ridiculous to configure if somebody wants 365 days backup: 3.1536e16

@drewwells @pjferrell, as per docs it only accepts value from 0-35 (0 means disabled) and it denotes number of days. We can't set it beyond 35.
image

@ssuman2-infoblox
Copy link
Contributor Author

IIUC, the default value for backupRetentionPeriod is set to 0 in the values file, which I believe will be added to the configMap. Is there any provision to set the value independent of the CM? The CM only holds default values if I'm not wrong.

@abalaven I initially tried using arguments to db-controller to pass such values but it required multiple function signature changes. Also we don't want it to be set at dbclaim level but db-controller so CM seemed a good option as it's already configured to take db-controller config as input.

Added something similar here: #172

@bjeevan-ib bjeevan-ib merged commit dd6e19a into main Sep 14, 2023
2 checks passed
@bjeevan-ib bjeevan-ib deleted the sujay branch September 14, 2023 04:59
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.

5 participants