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.

Sign up for GitHub

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

Jump to bottom

add extra_template back #331

Merged
merged 1 commit into from
Jul 20, 2017
Merged

add extra_template back #331

merged 1 commit into from
Jul 20, 2017

Conversation

flyinprogrammer
Copy link
Contributor

@flyinprogrammer flyinprogrammer commented Jun 9, 2017 •
edited
Loading

Fix regression in #261
where an extra_template was no longer getting concat'd to the template file.

@truthbk
Copy link
Member

truthbk commented Jun 16, 2017

@flyinprogrammer first of all, apologies for the regression and thank you for this awesome fix. This is ready to go, but I'd like to ask you for a favor.... Could you add a spec test for this? Just to ensure both the extra template and the apm footer are dropped in correctly (with and without an extra template being defined).

Let me know if you don't have the bandwidth

@truthbk truthbk added this to the 1.11.0 milestone Jun 16, 2017
@flyinprogrammer
Copy link
Contributor Author

yeah let's see what i can get done with this tonight

flyinprogrammer
flyinprogrammer commented Jul 19, 2017
View reviewed changes
Gemfile Outdated
@@ -18,4 +18,5 @@ group :development do
gem "puppet-blacksmith"
gem "nokogiri", "~> 1.6.0"
gem "guard-rake"
gem "xmlrpc"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise I kept getting:

Could not autoload puppet/type/package: Could not autoload puppet/provider/package/pip: cannot load such file -- xmlrpc/client

when running the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

xmlrpc-0.3.0 requires ruby version >= 2.3, which is incompatible with the
current version, ruby 2.0.0p648

this is why i write golang now. hld plz.

flyinprogrammer
flyinprogrammer commented Jul 19, 2017
View reviewed changes
spec/classes/datadog_agent_spec.rb
@@ -593,6 +596,20 @@
'content' => /^sd_jmx_enable: true\n/,
)}
end
context 'with extra_template enabled' do
let(:params) {
{:extra_template => 'custom_datadog/extra_template_test.erb',
Copy link
Contributor Author

@flyinprogrammer flyinprogrammer Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think I added this fixture correctly, I know we ignore this directory in .gitignore, but it seemed like the correct thing to do? Please advise!

Copy link
Member

@truthbk truthbk Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This might be the right thing to do.... it feels like it's the right location, but we'd want to have it outside of the .gitignore blacklist.... that might not be possible though:

from the docs

It is not possible to re-include a file if a parent directory of that file is excluded

We'll have to come up with something I guess.... maybe an alternative location to fixtures 🤔

@flyinprogrammer
Copy link
Contributor Author

idk how you want to fix:

Requested binary installation but no rubies are available to download, consider skipping --binary flag.
Gemset '' does not exist, 'rvm ruby-2.1.0 do rvm gemset create ' first, or append '--create'.

@truthbk
Copy link
Member

truthbk commented Jul 19, 2017

@flyinprogrammer looks like a problem with the current test matrix on Travis. Looking into it. Thanks a bunch for the tests 💯

@truthbk
Copy link
Member

truthbk commented Jul 19, 2017

Fixed the Travis issue, could you please rebase to the latest master?

truthbk
truthbk reviewed Jul 19, 2017
View reviewed changes
Gemfile Outdated
@@ -18,4 +18,5 @@ group :development do
gem "puppet-blacksmith"
gem "nokogiri", "~> 1.6.0"
gem "guard-rake"
gem "xmlrpc" if RUBY_VERSION >= '2.3'
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate a bit on this (for posterity)?

Copy link
Contributor Author

@flyinprogrammer flyinprogrammer Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

locally I'm running the tests against ruby 2.4.1p111 and in order to get the tests to pass you need to install this gem. of course when your ruby is < 2.3 the builds will barf apparently.

here are my ramblings from previous builds that failed: https://github.com/DataDog/puppet-datadog-agent/pull/331/files/bfe9b22bbbc2527c8a18f57519a26f6793e36e4e#r128139904

@truthbk
Copy link
Member

truthbk commented Jul 19, 2017 •
edited
Loading

OK - so this is how I addressed the issue:

diff --git a/.fixtures.yml b/.fixtures.yml
index 21cff47..5d7a8ec 100644
--- a/.fixtures.yml
+++ b/.fixtures.yml
@@ -9,4 +9,5 @@ fixtures:
     ruby: "git://github.com/puppetlabs/puppetlabs-ruby.git"
     remote_file: "git://github.com/lwf/puppet-remote_file.git"
   symlinks:
+    custom_datadog: "#{source_dir}/spec/custom_fixtures/custom_datadog"
     datadog_agent: "#{source_dir}"
diff --git a/Gemfile b/Gemfile
index 5e72eb7..11ccd1a 100644
--- a/Gemfile
+++ b/Gemfile
@@ -3,6 +3,7 @@ source "https://rubygems.org"
 group :test do
   gem "syck"
   gem "safe_yaml", "~> 1.0.4"
+  gem "hiera-eyaml"
   gem "listen", "~> 3.0.0"
   gem "puppet", ENV['PUPPET_VERSION'] || '~> 4.2.0'
   gem "puppet-lint"
diff --git a/spec/fixtures/modules/custom_datadog/templates/extra_template_test.erb b/spec/custom_fixtures/custom_datadog/templates/extra_template_test.erb
similarity index 100%
rename from spec/fixtures/modules/custom_datadog/templates/extra_template_test.erb
rename to spec/custom_fixtures/custom_datadog/templates/extra_template_test.erb

I copied spec/fixtures/modules/custom_datadog/templates/extra_template_test.erb to spec/custom_fixtures/custom_datadog/templates/extra_template_test.erb since spec/fixtures is considered ephemeral.

And I then fixed up .fixtures.yml as you can see in the diff. I had to add the hiera-eyaml change to gemfile, not sure if you might need it too to get the tests to fly.

Let me know if you don't like this approach. :)

@flyinprogrammer
Copy link
Contributor Author

I think this is great and I think you're great @truthbk !! Thanks for teaching me how to do the fixtures, and I'm sorry for kinda being lazy and not digging into the docs more. I rebased off master and squashed my commits.

truthbk
truthbk approved these changes Jul 20, 2017
View reviewed changes
Copy link
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.

This looks good to me! Thanks so much for taking the time to add and fix the tests - it's awesome to have these things tested out. 🙇

@truthbk truthbk merged commit 15c67f7 into DataDog:master Jul 20, 2017
cegeka-jenkins pushed a commit to cegeka/puppet-datadog_agent that referenced this pull request Jan 31, 2018
@flyinprogrammer @truthbk
add extra_template back (DataDog#331)
d091cc9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@truthbk truthbk truthbk approved these changes

Assignees
No one assigned
Labels
2/ Confirmed community
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.
2 participants
@flyinprogrammer @truthbk