[Stash] Implement gitprovider interfaces #114
Conversation
souleb
force-pushed
the
go-stash-provider-implem
branch
from
a267817 to
7c83923
Compare
Interfaces. Get and list is implemented for orgs(stash projects) and groups(stash teams) and users The missing implementations return "not implemented" for the moment as agreed. Signed-off-by: Soule BA <[email protected]>
souleb
force-pushed
the
go-stash-provider-implem
branch
2 times, most recently
from
23a0397 to
8b2a757
Compare
| teams := make([]gitprovider.Team, len(apiObjs)) | ||
| for i, apiObj := range apiObjs { | ||
| // Get detailed information about individual teams (including members). | ||
| // Slug is validated to be non-nil in ListGroupMembers. |
There was a problem hiding this comment.
I have to be honest, this is very expensive to do...
You are making n+1 queries to get the list of teams, without any pagination or limits.
There was a problem hiding this comment.
N+1 call yes, but the call to get teams is paginated. Then for each team the call to get permissions also is paginated. Page numbers is set to c.maxPages and is 6 by default. The default limit is set to 25 entries per page.
There was a problem hiding this comment.
As discussed, removed maxPages. I kept a limit of 25 entries per page
souleb
force-pushed
the
go-stash-provider-implem
branch
3 times, most recently
from
b85e279 to
5294401
Compare
| return &list.Paging, nil | ||
| }) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
What does the error look like here if this fails?
There was a problem hiding this comment.
It returns the first error encountered, be it request error or unmarshalling. I probably should use the multierror to get all pages errors.
souleb
force-pushed
the
go-stash-provider-implem
branch
4 times, most recently
from
3d9f65b to
84cfc44
Compare
Add client to create keys, commit, create branches and pull requests for the repositories. Add capabilities for reconciling repositories, deploy keys and teams access. Signed-off-by: Soule BA <[email protected]>
souleb
force-pushed
the
go-stash-provider-implem
branch
from
84cfc44 to
3a0139f
Compare
| @@ -187,3 +188,23 @@ func (s *GroupsService) ListGroupMembers(ctx context.Context, groupName string, | |||
|
|
|||
| return m, nil | |||
| } | |||
|
|
|||
| // AllGroupMembers retrieves all group members. | |||
There was a problem hiding this comment.
Not a blocker for this branch, but all the functions that are very costly like this should probably have a comment warning of this?
| // Create a new repo | ||
| repo, err := client.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{ | ||
| Description: gitprovider.StringVar(defaultDescription), | ||
| // Default visibility is private, no need to set this at least now |
There was a problem hiding this comment.
I'm confused by this comment, it says there's no need to set it, but it appears to set it?
There was a problem hiding this comment.
I'm approving this for now, but I think we need to reconsider how go-git-providers gets data like this, there are too many "load all the pages of Object, then load all Objects individually" without controls in place to limit the requests, or warnings that this is extremely expensive.
There was a problem hiding this comment.
LGTM
Thanks @souleb for all the hard work and thanks @bigkevmcd for the review 🏅
PS. Currently we don't use the List functions in flux bootstrap, we act on a single repo and deploy key. Going to merge this so we can move forward with the Stash implementation, but we should create an issue and address the pagination problem for all providers.
#102
Signed-off-by: Soule BA [email protected]
Description
Adds full implementation of gitprovider interfaces.
With this PR comes:
** the git client clone on disk. In the future shall be in-memory.
Test results
https://github.com/souleb/go-git-providers/runs/3964397313?check_suite_focus=true