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

#213 Add WSClientConfig support to the Akka Http backend #227

Closed

Conversation

2m
Copy link

@2m 2m commented Jan 23, 2018

This adds WSClientConfig support for the Akka Http backend. The only two supported config parameters are userAgent and compressionEnabled. New tests are added that tests these parameters in AHC and Akka Http backends for both Java and Scala APIs.

Other config parameters need corresponding features from #207.

This PR builds on top of #208 and therefore should be rebased and retargeted to master after #208 is merged.

Fixes #213

() -> req,
(ua) -> req.addHeader(UserAgent.create(
// FIXME JAVA API expose ProductVersion.parseMultiple in Java API
scala.collection.JavaConverters.seqAsJavaList(ProductVersion.parseMultiple(ua)).toArray(new ProductVersion[0])
Copy link
Author

Choose a reason for hiding this comment

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

This will be cleaned up a bit after akka/akka-http#1807

@2m 2m force-pushed the wip-akka-http-ws-config-2m branch from 5c60576 to 47565c8 Compare January 23, 2018 16:07
@@ -83,7 +84,7 @@ trait AkkaServerProvider extends BeforeAfterAll {
new HttpsConnectionContext(context)
}

def clientHttpsContext() = {
def clientHttpsContext(akkaSslConfig: Option[AkkaSSLConfig] = None): HttpsConnectionContext = {
Copy link
Member

Choose a reason for hiding this comment

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

prefer overloads than default values? (bin compat)

Copy link
Author

Choose a reason for hiding this comment

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

This is in testing machinery. So does not really matter.

*
* @return a WSClientConfig configuration object.
*/
def forConfig(): WSClientConfig =
Copy link
Member

Choose a reason for hiding this comment

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

for? from? Confusing name

Copy link
Author

Choose a reason for hiding this comment

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

I agree, but this is how it was done while creating AHC config. Therefore I decided to keep the consistency.

Copy link
Member

Choose a reason for hiding this comment

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

i see ok

case HttpEncodings.deflate ⇒
Deflate
case _ ⇒
NoCoding
Copy link
Member

Choose a reason for hiding this comment

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

should this log that unknown encoding?

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

hehe so weird / cool to see akka http routes in here, great :)

Copy link
Member

@ktoso ktoso 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 as far as I can tell. Good that the config object was pretty small :)

@2m
Copy link
Author

2m commented Feb 18, 2018

Closing the akka-http backend related tickets for now. These will be handled in the future.

@2m 2m closed this Feb 18, 2018
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.

2 participants