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

Adding resolver entries to streamhost.erb #1407

Merged
merged 1 commit into from
Aug 30, 2020

Conversation

smkillen
Copy link
Contributor

@smkillen smkillen commented Aug 27, 2020

Pull Request (PR) description

When declaring nginx::resource::streamhost, there is the ability to add a "resolver" option, but this is not actually honoured in the configuration, a section to test the existence of that Variable and define it in the configuration erb file is needed.

nginx::resource::streamhost {'test':
  ...
  resolver => ['255.255.255.255'],
  ...
}

should create a file in /etc/nginx/conf.stream.d/XXX-test.conf

which contains

server {
  ...
  resolver 255.255.255.255;
  ...
}

but currently does not, this code ensures this entry is present.

This Pull Request (PR) fixes the following issues

When declaring the parameter resolver as per

#   [*resolver*]            - Array: Configures name servers used to resolve
#     names of upstream servers into addresses.

for a streamhost, nothing is actually configured.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

can you please

  • explain in your pull request or commit messages, what that means, and what impact it has
  • add spec tests… should the code you're modifying be already tested in some way…

@smkillen
Copy link
Contributor Author

I have updated the PR description with details as to what this fixes.

@igalic
Copy link
Contributor

igalic commented Aug 27, 2020

we should first merge #1398

@igalic
Copy link
Contributor

igalic commented Aug 27, 2020

or alternatively, this one #1408

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Could you remove the redundant casting though?

@alexjfisher
Copy link
Member

Would you be able to add a test to spec/defines/resource_stream_spec.rb?

@@ -18,7 +18,7 @@ server {
listen [<%= @ipv6_listen_ip %>]:<%= @ipv6_listen_port %> <% if @ipv6_listen_options %><%= @ipv6_listen_options %><% end %>;
<%- end -%>
<%- end -%>
<%- if Array(@resolver).count > 0 -%>
<%- unless @resolver.empty -%>
resolver <% Array(@resolver).each do |res| %> <%= res %><% end %>;
Copy link
Member

Choose a reason for hiding this comment

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

casting is redundant here too.

@@ -18,6 +18,9 @@ server {
listen [<%= @ipv6_listen_ip %>]:<%= @ipv6_listen_port %> <% if @ipv6_listen_options %><%= @ipv6_listen_options %><% end %>;
<%- end -%>
<%- end -%>
<%- unless @resolver.empty -%>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry! There was a slight typo in what I told you.

Suggested change
<%- unless @resolver.empty -%>
<%- unless @resolver.empty? -%>

Copy link
Member

Choose a reason for hiding this comment

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

and that's why the tests are failing (and also why tests are great to have!) :)

Comment on lines 107 to 108
value: ['8.8.8.8', '8.8.4.4'],
match: %r{\s+resolver\s+8.8.8.8 8.8.4.4;}
Copy link
Member

Choose a reason for hiding this comment

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

I'm always a fan of using RFC5735 IPs.

Name Range
TEST-NET-1 192.0.2.0/24
TEST-NET-2 198.51.100.0/24
TEST-NET-3 203.0.113.0/24

Copy link
Contributor

Choose a reason for hiding this comment

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

i can't believe no one's tried to abolish and sell these yet

…ts]-[SMK]

[Correcting typo]-[SMK]

[Removing redundant casting as requested]-[SMK]

Co-authored-by: Alexander Fisher <[email protected]>

[Removing additional casting]-[SMK]

[Adding tests for resolvers]-[SMK]

[Correcting typo in the Template testing array is empty and changing IPs to RFC5735]-[SMK]
@alexjfisher alexjfisher added enhancement New feature or request and removed needs-squash labels Aug 30, 2020
@alexjfisher alexjfisher merged commit f042e75 into voxpupuli:master Aug 30, 2020
@alexjfisher
Copy link
Member

@smkillen Thanks!

@smkillen
Copy link
Contributor Author

smkillen commented Aug 30, 2020 via email

Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Adding resolver entries to streamhost.erb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants