Skip to content
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

update valid owner check to page through teams #23

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

jhump
Copy link
Contributor

@jhump jhump commented Feb 11, 2020

The existing code only downloads 1 page of results when trying to list an organization's teams. By default, a page is only 30 items. So organizations with >30 teams may have some teams that are not visible to the check. This causes the checker to think a config is invalid even if it actually is valid.

This change (1) sets the requested page size to 100 (the max), to reduce GitHub API calls; (2) adds a cache of teams by org to the checker struct, again, to reduce API calls to GitHub; and (3) iterates until the response indicates that there are no more pages.

@jhump jhump force-pushed the jh/fix-too-many-teams-for-1-page branch from 7ef82ae to e843b7a Compare February 11, 2020 21:28
@jhump jhump force-pushed the jh/fix-too-many-teams-for-1-page branch from e843b7a to b5fcd4e Compare February 11, 2020 21:36
@jhump jhump requested a review from mszostok February 11, 2020 21:40
@jhump
Copy link
Contributor Author

jhump commented Feb 12, 2020

@mszostok, any chance you could review this fix?

I am in a GitHub organization that has enough teams that we just hit this: we added two new teams but this tool reported our CODEOWNERS as broken when we tried to refer to the new teams. And the reason is because the new teams ended up on "page 2" of team results.

@mszostok
Copy link
Owner

Hi @jhump sorry for such late response, somehow I didnt notice that 🤦‍♂

I will check that this weekend and do new release.

once again really sorry. I will update readme how you can ping me:

  • u can find me on kubernetes slack https://slack.k8s.io/, as mszostok
  • or email me szostok.mateusz@gmail.com

but I will also pay more attention to that repo and change notification.

@jhump
Copy link
Contributor Author

jhump commented Mar 13, 2020

@mszostok, no worries. I'm patient! :)

Copy link
Owner

@mszostok mszostok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mszostok mszostok merged commit 12fc383 into mszostok:master Mar 14, 2020
@mszostok mszostok added the bug Something isn't working label Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants