sdk-codegen icon indicating copy to clipboard operation
sdk-codegen copied to clipboard

Published 20 hours ago • verified publisher icon looker-open-source

fix: add status code handling for delete endpoints

Open hirosassa opened this issue 3 years ago • 13 comments

I added status code handling in the Go SDK to deal with delete endpoints' response body. In Looker API, delete endpoints call returns empty response body with status code 204 when the API call is succeeded. Such API call will be failed with EOF error in the current implementation because of reading empty response body.

Developer Checklist ℹ️

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • [x] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • [x] Ensure the tests and linter pass
  • [x] Appropriate docs were updated (if necessary)

hirosassa avatar May 05 '22 00:05 hirosassa

Hi, could you review this changes?

hirosassa avatar May 20 '22 21:05 hirosassa

@jeremytchang Hi, could you review this change?

hirosassa avatar May 27 '22 20:05 hirosassa

@jkaster Hi, cloud you review this PR?

hirosassa avatar Aug 16 '22 19:08 hirosassa

@jeremytchang Thank you for the review!

I'll create a example to reproduce the error and add integration test if it is needed (maybe this weekend).

hirosassa avatar Aug 16 '22 20:08 hirosassa

@jeremytchang Here's the minimum example of reproducing error (need to run this on the local PC).

https://go.dev/play/p/EFRxP79d9z9

This example fails with failed to delete: EOF at line 77.

I'll implement integration test for this next.

hirosassa avatar Aug 16 '22 23:08 hirosassa

integration test added in 3a4cdcc

hirosassa avatar Aug 16 '22 23:08 hirosassa

@jeremytchang Could you approve to run github actions workflow for confirming the IT run correctly?

hirosassa avatar Aug 17 '22 22:08 hirosassa

@jeremytchang Could you approve to run CI? https://github.com/looker-open-source/sdk-codegen/pull/1074#issuecomment-1218537111

hirosassa avatar Sep 08 '22 02:09 hirosassa

Thanks Jeremy!

CI is failed by permission denied on the step of docker pull, but I didn't any change about this part. Do you have any idea for how to solve this? https://github.com/looker-open-source/sdk-codegen/actions/runs/3012134238/jobs/4901724930

hirosassa avatar Sep 13 '22 07:09 hirosassa

22.8 shouldn't be part of the integration test run now. I've requested a re-run of all CI tests

jkaster avatar Sep 13 '22 19:09 jkaster

Hmm. Looks like we have an issue in CI right now

jkaster avatar Sep 13 '22 19:09 jkaster

Rebasing branch to see if it fixes the CI issue.

jeremytchang avatar Sep 13 '22 20:09 jeremytchang

Hmmm. Required Checks is failed, too.

hirosassa avatar Sep 13 '22 20:09 hirosassa

Hey @hirosassa! Our CI can only run on branches in our repo because of security reasons. I've copied your changes to this PR https://github.com/looker-open-source/sdk-codegen/pull/1193. When we squash merge the PR, i'll add you as co-author of the squashed commit.

jeremytchang avatar Oct 17 '22 21:10 jeremytchang

I'll close this PR coz the problem is fixed in https://github.com/looker-open-source/sdk-codegen/pull/1193. @jeremytchang @jkaster Thank you for your support!

hirosassa avatar Oct 21 '22 22:10 hirosassa