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

add OrganizationsListOptions for filtering organization list results #144

Merged
merged 4 commits into from Oct 5, 2020

Conversation

sunny-b
Copy link
Contributor

@sunny-b sunny-b commented Oct 1, 2020

Description

Addresses #143

The Ops Manager and Atlas API supports filtering Organization list results by the name, however this Go client does not support that at the moment. This PR would add that functionality and adds a test to help ensure it works.

I recognize this is a breaking change to the interface so I'm open to discussion on a better way to do it. Though I saw this pattern in ContainersListOptions which is where I adopted it from.

type ContainersListOptions struct {
ProviderName string `url:"providerName,omitempty"`
ListOptions
}

Link to any related issue(s):

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

Further comments

@gssbzn gssbzn added the breaking Pull requests that breaks backwards compatibility label Oct 2, 2020
Copy link
Collaborator

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

Hi @sunny-b, thanks for your contribution, I'm happy to break the client to fix this, just one small comment in case you want to improve OM support as well.
I'll let @themantissa have final say on this breaking change

@@ -40,6 +40,12 @@ type OrganizationsServiceOp service

var _ OrganizationsService = &OrganizationsServiceOp{}

// OrganizationsListOptions filtering options for organizations
type OrganizationsListOptions struct {
Name string `url:"name,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you mention Ops Manager, Ops manager also has a boolean, includeDeletedOrgs, I've been wanting to fix this Listing option to add that one in particular but since we are breaking this I wouldn't mind taking the extra step here.
If you decide to add it be aware that it defaults to true so omitempty may get tricky

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using a bool pointer can help get around the omitempty business. According to the docs, if the includeDeletedOrgs param is missing in the url, then it'll still default to true.

image

I added in that change and made a test to show that setListOptions will properly omit an empty bool pointer. Please let me know if you need anything else for 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.

thanks for adding this, yes, making it a pointer should work

themantissa
themantissa previously approved these changes Oct 2, 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.

LGTM as I don't think this impacts us particularly. However added the DoU devs for a quick review to check just in case. FYI @coderGo93 and @leofigy Thanks all!

@sunny-b
Copy link
Contributor Author

sunny-b commented Oct 5, 2020

Anyway of getting the appropriate reviews for this? I would love for this feature to get merged in. We're planning on using this Go client at work and this is a much needed feature for us.

Copy link
Contributor

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

gssbzn commented Oct 5, 2020

@sunny-b feel free to add update https://github.com/mongodb/go-client-mongodb-ops-manager with this change as well, we may be doing this anyway in the coming days

@gssbzn gssbzn merged commit cefc31a into mongodb:master Oct 5, 2020
@sunny-b
Copy link
Contributor Author

sunny-b commented Oct 6, 2020

@gssbzn Yeah that's a good idea. Though you'd need to put out a new release of this package in order to update Ops Manager as it uses the various ListOptions from this package.

@gssbzn
Copy link
Collaborator

gssbzn commented Oct 6, 2020

@sunny-b on the ops-manager-client we usually update to master (go get -u go.mongodb.org/atlas@master), not ideal but we are ok with it since we manage both packages. releasing a new version for the atlas client takes a bit more time as we coordinate this with the team in charge of the terraform provider. Don't feel any pressure on doing this, we'll address this on our next update of the ops manager client dependency for atlas but I can't promise any dates right

@sunny-b
Copy link
Contributor Author

sunny-b commented Oct 6, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Pull requests that breaks backwards compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants