Skip to content
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

CLOUDP-67056: Implement search API in the go client #111

Merged
merged 8 commits into from Jul 15, 2020
Merged

Conversation

gssbzn
Copy link
Collaborator

@gssbzn gssbzn commented Jul 14, 2020

Description

Add search support

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Required Checklist:

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

robcarlan-mlab
robcarlan-mlab previously approved these changes Jul 14, 2020
Copy link
Contributor

@robcarlan-mlab robcarlan-mlab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! just a couple of var names that I noticed

mongodbatlas/search.go Outdated Show resolved Hide resolved
mongodbatlas/search.go Outdated Show resolved Hide resolved
robcarlan-mlab
robcarlan-mlab previously approved these changes Jul 14, 2020
Copy link
Contributor

@robcarlan-mlab robcarlan-mlab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gssbzn gssbzn requested a review from themantissa July 14, 2020 15:59
@gssbzn
Copy link
Collaborator Author

gssbzn commented Jul 14, 2020

@themantissa ready for you

themantissa
themantissa previously approved these changes Jul 14, 2020
Copy link
Collaborator

@themantissa themantissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question as it seemed an odd one to leave out but otherwise LGTM so approving. Will add DoU devs for review.

CreateIndex(context.Context, string, string, *SearchIndex) (*SearchIndex, *Response, error)
UpdateIndex(context.Context, string, string, string, *SearchIndex) (*SearchIndex, *Response, error)
DeleteIndex(context.Context, string, string, string) (*Response, error)
ListAnalyzers(context.Context, string, string, *ListOptions) ([]*SearchAnalyzer, *Response, error)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if leaving Update Analyzers out was intentional? (https://docs.atlas.mongodb.com/reference/api/fts-analyzers-update-all/)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat intentional, there are plans to deprecate Custom Analyzers so I'm trying to limit us to only read them (if ever needed)

@PacoDw PacoDw self-requested a review July 15, 2020 14:16
mongodbatlas/search.go Show resolved Hide resolved
PacoDw
PacoDw previously approved these changes Jul 15, 2020
Copy link
Contributor

@PacoDw PacoDw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thanks 👍

@gssbzn gssbzn merged commit bdd98ee into master Jul 15, 2020
@gssbzn gssbzn deleted the CLOUDP-67056 branch July 15, 2020 18: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.

None yet

4 participants