Skip to content

Conversation

@abeyad
Copy link

@abeyad abeyad commented Aug 8, 2016

Fixes netty4 module's CORS config to use defaults for http.cors.allow-methods and http.cors.allow-headers when none are specified. Currently, there was a bug where if no value was specified for http.cors.allow-methods or http.cors.allow-headers, an empty value was used instead of the actual defaults.

Note that this bug does not exist in the netty3 module.

@abeyad abeyad added >bug review :Distributed Coordination/Network Http and internode communication implementations v5.0.0-beta1 labels Aug 8, 2016
Copy link
Member

Choose a reason for hiding this comment

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

I find tests that reuse local variables, e.g., because they are testing different setups, quite confusing. Can we break this into two tests or at least use some scoping so that it's less confusing?

@jasontedor
Copy link
Member

It looks good, I left a comment about the test.

@abeyad
Copy link
Author

abeyad commented Aug 10, 2016

@jasontedor Thanks for the review. I pushed e09791c to split the tests into two and I made a small change that enabled refactoring these tests to not create the more heavy-weight Netty4HttpServerTransport class but instead just build the CorsConfig.

@jasontedor
Copy link
Member

@abeyad There are merge conflicts now, almost surely from the integration earlier this morning of the Expect: 100-continue test. Can you merge master in, resolve the conflicts, and push to this branch?

@abeyad abeyad force-pushed the fix/cors-config-defaults branch from e09791c to 7a9a7a1 Compare August 10, 2016 20:38
@abeyad
Copy link
Author

abeyad commented Aug 10, 2016

@jasontedor I've resolved all conflicts with master, the latest has been pushed up

@jasontedor
Copy link
Member

LGTM.

@abeyad
Copy link
Author

abeyad commented Aug 10, 2016

thanks for the review @jasontedor

@abeyad abeyad merged commit 04e8272 into elastic:master Aug 10, 2016
@abeyad abeyad deleted the fix/cors-config-defaults branch August 10, 2016 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Distributed Coordination/Network Http and internode communication implementations v5.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants