-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: multi region replica strike implementation #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the rdsClient.DescribeDBInstanceAutomatedBackups in the multiregion.go. Is that meant to be there?
No, i had the logic wrong for it. Fixed it |
The db I used didnt had a multi region setup so it failed as expected |
strikes/common.go
Outdated
if viper.IsSet("raids.RDS.aws.config.instance_identifier") { | ||
return viper.GetString("raids.RDS.aws.config.instance_identifier"), nil | ||
} | ||
return "", errors.New("database instance identifier must be set in the config file") | ||
} | ||
|
||
func getHostRDSRegion() (string, error) { | ||
if viper.IsSet("raids.RDS.aws.config.region") { | ||
return viper.GetString("raids.RDS.aws.config.region"), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably for a later PR, but RDS is specifically Amazon, right? If anything, we'd probably do raids.aws.rds...
🤔 Noting this for later discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was more so reading this as raiding a database against different cloud provider. RDS term is aws specific, you are right, maybe we change that to DB or RDMS in a later pr. Also noting that we need to change the repo name as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I added a couple questions in the comments for consideration
Updated
|
This adds the strike for checking the specified rds has a multi region read replica created or not.
Did some code reconfiguration for common functions
Also added a map of aws regions to its abbreviations because of the output from the
DescribeDBInstanceAutomatedBackups
functionChecked against the privateer binary as well