Skip to content

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

Merged
gmlewis merged 5 commits intogoogle:masterfrom
WillAbides:tokens
Aug 27, 2023
Merged

Remove dependency on "golang.org/x/oauth2"#2895
gmlewis merged 5 commits intogoogle:masterfrom
WillAbides:tokens

Conversation

@WillAbides
Copy link
Copy Markdown
Contributor

@WillAbides WillAbides commented Aug 22, 2023

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
Copy Markdown

codecov bot commented Aug 22, 2023 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

WillAbides commented Aug 22, 2023

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.

gmlewis
gmlewis approved these changes Aug 22, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

gmlewis commented Aug 22, 2023 •
edited
Loading

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.

@WillAbides WillAbides mentioned this pull request Aug 23, 2023
Closed
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 27, 2023
@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Aug 27, 2023

Thank you, @gabriel-samfira !
Merging.

@gmlewis gmlewis merged commit c36edbd into google:master Aug 27, 2023
@WillAbides WillAbides deleted the tokens branch August 27, 2023 16:33
@WillAbides WillAbides mentioned this pull request Aug 27, 2023
Merged
@gmlewis gmlewis mentioned this pull request Sep 5, 2023
Merged
@kamilsk kamilsk mentioned this pull request Sep 6, 2023
Closed
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
@WillAbides @gmlewis
Remove dependency on "golang.org/x/oauth2" (google#2895)
94747d6 
Fixes: google#2893.
@ashi009 ashi009 mentioned this pull request Nov 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis approved these changes

+1 more reviewer

@gabriel-samfira gabriel-samfira gabriel-samfira approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove dependency on golang.org/x/oauth2

3 participants

@WillAbides @gmlewis @gabriel-samfira