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

fix http result errors by wrapping the upstream error, allowing for isNack #764

Merged
merged 5 commits into from
Mar 31, 2022

Conversation

n3wscott
Copy link
Member

@n3wscott n3wscott commented Mar 23, 2022

fixes: #762

sbgreene1307 and others added 2 commits March 22, 2022 17:55
Signed-off-by: Stephen Greene <stephen.greene@sap.com>
…sNack

Signed-off-by: Scott Nichols <n3wscott@chainguard.dev>
@n3wscott
Copy link
Member Author

@sbgreene1307 would you mind reviewing this? Thanks for your unit test and your issue!!

@n3wscott n3wscott force-pushed the sbgreene-762-proto-result-nonnack branch from a43e5cb to 55e88fc Compare March 24, 2022 22:18
Signed-off-by: Scott Nichols <n3wscott@chainguard.dev>
@n3wscott n3wscott force-pushed the sbgreene-762-proto-result-nonnack branch from 55e88fc to b060ffb Compare March 24, 2022 23:24
Copy link
Contributor

@sbgreene1307 sbgreene1307 left a comment

Choose a reason for hiding this comment

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

A couple minor things to button up

@@ -113,6 +114,9 @@ func (t *tapHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(500)
return
}
if t.statusCode > 299 {
w.WriteHeader(t.statusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot to add a return here which I think is what is causing the "superfluous" log line that you see in the tests. Do you want to just add that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do!

@@ -83,6 +86,7 @@ func ClientDirect(t *testing.T, tc DirectTapTest, copts ...client.Option) {
t.Errorf("expected ACK, got %s", result)
}
} else if !cloudevents.ResultIs(result, tc.wantResult) {
t.Errorf("result.IsUndelivered = %v", cloudevents.IsUndelivered(result))
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed now as well

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind the extra output

Signed-off-by: Scott Nichols <n3wscott@chainguard.dev>
@n3wscott n3wscott force-pushed the sbgreene-762-proto-result-nonnack branch from ecbe51c to cfc6cef Compare March 31, 2022 15:46
…or before the handler

Signed-off-by: Scott Nichols <n3wscott@chainguard.dev>
@n3wscott n3wscott merged commit 9380b70 into cloudevents:main Mar 31, 2022
@n3wscott n3wscott deleted the sbgreene-762-proto-result-nonnack branch March 31, 2022 16:02
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.

HTTP Send results in IsUndelivered true for non-2XX responses
2 participants