-
Notifications
You must be signed in to change notification settings - Fork 33
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
support user model and listing groups #92
base: master
Are you sure you want to change the base?
Conversation
teamcity/group_test.go
Outdated
groupListBefore, err := client.Groups.List() | ||
require.NoError(t, 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.
Curious about why do we have to List()
before arranging the test setup?
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.
See also the comment about List
on the implementation.
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.
In cases of parallel testing, other group tests may add undefined behavior. Perhaps I am missing something and these tests are run sequentially?
teamcity/group.go
Outdated
@@ -78,3 +95,13 @@ func (s *GroupService) Delete(key string) error { | |||
err := s.restHelper.delete(locator, "group") | |||
return err | |||
} | |||
|
|||
// List - List of all groups | |||
func (s *GroupService) List() (*GroupList, 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.
For the List()
feature - At first this may seem an easy win to be able to return all items at once.
However, this project as whole has no support for paginated responses, and I think we should think more about implementing such support before returning potentially "unbounded" data sets.
Reasoning for this: In my experience, exposing such methods in a client is almost like an invitation for consumers/clients to abuse these calls.
I'd suggest to remove this unbounded List()
unless there's a very strong case behind it to be supported, so we can resume the discussion about it.
For reference, see "Beware of partial answers..."
https://www.jetbrains.com/help/teamcity/rest-api.html#API+Client+Recommendations
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 can support splitting with ?locator=count:1,start: 3
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.
func (s *GroupService) List(start, count 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.
I'd more inclined if we could provide this kind of support seamless for the client users, without needing to expose the pagination details. The response from such paginated methods would then be a sort of enumerator that could fetch all the results independently from the original method call.
This is something I thought about designing but haven't done the work yet.
I guess I'd be okay with the List()
method without any pagination for this resource, at least until not we have a more general solution for the whole client.
return &out, err | ||
} | ||
|
||
// GroupAddByID - Add User with userID to Group with groupKey |
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 API is an interesting point about the N:N relations.
I believe on TeamCity's API side these resources are managed on the /user/groups/...
, but wouldn't it make more sense (or be more intuitive) to have these operations on the "Group" side of things? That is, if we had to choose one side to manage it.
I'll take a closer look at how the API behaves and maybe we can sketch out a design here on this PR from the result of this discovery, wdyt?
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.
When considering the problem with List()
, an idea appeared instead of using the Groups field - to replace it with the func GetGroupListBy...(userLocator locator, groupKey string, start, count, int)
, similarly for groups the ability to get a list of group members func GetMemberListBy...(groupLocator locator, userLocator locator, start, count int)
, similar for Roles
and Properites
, wdyt?
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.
Regarding which resource should manage the addition and removal, unfortunately, I only found this method
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.
Regarding which resource should manage the addition and removal, unfortunately, I only found this method
Regardless of the underlying raw API of TeamCity, the general design idea of this client is to provide a user-friendly, albeit programmatic interface for that interaction. Most of this driving factor comes from the usage by the Terraform Provider, where if we simply translate the TeamCity API to client operations 1:1, the ultimate caller code gets very clunky.
IMO it makes more sense to either:
- Add users to groups via the
GroupService
instead ofUserService
. Somehow I think it's way more intuitive and discoverable to perform operations these operations on the "group" than on the "user", if we have to choose one. From the TeamCity UI, we can do both ways, however. - Create a third service,
UserGroupMembershipService
and do these explicit membership operations there.
This PR contains support for managing Users, also userGroups list