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

Remove dependency on "golang.org/x/oauth2" #2895

Merged
merged 5 commits into from
Aug 27, 2023
Merged

Conversation

WillAbides
Copy link
Contributor

Resolves #2893

This removes the primary module's dependency on "golang.org/x/oauth2" and replaces it with a RoundTripper that adds an "Authorization" header with the value "Bearer ".

TestNewTokenClient now does an http request and checks for the "Authorization" header.

I moved roundTripperFunc from test code to prod because it was useful for creating the RoundTripper.

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #2895 (9454fd3) into master (38ca69f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2895   +/-   ##
=======================================
  Coverage   98.10%   98.10%           
=======================================
  Files         142      142           
  Lines       12340    12347    +7     
=======================================
+ Hits        12106    12113    +7     
  Misses        159      159           
  Partials       75       75           
Files Changed Coverage Δ
github/github.go 98.00% <100.00%> (+0.02%) ⬆️

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 22, 2023

Do you want to update all the files that mention oauth2, including documentation, while you're here?

$ rg -l oauth2
README.md
go.sum
scrape/go.sum
example/newreposecretwithlibsodium/go.sum
example/newreposecretwithlibsodium/go.mod
go.mod
github/github.go
github/doc.go
example/go.sum
example/go.mod
example/tokenauth/main.go
example/newfilewithappauth/main.go
github/github_test.go

@WillAbides
Copy link
Contributor Author

These are the remaining oauth2 mentions:

$ rg -l oauth2
README.md
github/github.go
github/doc.go
example/go.sum
example/go.mod
example/newfilewithappauth/main.go
scrape/go.sum

The first three are docs and code comments about how to authenticate. I think it's appropriate to keep oauth2 there because go-github doesn't offer an Enterprise equivalent of NewTokenClient.

example/* is similar to the doc issue except this time it's actually using oauth2 as an example. Fortunately example is its own module so we don't need to worry about extra imports from there.

scrape/go.sum has an oauth2 line because scrape/go.mod requires "github.com/google/go-github/v54 v54.0.0" which still requires oauth2. That should go away the first time go mod tidy is run in scrape/ after this change is released.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Normally the maintainers perform the "go mod tidy", and we have been using "go mod tidy -compat=1.17" for a long while now.

But all the tests seem to be passing, so maybe we are still fine. This will be an experiment. 😄

Thank you, @WillAbides !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 22, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 22, 2023

scrape/go.sum has an oauth2 line because scrape/go.mod requires "github.com/google/go-github/v54 v54.0.0" which still requires oauth2. That should go away the first time go mod tidy is run in scrape/ after this change is released.

Thanks for the reminder! I should do that anyway.

Doh, I gotcha. I misread that before, but now I understand. Thanks.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 27, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Aug 27, 2023

Thank you, @gabriel-samfira !
Merging.

@gmlewis gmlewis merged commit c36edbd into google:master Aug 27, 2023
9 checks passed
@WillAbides WillAbides deleted the tokens branch August 27, 2023 16:33
@WillAbides WillAbides mentioned this pull request Aug 27, 2023
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on golang.org/x/oauth2
3 participants