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

Support the mod_proxy ProxPassReverseCookieDomain directive #1309

Merged
merged 1 commit into from Dec 24, 2015

Conversation

occelebi
Copy link

Hey,

I've added ProxyPassReverseCookieDomain directive in mod_proxy.

Regards,
Cem

@bmjen
Copy link
Contributor

bmjen commented Dec 21, 2015

@occelebi Thanks for the contribution! This PR looks good. It needs a squash, but also.. how come you reviewed the spec test? It would be great if you got that test working so we can validate that the template gets populated correctly.

@occelebi
Copy link
Author

@bmjen, It has been squashed. However I do not know how to write proper test for this case as I do not know ruby very well.

@@ -20,7 +20,12 @@
<Location <%= proxy['path']%>>
<%- if not proxy['reverse_cookies'].nil? -%>
<%- Array(proxy['reverse_cookies']).each do |reverse_cookies| -%>
ProxyPassReverseCookiePath <%= reverse_cookies['path'] %> <%= reverse_cookies['url'] %>
<%- if reverse_cookies['path'] -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is misaligned, can you add a space here.

@bmjen
Copy link
Contributor

bmjen commented Dec 22, 2015

@occelebi It's pretty simple to add a test case to an existing test. All we're expecting is that if a parameter is set, that the code does what it's intended.

Basically you can add another entry here:

'reverse_cookies' => [{
'path' => '/a',
'url' => 'http://backend-a/',
}],
with the domain key.

              'reverse_cookies' => [
                {
                  'path' => '/a',
                  'url'    => 'http://backend-a/',
                },
                {
                  'domain' => 'foo',
                  'url'         => 'http://foo',
               }
             ],

Then you can add another line below here:

it { is_expected.to contain_concat__fragment('rspec.example.com-proxy').with_content(
/ProxyPassReverseCookiePath\s+\/a\s+http:\/\//) }
to match the string output you'd expect based on what you set the domain to.

      it { is_expected.to contain_concat__fragment('rspec.example.com-proxy').with_content(
              /ProxyPassReverseCookieDomain\s+foo\s+http:\/\/foo/) }

@occelebi
Copy link
Author

@bmjen Thank you for your guidance. I've pushed new version.

But I'm still in need of clarification for test of ProxyPassReverseCookiePath:

          'reverse_cookies' => [
            {
              'path' => '/a',
              'url'    => 'http://backend-a/',
            },

According to that I expected something like that:

   it { is_expected.to contain_concat__fragment('rspec.example.com-proxy').with_content(
          /ProxyPassReverseCookiePath\s+\/a\s+http:\/\/backend-a\//) }

@bmjen
Copy link
Contributor

bmjen commented Dec 24, 2015

Thanks @occelebi ! That test has a loose regex match and passes because it only matches up to the "http://", your suggestion would have been better.

bmjen added a commit that referenced this pull request Dec 24, 2015
Support the mod_proxy ProxPassReverseCookieDomain directive
@bmjen bmjen merged commit a389184 into puppetlabs:master Dec 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants