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

Avoid duplicate scheme declaration when using proxy with SSL #396

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

jmcnatt
Copy link
Contributor

@jmcnatt jmcnatt commented Nov 20, 2021

Pull Request (PR) description

  • Added negation logic when scheme key is present when iterating through the proxy hash.
  • Added additional test for tomcat_native_ssl without proxy settings.

This Pull Request (PR) fixes the following issues

Fixes #395

@root-expert
Copy link
Member

I think the actual fix for this to add a ! in the condition here.

In fact, that was the case before the template was converted to epp and by accident ! was removed.

References, here and here

Can you also rebase with master branch to fix conflicts and the failing specs?
Adding some specs for this so it won't break in the future would be awesome!

@root-expert root-expert added bug Something isn't working needs-tests labels Nov 25, 2021
@root-expert root-expert changed the title Issue 395 Fix Avoid duplicate scheme declaration when using proxy with SSL Nov 25, 2021
@jmcnatt
Copy link
Contributor Author

jmcnatt commented Nov 29, 2021

Makes sense! I'll rebase, update the PR, and see if I can author a spec for this condition.

@jmcnatt
Copy link
Contributor Author

jmcnatt commented Dec 2, 2021

Updated the PR to use the ! logic instead of unless.

I was able to write a spec context for specifying tomcat_native_ssl: true with and without proxy. Looking for duplicate scheme="https" may be difficult without checking for exact contents of the entire file (at least when using with_content()).

Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

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

Hello, please perform a rebase instead of a merge commit 😄

@jmcnatt jmcnatt force-pushed the issue_395_fix branch 3 times, most recently from 0f55205 to ad17bfa Compare December 2, 2021 22:53
@jmcnatt
Copy link
Contributor Author

jmcnatt commented Dec 3, 2021

I rebased and removed the spacing and quotation change.

Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

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

Please squash your commits before we merged this 😄

And thanks for the contribution!

@root-expert root-expert merged commit 6fb7fb9 into voxpupuli:master Dec 3, 2021
@jmcnatt jmcnatt deleted the issue_395_fix branch December 3, 2021 20:46
@jmcnatt jmcnatt restored the issue_395_fix branch December 3, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scheme written twice in server.xml when using tomcat_native_ssl and supplying proxy settings
2 participants