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

fix: add status code handling for delete endpoints #1074

Closed
wants to merge 3 commits into from
Closed

fix: add status code handling for delete endpoints #1074

wants to merge 3 commits into from
+64 −0

Conversation

hirosassa
Copy link
Contributor

@hirosassa hirosassa commented May 5, 2022 •
edited
Loading

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:

  • 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
  • Ensure the tests and linter pass
  • Appropriate docs were updated (if necessary)

hirosassa
hirosassa commented May 5, 2022
View reviewed changes
go/rtl/auth.go Outdated
Comment on lines 121 to 123
if res.StatusCode == 204 { // for delete endpoints there's no response body
return nil
}
Copy link
Contributor Author

@hirosassa hirosassa May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This change is the essential part. The others comes from go fmt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change the comment to "204 No Content. DELETE endpoints returns response with no body"?

@hirosassa hirosassa mentioned this pull request May 5, 2022
Closed
@hirosassa
Copy link
Contributor Author

Hi, could you review this changes?

@hirosassa
Copy link
Contributor Author

@jeremytchang Hi, could you review this change?

@hirosassa
Copy link
Contributor Author

@jkaster Hi, cloud you review this PR?

jeremytchang
jeremytchang approved these changes Aug 16, 2022
View reviewed changes
Copy link
Collaborator

@jeremytchang jeremytchang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LG%C. Nitpick on the comment.

Good catch, though, which endpoint did this fail on?

We have integration tests which delete dashboard, user, and look, and they all pass.
We may need an additional integration test on the endpoint that caused this.

go/rtl/auth.go Outdated
Comment on lines 121 to 123
if res.StatusCode == 204 { // for delete endpoints there's no response body
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change the comment to "204 No Content. DELETE endpoints returns response with no body"?

@hirosassa
Copy link
Contributor Author

@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
Copy link
Contributor Author

hirosassa commented Aug 16, 2022 •
edited
Loading

@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
Copy link
Contributor Author

integration test added in 3a4cdcc

@jeremytchang jeremytchang self-assigned this Aug 17, 2022
@hirosassa
Copy link
Contributor Author

hirosassa commented Aug 17, 2022 •
edited
Loading

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

@hirosassa
Copy link
Contributor Author

@jeremytchang Could you approve to run CI?
#1074 (comment)

jeremytchang
jeremytchang previously approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@jeremytchang jeremytchang left a comment

Choose a reason for hiding this comment

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

LGTM if integration tests pass

@hirosassa
Copy link
Contributor Author

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

@jkaster
Copy link
Contributor

jkaster commented Sep 13, 2022

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

@jkaster
Copy link
Contributor

jkaster commented Sep 13, 2022

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

@jeremytchang
Copy link
Collaborator

Rebasing branch to see if it fixes the CI issue.

@hirosassa
Copy link
Contributor Author

Hmmm. Required Checks is failed, too.

@jeremytchang jeremytchang mentioned this pull request Oct 17, 2022
Merged
@jeremytchang
Copy link
Collaborator

jeremytchang commented Oct 17, 2022 •
edited
Loading

Hey @hirosassa! Our CI can only run on branches in our repo because of security reasons. I've copied your changes to this PR #1193. When we squash merge the PR, i'll add you as co-author of the squashed commit.

@jeremytchang jeremytchang force-pushed the fix-delete-user-attribute-group-value branch from e5d3545 to 9534bd8 Compare October 17, 2022 21:59
jeremytchang added a commit that referenced this pull request Oct 18, 2022
@jeremytchang @hirosassa
fix: Handle Go SDK 204 no responses correctly (#1193)
ba7ded8 
Changes copied from approved PR from forked repo #1074

Original commit message:
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.

Co-authored-by: hirohito-sasakawa <hirohito-sasakawa@m3.com>
Co-authored-by: hirosassa <hiro.sassa@gmail.com>
@hirosassa
Copy link
Contributor Author

I'll close this PR coz the problem is fixed in #1193.
@jeremytchang @jkaster Thank you for your support!

@hirosassa hirosassa closed this Oct 21, 2022
@jessicasomaiya jessicasomaiya mentioned this pull request Feb 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremytchang jeremytchang jeremytchang left review comments

Assignees

@jeremytchang jeremytchang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hirosassa @jkaster @jeremytchang