-
Notifications
You must be signed in to change notification settings - Fork 23
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
REALMC-12577: Add Data API Config support to the CLI #302
Conversation
With the import support PR in, these tests should pass on evergreen now, so I'm opening this up for review |
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 😎 looks pretty 🧼
@@ -34,6 +34,7 @@ type AppStructureV2 struct { | |||
Sync SyncStructure `json:"sync,omitempty"` | |||
Secrets SecretsStructure `json:"secrets,omitempty"` | |||
LogForwarders []map[string]interface{} `json:"log_forwarders,omitempty"` | |||
DataAPIConfig map[string]interface{} `json:"data_api_config,omitempty"` |
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] as a more general question, when do we need to create a struct and when can we just use map[string]interface{}
?
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 honest answer is "I don't know" 😅 but from looking around, it seems like we use structs for entities with more complicated file nesting. As an example, all of the info contained in a data source is spread between two directories and up to 4 different files, so in that case, it makes sense to be able to organize everything in a struct. For the Data API Config, all of the info is in just one JSON file, so using a map felt fine enough
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 good!!
JIRA: https://jira.mongodb.org/browse/REALMC-12577
This depends on https://github.com/10gen/baas/pull/6496 to be merged before tests will passThis has been merged