Skip to content
This repository was archived by the owner on May 21, 2025. It is now read-only.

Re-add handling of single value headers #55

Merged
merged 18 commits into from
Feb 4, 2020
Merged

Conversation

nsarychev
Copy link
Contributor

@nsarychev nsarychev commented Feb 4, 2020

Issue #, if available:

Description of changes:
This commit d7b2b4e removed setting single-value headers, which caused all of our tests against gin break. This PR adds back the code to populate single-value headers as it did before. Please consider this for your review.

Note: There are commits in history from my previous merged PR #33. I was using same fork just updated it.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -154,7 +154,7 @@ var _ = Describe("ResponseWriter tests", func() {
Expect(err).To(BeNil())

// Headers are not written to `Headers` field

Choose a reason for hiding this comment

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

This comment makes it seem like this was intentional. If we want to use Headers and MultiValueHeaders then this comment does not make sense to keep.

Copy link

@kheppenstall kheppenstall left a comment

Choose a reason for hiding this comment

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

LGTM

@nsarychev nsarychev changed the title Revert handling of single value headers Re-add handling of single value headers Feb 4, 2020
@nsarychev
Copy link
Contributor Author

@sapessi Could you please review this when you get a chance? Thank you!

Copy link
Collaborator

@sapessi sapessi left a comment

Choose a reason for hiding this comment

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

Looks good.

@sapessi sapessi merged commit 8650bc2 into awslabs:master Feb 4, 2020
@sapessi
Copy link
Collaborator

sapessi commented Feb 4, 2020

As soon as the CI for the merge completes I'll tag v0.6.0

@nsarychev
Copy link
Contributor Author

Thank you for such a quick response @sapessi!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants