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

Add recipe for sqlserver #425

Merged
merged 4 commits into from May 3, 2017

Conversation

Projects
None yet
2 participants
@mlcooper
Copy link
Contributor

commented Apr 15, 2017

No description provided.

@mlcooper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2017

Hi @olivielpeau
Travis failed with some rubocop errors on files unrelated to this PR. I'm running rubocop 0.47.1 locally and am unable to reproduce the errors that travis found.

@mlcooper

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2017

This PR will hopefully satisfy #333

@olivielpeau
Copy link
Member

left a comment

Had one question, other than that this looks good to me, thanks @mlcooper!

I'll spend some time to fix the CI

- host: HOST,PORT
username: my_username
password: my_password
<% if @init_config.empty? -%>

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Apr 20, 2017

Member

Shouldn't <%= JSON.parse(({'init_config' => @init_config, 'instances' => @instances}).to_json).to_yaml %> work as expected even when @init_config is empty?

(wondering if we could get rid of the if clause completely)

This comment has been minimized.

Copy link
@mlcooper

mlcooper Apr 23, 2017

Author Contributor

I did try that, however when I just use <%= JSON.parse(({'init_config' => @init_config, 'instances' => @instances}).to_json).to_yaml %>, when @init_config is empty, the chef spec test fails with:

1) datadog::sqlserver config 2 renders expected YAML config file
     Failure/Error: expect(YAML.load(content).to_json).to be_json_eql(YAML.load(expected_yaml).to_json)
     
       Expected equivalent JSON
       Diff:
       @@ -1,5 +1,6 @@
        {
       -  "init_config": null,
       +  "init_config": {
       +  },
          "instances": [
            {
              "command_timeout": 30,
       
     # ./spec/integrations/sqlserver_spec.rb:135:in `block (4 levels) in <top (required)>'
     # /home/coopem67/.chefdk/gem/ruby/2.3.0/gems/chefspec-5.4.0/lib/chefspec/matchers/render_file_matcher.rb:120:in `matches_content?'
     # /home/coopem67/.chefdk/gem/ruby/2.3.0/gems/chefspec-5.4.0/lib/chefspec/matchers/render_file_matcher.rb:13:in `matches?'
     # ./spec/integrations/sqlserver_spec.rb:134:in `block (3 levels) in <top (required)>'

Finished in 5.16 seconds (files took 5.76 seconds to load)
20 examples, 1 failure

Failed examples:

rspec ./spec/integrations/sqlserver_spec.rb:133 # datadog::sqlserver config 2 renders expected YAML config file

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau May 2, 2017

Member

Thanks @mlcooper for the explanation.

Since the behavior of having init_config set to the empty map ({}) instead of null is not a problem per se, what do you think of removing the if clause from the template and setting the first line of the expected yaml in the spec test to init_config: {}?

@olivielpeau olivielpeau added this to the 2.10.0 milestone Apr 20, 2017

@olivielpeau olivielpeau added the feature label Apr 20, 2017

@mlcooper

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2017

Hi @olivielpeau I got rid of the if statement since it's not a problem to have {} for init_config. Travis failed again however, due to an issue downloading a gpg key from Chef it seems.

@olivielpeau

This comment has been minimized.

Copy link
Member

commented May 3, 2017

Thanks, looks ready to go!

The CI failure was transient, a re-run fixed it. Merging

@olivielpeau olivielpeau merged commit 5d9b2fa into DataDog:master May 3, 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.