-
Notifications
You must be signed in to change notification settings - Fork 25
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
(DOCSP-32047) New push/pull Admin API endpoints #625
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.
It looks like there are some discrepancies here with what the API is actually returning. I started listing them out, but they got bigger when I got to data_sources
. Ping me on Slack if you'd like and I can share my response object with you, or you can try hitting the pull
endpoint and see what you get back.
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 handful of questions, including what appears to me to be some bigger discrepancies in the data_sources.
$ref: "#/components/schemas/CustomUserDataConstructor" | ||
data_api_config: | ||
$ref: "#/components/schemas/DataApiConfig" | ||
data_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.
According to our API reference, the DataSourceConstructor
schema contains one of AtlasClusterConstructorWithRules
or AtlasFederatedInstanceConstructor
.
My response body for data_sources
included the details in the AtlasClusterConstructorWithRules
schema, AND the ServiceResponse
schema (I had type
, name
, version
, but I did not have _id
). Should both of those things be included in the DataSourceConstructorSchema
?
source/openapi-admin-v3.yaml
Outdated
@@ -5603,7 +5688,24 @@ components: | |||
error_code: | |||
type: string | |||
description: The error type. | |||
AtlasClusterConstructorWithRules: | |||
allOf: | |||
- $ref: "#/components/schemas/AtlasClusterConstructor" |
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.
My response body did not match the AtlasClusterConstructor
schema config
object. My config only had clusterName
, readPreference
, and wireProtocolEnabled
. It did not have clusterName
, clusterId
, clusterType
, groupName
, etc. My response body had a sync
config outside of this config object - it wasn't in the Atlas cluster config. I'm not sure if this schema is used elsewhere, but it doesn't match what I got from the pull endpoint.
description: |- | ||
If true, clients may [connect to the app over the | ||
MongoDB Wire | ||
Protocol](https://www.mongodb.com/docs/atlas/app-services/mongodb/wire-protocol/#connect-over-the-wire-protocol). | ||
FlexibleSync: |
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 notice we don't have required/optional on these fields. I'm missing collection_queryable_fields_names
and is_recovery_mode_disabled
.
I also don't see anything about data ingest, and realize I don't know if you configure data ingest, how it shows up in the config object. I looked around and we don't seem to have anything documented for that. But I assume it shows up in the config somehow.
Should we be more explicit about which of these fields may be absent? For a lot of our other config options, if there are no values, we just get an empty array - for example, my pull had empty arrays for triggers, values, log_forwarders, etc. So I might assume that collection_queryable_fields_names
would show up as an empty array, and I'm not sure why is_recovery_mode_disabled
is omitted.
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.
Added a bunch of required annotations. I think we'll want to do a separate ticket to look at ingest / sync config stuff
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, @nlarew !
type: array | ||
items: | ||
$ref: "#/components/schemas/AuthProviderConstructor" | ||
custom_user_data: |
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.
My custom_user_data
and data_api_config
responses are both null. We say in the API doc that these are required properties, and that they use these objects. Should we explicitly state they can be null?
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 catch - not actually required and can be null
if unconfigured.
source/openapi-admin-v3.yaml
Outdated
type: object | ||
required: | ||
- clusterName | ||
- clusterType |
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.
We list clusterType
as a required field, but it's not present in the pull
response I got. I am guessing this is not required, or is maybe required for push but not guaranteed to be present in pull?
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 think this one snuck in - it's not present anywhere we use this schema. Removed!
type: string | ||
description: The role's unique ObjectId identifier. | ||
- $ref: "#/components/schemas/RoleConstructor" | ||
RoleConstructor: |
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 response I got from the pull
endpoint had an object that isn't present in our RoleConstructor here: document_filters
- which has a read
bool and a write
object where I've got "owner_id": "%%user.id"
I'm guessing that's optional but I would expect to see it covered 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.
I've also got a read
bool (which I guess is probably an expression?) at the same level as this write
bool, which we don't list 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.
I believe document_filters
is sync-specific and we don't have good Admin API docs for that atm. I added a reusable schema for them (can reuse when we doc the sync config better) and included that in RoleConstructor
+ added the non-sync read
permission + noted that you should use document_filters
if sync is enabled.
@@ -7298,9 +7528,12 @@ components: | |||
|
|||
- the relationship may point to many foreign documents. | |||
- the local field must be defined as an array in the collection schema. | |||
SchemaDefinition: | |||
type: object | |||
description: A valid [schema](https://www.mongodb.com/docs/realm/schemas) for the collection. |
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.
Is there a reason we don't document the schema object here? The page we link out to has a "Define a Schema" section with an example object: https://www.mongodb.com/docs/atlas/app-services/schemas/#define-a-schema
It struck me as weird that I can view all the other object details here in the API spec, but not schema.
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.
Historically because it's highly dependent on the user's definition. That said there are some invariants that we can include here and use them in a recursive definition - good catch!
@@ -6851,7 +7056,7 @@ components: | |||
type: string | |||
collection_queryable_fields_names: | |||
type: object | |||
description: |- | |||
description: |- |
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.
My Sync config has a handful of fields that aren't represented here (type
, development_mode_enabled
, service_name
, database_name
). I notice there's still an outstanding comment from the last review where you suggested a separate ticket to look at ingest/sync config stuff. Does that ticket exist somewhere so we can capture these details?
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.
…shpull
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 looks great, Nick! I know this is a massive thing that touched a lot of individual components, and I really appreciate your thoroughness here. Thanks so much!
✨ Staging URL: https://preview-mongodbmongodb.gatsbyjs.io/atlas-app-services/master/ 🪵 Logs |
Pull Request Info
/services/config
endpoints and combines them with the correct/services/{serviceId}/config
- this fixes the lint error I was getting!Jira
Staged Changes