Skip to content

Conversation

@cbullinger
Copy link
Collaborator

  • examples/performance/archive/main.go & helper functions
  • updated existing examples to use new loader that supports different env files

Copy link
Collaborator

@dacharyc dacharyc left a 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 🙇‍♀️

@cbullinger cbullinger requested a review from dacharyc August 20, 2025 11:23
Copy link
Collaborator

@dacharyc dacharyc left a 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 {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

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
// considers docThreshold = 100000 criteria 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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@dacharyc dacharyc left a 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"
Copy link
Collaborator

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.

@cbullinger cbullinger requested a review from dacharyc August 21, 2025 18:29
Copy link
Collaborator

@dacharyc dacharyc left a 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"
Copy link
Collaborator

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.

@cbullinger cbullinger merged commit 5e033c0 into mongodb:main Aug 21, 2025
2 checks passed
@cbullinger cbullinger deleted the docsp-46224-46228 branch August 21, 2025 19:29
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.

2 participants