Added pagination support for /user/teams github api with link header . #274

toshi-miura · 2019-10-07T13:59:44Z

Description

Added handling of last page pointed out below to #102

Could we maybe use the Link values to make sure we only call up until the last page?

Test is done by rewriting the code with a smaller "per_page" value.

…roxy into ap-gh-pagination-with-lastpage

JoelSpeed

Looks good generally, I think there may be a few typos in the comments and I think the logs are potentially a bit too verbose, bear in mind that each of these log lines would be printed for every request

Thanks for picking this up from #102

JoelSpeed · 2019-10-07T14:11:34Z

providers/github.go

+			// 1. When paging is required (Example: When the data size is 100 and the page size is 99 or less)
+			// 2. When it exceeds the paging frame (Example: When there is only 10 records but the second page is called with a page size of 100)
+
+			// link herder at not last page


Typo I think

Suggested change

// link herder at not last page

// link header at not last page

JoelSpeed · 2019-10-07T14:11:44Z

providers/github.go

+
+			// link herder at not last page
+			// <https://api.github.com/user/teams?page=1&per_page=100>; rel="prev", <https://api.github.com/user/teams?page=1&per_page=100>; rel="last", <https://api.github.com/user/teams?page=1&per_page=100>; rel="first"
+			// link herder at last page (doesn't exist last info)


Typo again?

Suggested change

// link herder at last page (doesn't exist last info)

// link header at last page (doesn't exist last info)

JoelSpeed · 2019-10-07T14:12:19Z

providers/github.go

+			// link herder at last page (doesn't exist last info)
+			// <https://api.github.com/user/teams?page=3&per_page=10>; rel="prev", <https://api.github.com/user/teams?page=1&per_page=10>; rel="first"
+
+			logger.Printf("1:endpoint :[%s]",endpoint.String())


Not sure we need to log these? Could become very noisy in the logs as these would both be printed on every request

JoelSpeed · 2019-10-07T14:14:41Z

providers/github.go

