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

feat: prevent body double-read in googleapi.CheckResponse #1602

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

bjohnso5
Copy link
Contributor

Call into non-reading variant of CheckResponse added here.

Fixes googleapis/google-cloud-go#11458 (part 2 of 2).

@bjohnso5 bjohnso5 requested review from a team as code owners January 16, 2025 17:33
@quartzmo quartzmo added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 16, 2025
@quartzmo quartzmo self-assigned this Jan 16, 2025
@quartzmo
Copy link
Member

Setting do not merge label pending publication of change in googleapis/google-api-go-client#2964.

@quartzmo quartzmo added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 21, 2025
@quartzmo
Copy link
Member

This is waiting on googleapis/google-api-go-client#2965, at which point https://pkg.go.dev/google.golang.org/api/googleapi should list CheckResponseWithBody.

@codyoss codyoss added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 22, 2025
@bjohnso5
Copy link
Contributor Author

bjohnso5 commented Jan 23, 2025

@quartzmo do I need to update the go.mod in the showcase to point at the 0.218.0 release of google.golang.org/api?

Edited to add: based on the failure in the integration tests output, I think the answer is yes, so I've pushed a change to include that chore.

@codyoss codyoss removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 23, 2025
@codyoss
Copy link
Member

codyoss commented Jan 23, 2025

Looks like we need to add a new leakchecker skip. maybe transport/controlbuf.go. The file that holds the items to ignore is leakcheck.go. Other than that this looks good to me. This is likely do to the new version on grpc

@codyoss codyoss merged commit bd42bc4 into googleapis:main Jan 24, 2025
7 checks passed
@codyoss
Copy link
Member

codyoss commented Jan 24, 2025

@bjohnso5 Thank you for all your work on these contributions! We will get our generator updated. I expect the new code won't land until early next week though just FYI.

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.

compute: field validation errors are stripped of their detail
3 participants