-
Notifications
You must be signed in to change notification settings - Fork 6
DOCSP-46224: Add archive examples for Scalability page (Arch Center) #53
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
Conversation
f484d2c to
7662323
Compare
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.
Back to you with a collection of questions & nits for your consideration 🙇♀️
usage-examples/go/atlas-sdk-go/examples/monitoring/metrics_disk/main.go
Outdated
Show resolved
Hide resolved
generated-usage-examples/go/atlas-sdk-go/project-copy/examples/performance/archiving/main.go
Show resolved
Hide resolved
…to docsp-46224-46228
2c3493f to
117ee36
Compare
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.
Back to you with some little things we need to clean up, and a couple of Qs that I might be overthinking entirely. 🤷♀️
| ) | ||
|
|
||
| // Config holds the configuration for connecting to MongoDB Atlas | ||
| type Config struct { |
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.
Unless I'm reading this wrong, it seems we still have a discrepancy between the keys we're expecting here in the loadconfig function and the keys we're showing in the configs in the configs directory. The config.example.com contains an "ATLAS_CLUSTER_NAME": "<clusterName>", key, while the other two configs (development and production) contain "ATLAS_PROJECT_NAME": "Customer Portal - Dev",. It looks like in the loader here, we're expecting it to be ATLAS_CLUSTER_NAME.
We're now also looking for an ATLAS_HOSTNAME key but don't show that in any of the config files.
Seems we need to do a little more aligning across these sources.
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.
currently, hostname gets programmatically derived from process_id. the other confusing parts were from changes that shouldn't have been in this PR (trying to align with TF and the other Arch Center examples). I've removed those configs 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.
I'm a little confused about the purpose of this file at this point.
The .env.example has a couple of additional keys now (ATLAS_DOWNLOADS_DIR and CONFIG_PATH) which we don't do anything with here. I do see us using those around the app directly with os.Getenv.
But here, we're creating a Secrets struct which we are explicitly passing around the app, instead of using os.Getenv where we want to use these values.
So I guess my question is - should we be creating a larger struct that holds all of the .env vars that we pass around as needed, instead of directly accessing os.Getenv in the files where we're using them? Or is there a reason we're handling the secrets differently than the other env keys? And if there is a reason we're handling the secrets differently, should this file be renamed to loadsecrets.go since it explicitly isn't handling the other env keys?
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.
no, but only because they're serving different purposes. I've left this and the secrets-specific struct alone for now.
| MONGODB_ATLAS_SERVICE_ACCOUNT_ID=<your_mdb_service_account_id> | ||
| MONGODB_ATLAS_SERVICE_ACCOUNT_SECRET=<your_mdb_service_account_secret> | ||
| ATLAS_DOWNLOADS_DIR=tmp/atlas_downloads # optional directory for downloads | ||
| CONFIG_PATH=./configs/config.<env>.json # path to corresponding config file No newline at end of 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.
This CONFIG_PATH key kinda makes me 😢
We're providing configs in the configs directory that map to the env names. So could we just keep track of what .env we're using and set the corresponding config path without requiring a dev to specify a value here?
Related, I see the configs directory is not present in the generated-usage-examples ... project copy - is that intentional? If so, I wonder why we provide multiple configs in the source.
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.
applied your suggestion and load it from env
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.
Question: we tell users in main that:
// NOTE: The actual implementation of this function would involve more complex logic
// to determine which collections are eligible for archiving.
So on the one hand, I think the code in main is just intended to show me a pattern for how I could structure my code to do the configuring. BUT since we're calling out that this is basically a simplistic implementation and users should be using more complex logic to determine which collections are eligible, that invites me as a dev to go look at this code and see what it's doing and try to figure out why my logic needs to be more complex.
By including this in an internal folder, I assume this is some sort of guts or helper function that should be abstracted away from me in the example context. So I wonder whether we should be providing more context in main for what this is doing and why I don't need to look at it, i.e.:
// NOTE: This simplistic example iterates through collections and
// considersdocThreshold = 100000criteria for archiving. Your
// implementation will vary based on your project's needs.
Or whether this should be moved out of internal and into some file alongside main in the archiving directory to indicate that the user could examine this implementation and build on it for their purposes.
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.
good point. I'll move it to main()
| } | ||
| } | ||
|
|
||
| // ValidateCandidate ensures the archiving candidate meets requirements |
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.
Whose requirements are we meeting here? is this something that Atlas requires in order to perform the configuration, or is this some sort of second check that a candidate that has met our criteria for archiving is actually suitable for archiving? i.e. are these requirements that I as a user am intended to define/modify, or is this the structure a candidate has to have in order to successfully configure archiving?
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's a demo, so it's just showing some example requirements that you can update or fully remove in your own implementation. I've updated the description and comments so this is hopefully more clear
| # Secrets | ||
| .env | ||
| # Secrets (keep example) | ||
| tmp/.env |
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.
Nit: some of these .gitignore entries seem duplicative with the tmp and temp entries down below.
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.
only if you're using the pre-defined default; I was covering my bases from earlier testing
|
|
||
| # Logs | ||
| # config files (keep example) | ||
| configs |
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.
Q about this one - this PR contains a configs directory here with two net new configs - config.development.json and config.production.json. If this configs is intended to prevent committing files other than configs/config.example.json - it doesn't seem to have worked.
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.
the .development and .production.json aren't really needed (yet?). I assume they'll be used as snippets in the near future, but they shouldn't have been introduced in this PR. I've removed
5eb777a to
ef1eb45
Compare
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.
Couple of small things to clean up, but the only really blocking one is the ValidateCandidate call being used in two places. Very close!
| func main() { | ||
| if err := godotenv.Load(); err != nil { | ||
| log.Printf("Warning: .env file not loaded: %v", err) | ||
| envFile := ".env.development" |
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.
Looks like this is still pointing to .env.development instead of .env.production. If we want to change this to be consistent with the other examples, I guess we'd need to rerun Bluehawk to regenerate the copied app.
usage-examples/go/atlas-sdk-go/examples/performance/archiving/main.go
Outdated
Show resolved
Hide resolved
…main.go Co-authored-by: Dachary <dc@dacharycarey.com>
Co-authored-by: Dachary <dc@dacharycarey.com>
…code-examples into docsp-46224-46228
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.
LGTM 🎉 And I also tested the generated project and everything there looks good too. One open question for future consideration but mainly just wondering what people might expect.
| func main() { | ||
| if err := godotenv.Load(); err != nil { | ||
| log.Printf("Warning: .env file not loaded: %v", err) | ||
| envFile := ".env.development" |
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 guess it makes sense for the snippet to match the docs page, but if we are proposing the artifact repo is a clonable resource, I'm not sure it makes sense in that context. Unless we're suggesting this example should only be valid in development, I'm thinking it makes sense to assume that people who are looking around and trying things in the artifact repo may be doing it decoupled from the companion docs page. So I'm wondering if we would want to do something with states here to make the snippet version use development, and the copied artifact repo project use production. 🤷♀️
I suspect in either case, some number of people will be surprised by this. I expect we would get complaints if it doesn't match the docs, or also if it's not internally consistent. So this is truly just an open question to consider for the future - non-blocking at all for this PR.
examples/performance/archive/main.go& helper functions