-
Notifications
You must be signed in to change notification settings - Fork 178
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
refactor: Migrates search index resource and data sources to new SDK #1536
Conversation
options := &matlas.ListOptions{ | ||
PageNum: d.Get("page_num").(int), | ||
ItemsPerPage: d.Get("items_per_page").(int), | ||
} |
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.
still pending what to do with page_num and items_per_page, but please review the rest of the 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.
will be done in another PR to better track changelog
|
||
mappingFieldJSON, err := json.Marshal(fields) | ||
return string(mappingFieldJSON), err | ||
func marshalSearchIndex(fields any) (string, error) { |
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.
why are we changing from []map[string]interface{}
to any
?
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.
maybe better []map[string]any
?
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 method is just realling calling json.Marshal that accepts any.
so i joined marshallSearchIndexAnalyzers and marshallSearchIndexMappingsField that accepts an array or a map into the same fuction, no need to have separate functions for this
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 could rename the function directly to something more representative, like toJsonString
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 like the idea of calling Marshal as there is an opossed method to unmarshal, and SearchIndex as it's meant to be used in this resource. so marshal as packaging the info (that turns out to be in json) and unmarshaling to unpackage it
@@ -258,10 +258,10 @@ An [Atlas Search analyzer](https://docs.atlas.mongodb.com/reference/atlas-search | |||
|
|||
* `token_filters` - Array containing zero or more token filters. Always require a type field, and some take additional options as well: | |||
```terraform | |||
"tokenFilters":{ | |||
"tokenFilters":[{ |
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 this PR is gonna be a breaking change?
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 agreed with @Zuhairahmed that this is not a breaking change but a documentation bug. In fact the title is fine "Array containing zero or more token filters" but the example is wrong
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
Description
Jira ticket: INTMDB-1195
Migrates search index resource and data sources to new SDK.
Type of change:
Required Checklist:
Further comments