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

Address #194 for nginx #417

Merged
merged 2 commits into from Mar 31, 2017

Conversation

Projects
None yet
2 participants
@iancward
Copy link
Contributor

iancward commented Mar 27, 2017

Update nginx integration template with features expressed in current integration.

Address #194 for nginx
Update nginx integration template with features expressed in current integration
@olivielpeau
Copy link
Member

olivielpeau left a comment

Thanks @iancward! Made one comment but other than that this change looks good.

In the future we'll want to have a spec test covering this integration recipe/template

@@ -1,6 +1,12 @@
instances:
<% @instances.each do |i| -%>
- nginx_status_url: <%= i['nginx_status_url'] %>
<%= "ssl_validation: #{i['ssl_validation']}" unless i['ssl_validation'].nil? -%>

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Mar 28, 2017

Member

IMO the indentation and format of this line could break easily if we make changes to it in the future, could you rather write:

<% unless i['ssl_validation'].nil? -%>
    ssl_validation: i['ssl_validation']
<% end -%>

What do you think?

This comment has been minimized.

Copy link
@iancward

iancward Mar 28, 2017

Author Contributor

I waffled on whether to do it that way, but now that I think about it, what you put above will also be more consistent with the rest of the template. Let me take care of that.

This comment has been minimized.

Copy link
@iancward

iancward Mar 28, 2017

Author Contributor

Oh! I had not even thought of chefspec; let me add that as well.

This comment has been minimized.

Copy link
@iancward

iancward Mar 30, 2017

Author Contributor

@olivielpeau I've pushed a5e1f25 with the recommended changes; I also added spec tests and updated the examples in the nginx recipe to demonstrate the new features.

Improvements per PR #417 feedback
- For nginx integration template: use use block instead of inline 'unless'
- For nginx integration recipe: Add ssl_validation and user/password to examples
- Add spec test coverage for nginx integration
@olivielpeau
Copy link
Member

olivielpeau left a comment

Thanks a bunch @iancward!

The spec tests are great 👌 , thanks again for these

@olivielpeau olivielpeau added this to the 2.10.0 milestone Mar 31, 2017

@olivielpeau olivielpeau merged commit 35c3de1 into DataDog:master Mar 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.