PermsSyncer: refactor fetchUserPermsViaExternalAccounts results#44251
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff a55b91d...172db6b.
|
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
kopancek
left a comment
There was a problem hiding this comment.
Looks good to me, but I would maybe go even further and apply the same behavior to repo syncing, see my comment (non-blocking for this PR).
There was a problem hiding this comment.
We also sync permissions based on repositories, see method syncRepoPerms
Should we extend this type to include that as well? E.g. something like:
| type fetchUserPermsViaExternalAccountsResults struct { | |
| repoIDs []uint32 | |
| subRepoPerms map[api.ExternalRepoSpec]*authz.SubRepoPermissions | |
| } | |
| type PermsResults struct { | |
| repoIDs []uint32 | |
| userIDs []uint32 | |
| subRepoPerms map[api.ExternalRepoSpec]*authz.SubRepoPermissions | |
| } |
There was a problem hiding this comment.
I don't think so - syncRepoPerms intentionally does not return anything because it does not have another aggregation layer the way user perms does
There was a problem hiding this comment.
i.e.
- User perms: syncUserPerms -> fetchUserPermsViaExternalAccounts -> provider
- Repo perms: syncRepoPerms -> provider
There was a problem hiding this comment.
User and repo perms also return very different things, i.e. in user-centric sync we only return repo lists and in repo-centric sync we only return user lists, creating a struct to carry both would just be a mutually exclusive result set
850a68d to
9d688b9
Compare
9d688b9 to
1b5569d
Compare
Refactors fetchUserPermsViaExternalAccounts to always return a result type, and collects the multiple return values into this one result type. This does not cause any behaviour changes because the callers check for error first anyway.
I want to eventually have each authz provider report separate states from each job back to PermsSyncer, so this result type will be extended: https://github.com/sourcegraph/sourcegraph/pull/44257
Test plan
Existing tests pass