Skip to content

Integrate go-gh API package - #5614

Merged
samcoe merged 10 commits into
trunkfrom
go-gh-integration
Jun 23, 2022
Merged

Integrate go-gh API package#5614
samcoe merged 10 commits into
trunkfrom
go-gh-integration

Conversation

@samcoe

@samcoe samcoe commented May 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread api/queries_pr.go Outdated
@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
Comment thread api/client.go
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread cmd/gh/main.go Outdated
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread cmd/gh/main.go Outdated
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread internal/authflow/flow.go
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread internal/featuredetection/feature_detection.go
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread pkg/cmd/api/api.go Outdated
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread pkg/cmd/api/api_test.go
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread pkg/cmd/factory/default.go Outdated
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread pkg/cmd/factory/http.go
samcoe
samcoe commented May 23, 2022
View reviewed changes
Comment thread pkg/cmd/pr/shared/templates.go Outdated
@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
mislav
mislav suggested changes Jun 13, 2022
View reviewed changes

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

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 🤔

Comment thread api/client_test.go Outdated
Comment thread cmd/gh/main.go Outdated
Comment thread internal/authflow/flow.go
Comment thread internal/featuredetection/feature_detection_test.go Outdated
Comment thread internal/update/update_test.go Outdated
Comment thread pkg/cmd/factory/http_test.go Outdated
Comment thread pkg/cmd/issue/delete/delete_test.go Outdated
@samcoe samcoe mentioned this pull request Jun 14, 2022
Merged
@samcoe

samcoe commented Jun 14, 2022

Copy link
Copy Markdown
Contributor Author

@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

samcoe commented Jun 14, 2022

Copy link
Copy Markdown
Contributor Author

@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

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

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

@samcoe
samcoe force-pushed the go-gh-integration branch from e896e8c to 3f7f89d Compare June 21, 2022 15:40
@samcoe
samcoe force-pushed the go-gh-integration branch from 3f7f89d to f50a0ae Compare June 21, 2022 15:45
@samcoe
samcoe merged commit 074ed50 into trunk Jun 23, 2022
@samcoe
samcoe deleted the go-gh-integration branch June 23, 2022 03:05
@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

1 more reviewer
@mislav mislav mislav approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@samcoe samcoe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samcoe @mislav @cliAutomation