-
Notifications
You must be signed in to change notification settings - Fork 6
DOCSP-46223: Add Disaster Recovery example script for go sdk #74
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
base: main
Are you sure you want to change the base?
Conversation
dacharyc
left a comment
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.
Overall looks good, but a couple of small fixups are needed. There are a couple of files here that I think probably should not be here, and we probably need to provide some info about the new expected DR env vars.
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.
It looks like this is a net new file as of this PR, so we can just omit this from this PR, can't we? I assume we don't need a comment about tests moving if they're net new here.
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.
Can we omit this empty file from this PR?
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 the first test file I've seen in the examples directory, so just curious about the decision to test here instead of in internal.
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.
it was so i could test the private helper functions that i ended up leaving in main.go so they'd be part of the on-page snippet
| AddNodes: defaultAddNodes, | ||
| } | ||
|
|
||
| o.Scenario = strings.ToLower(strings.TrimSpace(os.Getenv("DR_SCENARIO"))) |
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 see we're now expecting some disaster-recovery-related vars in the env file, but I don't see any info about that in the README or .env.example. How are we exposing these new required vars to devs?
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.
these are now part of the config in a new stanza (similar to the scaling settings)
| // | ||
| // DR_SCENARIO (req) regional-outage | data-deletion | ||
| // ATLAS_PROJECT_ID (red unless provided via config loader) | ||
| // ATLAS_CLUSTER_NAME (req) target cluster name |
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.
It looks like this PR is expecting ATLAS_CLUSTER_NAME in the env file, but other PRs have them in the config. Should we be consistent about where we expect this to live?
| var summary string | ||
| var opErr error | ||
|
|
||
| switch opts.Scenario { |
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.
Overall, nothing here jumps out at me. However, I wasn't able to actually test this. When I tried testing the regional outage scenario in Bushicorp, I get an error that scenario failed because the target region isn't found in replication specs. I didn't try to test the data recovery scenario because I'm not sure how to get/create a snapshot ID.
So my caveat here is that I haven't fully validated the functionality as "works on my machine."
| var opErr error | ||
|
|
||
| switch opts.Scenario { | ||
| case scenarioRegionalOutage: |
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.
Everything looks good here in terms of output files, and I was able to build and execute this new example, but same caveat applies that I wasn't able to fully complete either scenario due to configuration constraints.
| MONGODB_ATLAS_SERVICE_ACCOUNT_ID=<your_service_account_id> | ||
| MONGODB_ATLAS_SERVICE_ACCOUNT_SECRET=<your_service_account_secret> | ||
| ATLAS_DOWNLOADS_DIR="tmp/atlas_downloads" # optional download directory | ||
| CONFIG_PATH="configs/config.development.json" # optional path to Atlas config file |
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.
Just noting I'm not seeing the new DR env vars we expect here.
recovery/main.gowith sample disaster recovery script (for handling data restore via snapshot and regional outage scenarios)