+				logger.Printf("3:doesn't exist last page info. ")
+			}else{
+				last = i
+				logger.Printf("3:last page index  :[%d]", last)


Potentially over-logging here

Thank you for the review. How about fixing it (typo &over-logging)?

providers/github.go

JoelSpeed

Please rebase on master and add a note to the changelog for the entry and we can then get this merged!

@toshi-miura

====================================================== changelog note [oauth2-proxy#274](oauth2-proxy#274) Add github api pagination support (@toshi-miura ,@apratina) ====================================================== I didn't edit CHANGELOG.md. Since # 102 was taken over and the change difference of CHANGELOG.md was large

toshi-miura · 2019-10-09T20:41:07Z

@JoelSpeed

Please rebase on master and add a note to the changelog for the entry and we can then get this merged!

My statement was rebase, except for changes to the original author.

I didn't edit CHANGELOG.md.
Since # 102 was taken over and the change difference of CHANGELOG.md was large

providers/github.go

JoelSpeed · 2019-10-11T09:23:50Z

@toshi-miura I appreciate the diff was large, but you can add a new changelog comment now there's no merge to handle right? Since this is a functional change it needs a changelog entry before it can be merged

toshi-miura · 2019-10-11T10:10:37Z

@JoelSpeed

it needs a changelog entry

Does it mean to add to CHANGELOG.md?
Is it like the following?

Changes since v4.0.0
#227 Add Keycloak provider (@Ofinka)
#273 Support Go 1.13 (@dio)
#274 XXXXXXXXXXXXX (@xxxx)

JoelSpeed · 2019-10-11T10:36:32Z

Is it like the following?

Yep that's exactly what I'd like you to add, thank you :)

toshi-miura · 2019-10-11T10:57:01Z

Can't pull the latest master CHANGELOG.md because I am a child of # 102, is that okay?

toshi-miura · 2019-10-11T11:13:21Z

I would like to ask you what kind of cycle is it to make a release binary?
In my job, I'm wondering whether to use what I built or use this release build.

JoelSpeed

I think this is good, has it been tested in it's latest form?

toshi-miura · 2019-10-11T17:19:26Z

I ’m doing a manual test.
In my environment, there are only about 50 github teams, so I have the following two types of tests

Test with this source code (Paging logic doesn't work )
It's bad, but test with editing source code ("per_page": {"5"}, (Paging logic work ))

loshz

! small typo and then I think we're good to merge :D

loshz · 2019-10-11T17:23:54Z

CHANGELOG.md

@@ -5,6 +5,7 @@
 - [#227](https://github.com/pusher/oauth2_proxy/pull/227) Add Keycloak provider (@Ofinka)
 - [#273](https://github.com/pusher/oauth2_proxy/pull/273) Support Go 1.13 (@dio)
 - [#275](https://github.com/pusher/oauth2_proxy/pull/275) docker: build from debian buster (@syscll)
+- [#274](https://github.com/pusher/oauth2_proxy/pull/274)  Supports many github teams with api pagination support (@toshi-miura ,@apratina)


Suggested change

- [#274](https://github.com/pusher/oauth2_proxy/pull/274) Supports many github teams with api pagination support (@toshi-miura ,@apratina)

- [#274](https://github.com/pusher/oauth2_proxy/pull/274) Supports many github teams with api pagination support (@toshi-miura, @apratina)

Thanks for the review, fixed

CHANGELOG.md

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

toshi-miura · 2019-10-21T09:14:46Z

I'm sorry to be late

JoelSpeed

LGTM, thanks for your work and patience with our reviews @toshi-miura!

loshz

Once the changelog conflict is resolved this should be good to merge.

toshi-miura · 2019-10-24T10:28:30Z

Once the changelog conflict is resolved this should be good to merge.

I resolved the conflict.

JoelSpeed · 2019-11-14T14:25:56Z

I'm happy with this now, I've fixed up the changelog again, @syscll gonna override your requested changes as I can see this is now resolved

toshi-miura · 2019-11-26T08:40:46Z

@JoelSpeed

Thank you for the merge.

If the release schedule is decided,
I would be grateful if you would tell the release schedule

loshz · 2019-11-26T16:37:37Z

We are planning to do a release in the next few weeks. We're just finalising a few other PRs.

toshi-miura · 2019-11-26T22:56:13Z

Thank you very mush!

…way config (oauth2-proxy#274) Signed-off-by: bitliu <bitliu@tencent.com>

Akshay Pratinav and others added 4 commits March 12, 2019 21:24

add pagination support for /user/teams

3c19c36

limit => per_page

e73f650

change per_page value from 200 to 100

6d15fe0

Merge branch 'ap-gh-paginate' of https://github.com/apratina/oauth2_p…

1c36b5e

…roxy into ap-gh-pagination-with-lastpage

toshi-miura requested review from steakunderscore and loshz as code owners October 7, 2019 13:59

toshi-miura requested a review from a team October 7, 2019 13:59

JoelSpeed requested changes Oct 7, 2019

View reviewed changes

JoelSpeed reviewed Oct 7, 2019

View reviewed changes

providers/github.go Show resolved Hide resolved

JoelSpeed reviewed Oct 9, 2019

View reviewed changes

toshi-miura force-pushed the ap-gh-pagination-with-lastpage branch from b24a937 to ee6065d Compare October 9, 2019 19:47

toshi-miura force-pushed the ap-gh-pagination-with-lastpage branch from ee6065d to 31d7b61 Compare October 9, 2019 20:35

loshz suggested changes Oct 9, 2019

View reviewed changes

providers/github.go Outdated Show resolved Hide resolved

ReadAll() & Close() 　close together.

e71797b

add change log.

0d256a3

Merge branch 'master' into ap-gh-pagination-with-lastpage

eb1d64a

JoelSpeed previously approved these changes Oct 11, 2019

View reviewed changes

loshz suggested changes Oct 11, 2019

View reviewed changes

Fix typo.

610ee6d

toshi-miura dismissed JoelSpeed’s stale review via 610ee6d October 11, 2019 17:31

JoelSpeed reviewed Oct 12, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

e34f18e

Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>

Merge branch 'master' into ap-gh-pagination-with-lastpage

535f6b8

JoelSpeed previously approved these changes Oct 21, 2019

View reviewed changes

JoelSpeed requested a review from loshz October 23, 2019 09:06

loshz reviewed Oct 23, 2019

View reviewed changes

Merge branch 'master' into ap-gh-pagination-with-lastpage

eb89047

toshi-miura dismissed JoelSpeed’s stale review via eb89047 October 24, 2019 02:23

toshi-miura requested a review from loshz October 24, 2019 10:28

Merge branch 'master' into ap-gh-pagination-with-lastpage

6326660

JoelSpeed force-pushed the ap-gh-pagination-with-lastpage branch from 6d98808 to 6326660 Compare November 14, 2019 14:22

JoelSpeed merged commit 9fdb8ac into oauth2-proxy:master Nov 14, 2019

Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024

feat: add support for hgctl gateway-config to retrieve higress gate…

0acb04f

…way config (oauth2-proxy#274) Signed-off-by: bitliu <bitliu@tencent.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added pagination support for /user/teams github api with link header . #274

Added pagination support for /user/teams github api with link header . #274

toshi-miura commented Oct 7, 2019

JoelSpeed left a comment

JoelSpeed Oct 7, 2019

JoelSpeed Oct 7, 2019

JoelSpeed Oct 7, 2019

JoelSpeed Oct 7, 2019

toshi-miura Oct 7, 2019 •

edited

Loading

JoelSpeed left a comment

toshi-miura commented Oct 9, 2019

JoelSpeed commented Oct 11, 2019

toshi-miura commented Oct 11, 2019

JoelSpeed commented Oct 11, 2019

toshi-miura commented Oct 11, 2019

toshi-miura commented Oct 11, 2019

JoelSpeed left a comment

toshi-miura commented Oct 11, 2019

loshz left a comment

loshz Oct 11, 2019

toshi-miura Oct 11, 2019

toshi-miura commented Oct 21, 2019

JoelSpeed left a comment

loshz left a comment

toshi-miura commented Oct 24, 2019

JoelSpeed commented Nov 14, 2019

toshi-miura commented Nov 26, 2019

loshz commented Nov 26, 2019

toshi-miura commented Nov 26, 2019

	// link herder at not last page
	// link header at not last page

	// link herder at last page (doesn't exist last info)
	// link header at last page (doesn't exist last info)

	- [#274](https://github.com/pusher/oauth2_proxy/pull/274) Supports many github teams with api pagination support (@toshi-miura ,@apratina)
	- [#274](https://github.com/pusher/oauth2_proxy/pull/274) Supports many github teams with api pagination support (@toshi-miura, @apratina)

Added pagination support for /user/teams github api with link header . #274

Added pagination support for /user/teams github api with link header . #274

Conversation

toshi-miura commented Oct 7, 2019

Description

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Oct 7, 2019

Choose a reason for hiding this comment

JoelSpeed Oct 7, 2019

Choose a reason for hiding this comment

JoelSpeed Oct 7, 2019

Choose a reason for hiding this comment

JoelSpeed Oct 7, 2019

Choose a reason for hiding this comment

toshi-miura Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

toshi-miura commented Oct 9, 2019

JoelSpeed commented Oct 11, 2019

toshi-miura commented Oct 11, 2019

JoelSpeed commented Oct 11, 2019

toshi-miura commented Oct 11, 2019

toshi-miura commented Oct 11, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

toshi-miura commented Oct 11, 2019

loshz left a comment

Choose a reason for hiding this comment

loshz Oct 11, 2019

Choose a reason for hiding this comment

toshi-miura Oct 11, 2019

Choose a reason for hiding this comment

toshi-miura commented Oct 21, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

loshz left a comment

Choose a reason for hiding this comment

toshi-miura commented Oct 24, 2019

JoelSpeed commented Nov 14, 2019

toshi-miura commented Nov 26, 2019

loshz commented Nov 26, 2019

toshi-miura commented Nov 26, 2019

toshi-miura Oct 7, 2019 •

edited

Loading