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 large github organisations #8846

Merged
merged 1 commit into from Jul 31, 2017

Conversation

Projects
None yet
5 participants
@skwashd
Contributor

skwashd commented Jul 12, 2017

The current github integration only retrieves the first page of results when looking up user's team membership. For most organisations this isn't a problem. When you have a large organisation with over 100 teams not all the teams are retrieved. This patch solves this problem by doing the following things:

  • Modify HttpGet() return to include the response headers
  • Look up all the teams the user is a member of
@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jul 12, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jul 12, 2017

CLA assistant check
All committers have signed the CLA.

@marji

This comment has been minimized.

Show comment
Hide comment
@marji

marji Jul 12, 2017

I have tested this change and it works.

  1. I had grafana 4.3.2 installed, and had trouble with github authentication - some users from the same team and organisation could not log in.
  2. I built grafana from this PR and copied over the new grafana-server binary over the existing installation
  3. restarted grafana-server and users can now login with no problems.

marji commented Jul 12, 2017

I have tested this change and it works.

  1. I had grafana 4.3.2 installed, and had trouble with github authentication - some users from the same team and organisation could not log in.
  2. I built grafana from this PR and copied over the new grafana-server binary over the existing installation
  3. restarted grafana-server and users can now login with no problems.
Support large github organisations
* Add new HttpGetResponse struct type
* Modify HttpGet() return to use HttpGetResponse
* Look up _all_ the teams the user is a member of
@skwashd

This comment has been minimized.

Show comment
Hide comment
@skwashd

skwashd Jul 29, 2017

Contributor

@torkelo I have implemented the change you requested. Given #6347 says 1.7 is the minimum version, I implemented @miguelbernadi's suggestion of using named return values.

Is there anything else you want to see done before this is merged? I have signed the CLA and the CI bot is happy.

Contributor

skwashd commented Jul 29, 2017

@torkelo I have implemented the change you requested. Given #6347 says 1.7 is the minimum version, I implemented @miguelbernadi's suggestion of using named return values.

Is there anything else you want to see done before this is merged? I have signed the CLA and the CI bot is happy.

@torkelo torkelo merged commit 0c70d27 into grafana:master Jul 31, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@torkelo torkelo added this to the 5.0 milestone Jul 31, 2017

@skwashd skwashd deleted the skwashd:github-teams branch Jul 31, 2017

@skwashd

This comment has been minimized.

Show comment
Hide comment
@skwashd

skwashd Aug 1, 2017

Contributor

@torkelo thanks for merging the PR. Would you consider a new patch that contains a hacked up version of the new HttpGet() logic in SocialGithub.FetchTeamMemberships() as a backport for 4.x?

Contributor

skwashd commented Aug 1, 2017

@torkelo thanks for merging the PR. Would you consider a new patch that contains a hacked up version of the new HttpGet() logic in SocialGithub.FetchTeamMemberships() as a backport for 4.x?

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Aug 2, 2017

Member

Not Not i want this in stable release branch, but master / nightly is basically the same as 4.4.x as most of our current unstable / unfinished stuff is in develop branch

Member

torkelo commented Aug 2, 2017

Not Not i want this in stable release branch, but master / nightly is basically the same as 4.4.x as most of our current unstable / unfinished stuff is in develop branch

@skwashd

This comment has been minimized.

Show comment
Hide comment
@skwashd

skwashd Aug 2, 2017

Contributor

Thanks for the clarification. It makes sense. For now we'll carry the patch internally against 4.4.x until 5.0 is released.

Contributor

skwashd commented Aug 2, 2017

Thanks for the clarification. It makes sense. For now we'll carry the patch internally against 4.4.x until 5.0 is released.

@torkelo torkelo modified the milestones: 5.0, 4.5.0 Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment