Skip to content

set disable_ssl_validation parameter#258

Merged
truthbk merged 3 commits intomasterfrom
scott/http-check-instance-fix
Nov 15, 2016
Merged

set disable_ssl_validation parameter#258
truthbk merged 3 commits intomasterfrom
scott/http-check-instance-fix

Conversation

@sjenriquez
Copy link
Copy Markdown
Contributor

If an array of instances is declared in the main manifest disable_ssl_validation is not set, this PR fixes that bug and adds an example.

<% if instance['disable_ssl_validation'] -%>
disable_ssl_validation: <%= instance['disable_ssl_validation'] %>
<% else %>
disable_ssl_validation: false
Copy link
Copy Markdown
Member

@truthbk truthbk Nov 14, 2016

Choose a reason for hiding this comment

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

We should just remove this.... the if here in line 31 should be enough - just check if 'disable_ssl_validation' key is defined in the hash and then drop in the value.

@sjenriquez
Copy link
Copy Markdown
Contributor Author

@truthbk updated with same style as other key checks for consistency.

collect_response_time: <%= instance['collect_response_time'] %>
<% end -%>
disable_ssl_validation: <%= instance['disable_ssl_validation'] %>
<% if instance['disable_ssl_validation'] -%>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this will fail if the key doesn't exist... can you try with has_key(instance, 'disable_ssl_validation') instead?

Copy link
Copy Markdown
Contributor Author

@sjenriquez sjenriquez Nov 14, 2016

Choose a reason for hiding this comment

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

I tested successfully with this config in the manifest:

node 'ip-0-0-0-0.ec2.internal' {
    class { "datadog_agent":
        api_key => "key"
    }

    class { 'datadog_agent::integrations::http_check':
      instances => [{
        'sitename'  => 'google',
        'url'       => 'http://www.google.com',
        'tags'      => 'site:google'
      },
      {
        'sitename'  => 'datadog',
        'url'       => 'http://www.datadoghq.com',
        'tags'      => 'site:dog'
      }]
    }
}

We only add parameters when they are found. Another alternative would be to use:
if instance.has_key?("disable_ssl_validation") although I used the above for consistency. What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nil evaluates as false:

irb(main):001:0> instance = {"sitename"=>"google", "url"=>"http://www.google.com", "tags"=>"site:google"}
=> {"sitename"=>"google", "url"=>"http://www.google.com", "tags"=>"site:google"}
irb(main):002:0> instance['disable_ssl_validation']
=> nil
irb(main):003:0> if instance['disable_ssl_validation']; puts 'TRUE' ; else puts 'FALSE' ; end
FALSE
=> nil

Copy link
Copy Markdown
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

I think the current template will fail in some cases...

Copy link
Copy Markdown
Member

@truthbk truthbk 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 checking it out @sjenriquez! Merging!

@truthbk truthbk merged commit 6d63b24 into master Nov 15, 2016
@truthbk truthbk deleted the scott/http-check-instance-fix branch November 15, 2016 23:03
cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Jan 31, 2018
…-fix

set disable_ssl_validation parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants