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

Add content compression support to StyxServer object #595

Merged
merged 3 commits into from
Jan 24, 2020
Merged

Add content compression support to StyxServer object #595

merged 3 commits into from
Jan 24, 2020

Conversation

chrisgresty
Copy link
Contributor

See issue #593

dvlato
dvlato suggested changes Jan 20, 2020
View reviewed changes
Copy link
Contributor

@dvlato dvlato left a comment

Choose a reason for hiding this comment

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

I see some of these tests might be equivalent to the ones in CompressionSpec. Do you think we need both?

@chrisgresty
Copy link
Contributor Author

chrisgresty commented Jan 20, 2020 •
edited
Loading

I see some of these tests might be equivalent to the ones in CompressionSpec. Do you think we need both?

Probably worth leaving in a couple of basic tests to ensure that the new configuration has the desired effect here.

dvlato
dvlato reviewed Jan 22, 2020
View reviewed changes
Copy link
Contributor

@dvlato dvlato left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Would you like to introfduce some of the improvements you will have for sure made to the tests into CompressionSpec?

mikkokar
mikkokar approved these changes Jan 23, 2020
View reviewed changes
components/proxy/src/main/kotlin/com/hotels/styx/servers/StyxHttpServer.kt Outdated
@@ -79,6 +80,7 @@ private data class StyxHttpServerTlsSettings(
private data class StyxHttpServerConfiguration(
val port: Int,
val handler: String,
val compressResponses: Boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this defaults to false?

components/proxy/src/test/kotlin/com/hotels/styx/servers/StyxHttpServerTest.kt Outdated
.body("Hello, test!", UTF_8)
.build()

private val compressedResponse = HttpResponse.response(OK)
Copy link
Contributor

Choose a reason for hiding this comment

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

Static import response.

@mikkokar
Copy link
Contributor

I see some of these tests might be equivalent to the ones in CompressionSpec. Do you think we need both?

At the moment, yes. Because there are two separate ways to start servers. But we will, sooner or later (hope sooner), to eliminate the old way.

@chrisgresty chrisgresty merged commit 24b9e8e into ExpediaGroup:master Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvlato dvlato dvlato approved these changes

@mikkokar mikkokar mikkokar approved these changes

@Noddy76 Noddy76 Awaiting requested review from Noddy76

@OwenLindsell OwenLindsell Awaiting requested review from OwenLindsell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisgresty @mikkokar @dvlato