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

fsttest: discard header changes after send #124

Merged
merged 3 commits into from
Jun 28, 2024

Conversation

fgsch
Copy link
Member

@fgsch fgsch commented Jun 27, 2024

This should correctly ignore headers set too late to make it into the response.

@fgsch fgsch force-pushed the fgsch/discard-changes-after-sent branch 2 times, most recently from 9aa9278 to 7d8cdd8 Compare June 27, 2024 10:22
This should correctly ignore headers set too late to make it into
the response.
@fgsch fgsch force-pushed the fgsch/discard-changes-after-sent branch from 7d8cdd8 to 0e1556c Compare June 27, 2024 10:28
Copy link
Member

@joeshaw joeshaw left a comment

Choose a reason for hiding this comment

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

LGTM but I have a request for a comment explaining why we return a clone.

Comment on lines 32 to 35
if !r.headersDone {
return r.HeaderMap
}
return r.HeaderMap.Clone()
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding a comment here to explain the purpose? I'm not sure it's obvious without additional context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good point and I agree. Being part of our test harness, I was initally being more liberal about the lack of comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will update the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Let me know if that works for you.

@cee-dub cee-dub changed the title Discard changes after we send the response fsttest: discard header changes after send Jun 27, 2024
Copy link
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

Could have given better feedback the first time through. Apologies for the back and forth!

fsttest/recorder.go Outdated Show resolved Hide resolved
@fgsch fgsch force-pushed the fgsch/discard-changes-after-sent branch from c7df4c4 to 44ebfb9 Compare June 28, 2024 16:00
@fgsch fgsch force-pushed the fgsch/discard-changes-after-sent branch from 44ebfb9 to 21c408f Compare June 28, 2024 16:01
@fgsch
Copy link
Member Author

fgsch commented Jun 28, 2024

Updated to address feedback. PTAL

Copy link
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

LGTM!

@fgsch fgsch merged commit a5aefc0 into main Jun 28, 2024
10 checks passed
@fgsch fgsch deleted the fgsch/discard-changes-after-sent branch June 28, 2024 20:20
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.

3 participants