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.

Sign up for GitHub

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

Jump to bottom

Integrate go-gh API package #5614

Merged
merged 10 commits into from Jun 23, 2022
Merged

Integrate go-gh API package #5614

merged 10 commits into from Jun 23, 2022

Conversation

samcoe
Copy link
Contributor

@samcoe samcoe commented May 11, 2022 •
edited

This PR replaces our API package implementation with implementation backed by go-gh API package. This has resulted in changes across numerous files, because of this I have noted design choices and questions inline with the code.

cc #5560

@samcoe samcoe self-assigned this May 11, 2022
samcoe
samcoe commented May 11, 2022
View reviewed changes
api/queries_pr.go Outdated Show resolved Hide resolved
@samcoe samcoe force-pushed the go-gh-integration branch 5 times, most recently from 523a40d to 4073d37 Compare May 12, 2022 15:07
@samcoe samcoe mentioned this pull request May 13, 2022
Merged
@samcoe samcoe force-pushed the go-gh-integration branch 7 times, most recently from 5ab69cd to 4cf7b35 Compare May 23, 2022 15:07
samcoe
samcoe commented May 23, 2022
View reviewed changes
api/client.go Show resolved Hide resolved
samcoe
samcoe commented May 23, 2022
View reviewed changes
cmd/gh/main.go Outdated Show resolved Hide resolved
samcoe
samcoe commented May 23, 2022
View reviewed changes
cmd/gh/main.go Outdated Show resolved Hide resolved
samcoe
samcoe commented May 23, 2022
View reviewed changes
pkg/cmd/api/api.go Outdated Show resolved Hide resolved
samcoe
samcoe commented May 23, 2022
View reviewed changes
pkg/cmd/factory/default.go Outdated Show resolved Hide resolved
samcoe
samcoe commented May 23, 2022
View reviewed changes
pkg/cmd/factory/http.go Outdated Show resolved Hide resolved
@samcoe samcoe marked this pull request as ready for review May 23, 2022 15:33
@samcoe samcoe requested a review from a team as a code owner May 23, 2022 15:33
@samcoe samcoe requested review from mislav and removed request for a team May 23, 2022 15:33
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI May 23, 2022
mislav
mislav suggested changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thanks for the gargantuan body of work!

Could there be a way to continue generating a "cached" API client from the go-gh client? That way we wouldn't have to restructure so much code to allow for explicitly passing a cached client around 🤔

api/client_test.go Outdated Show resolved Hide resolved
cmd/gh/main.go Outdated Show resolved Hide resolved
internal/authflow/flow.go Show resolved Hide resolved
internal/featuredetection/feature_detection_test.go Outdated Show resolved Hide resolved
internal/update/update_test.go Outdated Show resolved Hide resolved
pkg/cmd/factory/http_test.go Outdated Show resolved Hide resolved
pkg/cmd/issue/delete/delete_test.go Outdated Show resolved Hide resolved
@samcoe samcoe mentioned this pull request Jun 14, 2022
Merged
@samcoe
Copy link
Contributor Author

samcoe commented Jun 14, 2022

@mislav I addressed the PR feedback. The tests are going to continue to fail until cli/go-gh#49 gets merged in as that PR has some of the changes you requested.

The only thing that has not been addressed is the caching client issue. I am open to ideas here as to how to generate a cache client from a normal client, but right now I am running into an order of operations issue. We determine the cache key from the request headers which do not get set right away, they get set somewhere in the transport chain, so adding a caching layer to the top of the transport chain will result in a cache key that is derived from blank headers and won't be unique in some situations, for example if a user refreshes their auth token.

@samcoe
Copy link
Contributor Author

samcoe commented Jun 14, 2022

@mislav This is now ready for re-review. After the changes to go-gh I was able to remove most of the changes around cached clients which makes this PR a bit less complex.

@samcoe samcoe requested a review from mislav June 14, 2022 19:25
mislav
mislav approved these changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Fantastic! Thank you for the hard work

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Jun 20, 2022
@samcoe samcoe merged commit 074ed50 into trunk Jun 23, 2022
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Jun 23, 2022
@samcoe samcoe deleted the go-gh-integration branch June 23, 2022 03:05
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Jul 12, 2022
@mislav mislav mentioned this pull request Jul 14, 2022
Merged
@mislav mislav mentioned this pull request Jul 25, 2022
Closed
@williammartin williammartin mentioned this pull request Apr 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mislav mislav mislav approved these changes

Assignees

@samcoe samcoe

Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@samcoe @mislav