-
Notifications
You must be signed in to change notification settings - Fork 47
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
Implement organizations API. #477
Conversation
Adds the APIs for managing the organization: - Get/List/Update organizations - List/Create/Delete organization invitations - List/Delete organization members - Add/Remove organization member role assignments (API docs: https://www.elastic.co/guide/en/cloud/current/Organizations.html)
0f3d6bd
to
5f5f60d
Compare
if got != nil && !assert.Equal(t, test.want, *got) { | ||
t.Error(err) | ||
} | ||
}) |
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 a nit, but it seems like we wouldn't verify want == got
if got is nil
. It seems like we would maybe want to fail the test in that case?
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.
Yeah, true. It is done this way to ensure the 'want' is not checked when an error is expected. But I think I can write this in a clearer way:
if test.err != "" {
assert.EqualError(t, err, test.err)
assert.Nil(t, got)
} else {
assert.NoError(t, err)
assert.Equal(t, test.want, *got)
}
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 updated all tests to use this format.
if err != nil && !assert.EqualError(t, err, test.err) { | ||
t.Error(err) | ||
} | ||
if got != nil && !assert.Equal(t, test.want, *got) { |
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.
Just commenting here so that if a change is made for the prior comment RE: failing the test if got == nil
it also gets made 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.
Makes sense and is well tested. One minor question with some tests, otherwise LGTM
Clearly check either the error case or the successful result case.
59be954
to
4ceec3a
Compare
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
Adds the APIs for managing an Elastic cloud organization and its members.
(https://www.elastic.co/guide/en/cloud/current/Organizations.html)
This change is needed to implement RBAC support in terraform-provider-ec
Description
Related Issues
Motivation and Context
How Has This Been Tested?
Types of Changes
Readiness Checklist